Multiversion client tests #25566

soltysh · 2016-05-13T09:24:06Z

@caesarxuchao this is a follow up to the discussion we've had while I was working on the JobTemplate, esp. the part about introducing batch/v2alpha1 (#21675). I've split this from #25475 not to block progress on ScheduledJobs, so look only at the last commit. We need some sort of solution for the multi-version client testing and I'm willing to work on it but will need your help.

In the aforementioned commit I've extended simpletest_client.go's Config withGroupVersionwhich allows you to specify which version you want to test. This required some additional changes intestapi.goto be able to specify which specific version we're testing. Seejobs_test.go, since with those changes I was easily able to test all possible versionsJob` exists in.

I'm open to suggestions how to improve the code, this is just a dirty plumbing I did to make the test work, while mostly focusing on ScheduledJobs.

@erictune also mentioned there's a plan to get rid of manually writing those clients, in that case it might make sense just to ignore the problems for now and wait for the generated clients make their way. The only question is when will that happen?

k8s-bot · 2016-05-13T09:33:37Z

GCE e2e build/test failed for commit a78a6ec.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

deads2k · 2016-05-13T12:14:02Z

pkg/api/testapi/testapi.go

 	if serializer.Serializer == nil {
+		if g.IsEnabledVersion(gv) {


If I requested a specific version and I can't have it, shouldn't I fail?

Yeah the overall lack of error handling or panic (it's just tests) is something I've struggled while working on this.

caesarxuchao · 2016-05-13T17:05:56Z

@soltysh could you remind me the context? I don't remember why can't we use the KUBE_TEST_API_VERSIONS environment variable to control what version to test.

soltysh · 2016-05-13T18:55:04Z

@caesarxuchao the problem with enabling just batch/v2alpha1 with KUBE_TEST_API_VERSIONS is that when that happens batch/install.go still installs all the versions at once. Since batch/v1 is the preferred version it is installed along with batch/v2alpha1. In that cases we need to force specific version in tests, especially with ScheduledJobs since these are available only in batch/v2alpha1. In that case the tests must be conditional, iow. just for that specific version, not just group. Generally this is the first place where we actually introduce 2nd version in any of the groups.

caesarxuchao · 2016-05-13T21:19:38Z

the problem with enabling just batch/v2alpha1 with KUBE_TEST_API_VERSIONS is that when that happens batch/install.go still installs all the versions at once. Since batch/v1 is the preferred version it is installed along with batch/v2alpha1

I must have missed something: it seems if you only put batch/v2alpha1 in KUBE_TEST_API, you will only get that version from the testapi. (https://github.com/kubernetes/kubernetes/blob/master/pkg/api/testapi/testapi.go#L94)
In the tests that should only be run for v2alpha1, you can add the following if statement at the beginning of your test:

if testapi.Batch.GroupVersion != "batch/v2alpha1" {
  return
}

Sorry I cannot recall our discussion in the other PR..

soltysh · 2016-05-13T21:32:23Z

I must have missed something: it seems if you only put batch/v2alpha1 in KUBE_TEST_API, you will only get that version from the testapi. (https://github.com/kubernetes/kubernetes/blob/master/pkg/api/testapi/testapi.go#L94)

That's not quite the way it works, iirc. I can't remember the full details, since that was a while, but in general this line will allow only this version, but since batch/install.go is called it installs all the versions internally and that is later the root cause of all the problems.

I think the case was for the situation where you don't specify any of batch versions and then all are installed, which is why the tests were failing (and one of the groups installs extensions but no batch, see the first group in KUBE_TEST_API_VERSIONS.

caesarxuchao · 2016-05-13T21:41:26Z

Just to reiterate, so you agree that if only batck/v1beta2 is specified in "KUBE_API_TEST", then there will be no problem?

and one of the groups installs extensions but no batch, see the first group in KUBE_TEST_API_VERSIONS

In that case, calling testapi.Batch.GroupVersion() should give you "batch/v1beta1", and the if statement I mentioned should help you skip the tests.

soltysh · 2016-05-13T21:48:54Z

Hmmm... lemme check that again. I think there was more than just that.

soltysh · 2016-05-13T22:25:26Z

@caesarxuchao so here's the problem I've stumbled upon, when I've enabled just batch/v1 I'm getting following test error:

--- FAIL: TestDiscoveryAtAPIS (0.42s)
    Error Trace:    master_test.go:475
    Error:      Not equal: []unversioned.GroupVersionForDiscovery{unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v1", Version:"v1"}} (expected)
                    != []unversioned.GroupVersionForDiscovery{unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v1", Version:"v1"}, unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v2alpha1", Version:"v2alpha1"}} (actual)

            Diff:
            --- Expected
            +++ Actual
            @@ -1,3 +1,4 @@
            -([]unversioned.GroupVersionForDiscovery) (len=1 cap=1) {
            - (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v1" version:"v1" 
            +([]unversioned.GroupVersionForDiscovery) (len=2 cap=4) {
            + (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v1" version:"v1" ,
            + (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v2alpha1" version:"v2alpha1" 
             }

    Error Trace:    master_test.go:506
    Error:      Not equal: []unversioned.GroupVersionForDiscovery{unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v1", Version:"v1"}} (expected)
                    != []unversioned.GroupVersionForDiscovery{unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v1", Version:"v1"}, unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v2alpha1", Version:"v2alpha1"}} (actual)

            Diff:
            --- Expected
            +++ Actual
            @@ -1,3 +1,4 @@
            -([]unversioned.GroupVersionForDiscovery) (len=1 cap=1) {
            - (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v1" version:"v1" 
            +([]unversioned.GroupVersionForDiscovery) (len=2 cap=4) {
            + (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v1" version:"v1" ,
            + (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v2alpha1" version:"v2alpha1" 
             }

I remember I was trying to fix it, but failed badly. See that although batch/v1 is forced through KUBE_TEST_API_VERSIONS both versions get actually installed. Without the multiple versions (I've done so far) only the latest registered version got in. In all cases (for every batch version) that used to be batch/v2alpha1, being the last one. I'm open to any ideas.

soltysh · 2016-05-13T22:31:46Z

Identical problems in pkg/master/ are when just running batch/v2alpha1:

--- FAIL: TestDiscoveryAtAPIS (0.49s)
    Error Trace:    master_test.go:475
    Error:      Not equal: []unversioned.GroupVersionForDiscovery{unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v2alpha1", Version:"v2alpha1"}} (expected)
                    != []unversioned.GroupVersionForDiscovery{unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v1", Version:"v1"}, unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v2alpha1", Version:"v2alpha1"}} (actual)

            Diff:
            --- Expected
            +++ Actual
            @@ -1,2 +1,3 @@
            -([]unversioned.GroupVersionForDiscovery) (len=1 cap=1) {
            +([]unversioned.GroupVersionForDiscovery) (len=2 cap=4) {
            + (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v1" version:"v1" ,
              (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v2alpha1" version:"v2alpha1" 

    Error Trace:    master_test.go:506
    Error:      Not equal: []unversioned.GroupVersionForDiscovery{unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v2alpha1", Version:"v2alpha1"}} (expected)
                    != []unversioned.GroupVersionForDiscovery{unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v1", Version:"v1"}, unversioned.GroupVersionForDiscovery{GroupVersion:"batch/v2alpha1", Version:"v2alpha1"}} (actual)

            Diff:
            --- Expected
            +++ Actual
            @@ -1,2 +1,3 @@
            -([]unversioned.GroupVersionForDiscovery) (len=1 cap=1) {
            +([]unversioned.GroupVersionForDiscovery) (len=2 cap=4) {
            + (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v1" version:"v1" ,
              (unversioned.GroupVersionForDiscovery) groupVersion:"batch/v2alpha1" version:"v2alpha1"

soltysh · 2016-05-13T22:32:26Z

Maybe I'm missing a better solution here, I don't deny it, that's why I've opened this issue to discuss possible approaches :)

soltysh · 2016-05-13T22:54:53Z

After second thought, we could install in the testapi only the version that is specified and ignore everything else. I need to give it a try, since it looks like it might help here.

soltysh · 2016-05-16T15:10:49Z

Current state is that I've reverted the changes I've did in pkg/api/testapi/testapi.go to a single externalGroupVersion, like before and modified pkg/master/master_test.go to pass the tests. I've introduced 3rd KUBE_TEST_API_VERSIONS group with batch/v2alpha. The only problem I'm trying to address right now is the serialization tests. Hopefully I'll be able to fix it and re-do this PR with a better approach (or probably open a new one).

caesarxuchao · 2016-05-16T15:38:36Z

Thanks @soltysh! Yes, those master_test should only be run for a certain KUBE_API_TEST_VERSION.

@erictune

Automatic merge from submit-queue Scheduledjob storage This builds on top of #25475 so only last commit is significant. There's still one small problem with conversions, I'm currently working, but the biggest is still multi-version client tests. Read on... @erictune unfortunately the multi-version tests are biting again, this time in `pkg/registry/scheduledjob/etcd`. If I run the tests with just `batch/v2alpha1` then these are working correctly, but we can't have only `batch/v2alpha1` enabled since `batch/v1` is the preferred version for the entire `batch/` group. We either going to skip the tests here like we did with the scheduledjob client (I'm talking about `pkg/registry/scheduledjob/etcd/etcd_test.go`) or fix it. I've talked with @deads2k about how we can plumb the tests so it just passes, but there's no simple solution we can come up with 😞 (other than the one from #25566), unless @caesarxuchao can chime in and propose something. ```release-note Introducing ScheduledJobs as described in [the proposal](https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/scheduledjob.md) as part of `batch/v2alpha1` version (experimental feature). ``` [![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/.github/PULL_REQUEST_TEMPLATE.md?pixel)]()

@caesarxuchao

Automatic merge from submit-queue ScheduledJob tests This builds on top of #25569, so only the last two commits matter ([Revert commit a31ca0d and move batch/v2alpha1 tests to separate group](e7f6ba7) and [ScheduledJob client and storage tests](c59c045)). This also supersedes #25566 with simpler approach to testing, by testing single version at a time. @caesarxuchao I've reverted the changes I've did previously to enable multi-version tests and introduced new group where `batch/v2alpha1` is tested (see 1st commit). I've also added back the necessary tests for scheduledjob related stuff (2nd commit) that are already in queue (@erictune fyi - I can't stand having code without tests 😉). [![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/.github/PULL_REQUEST_TEMPLATE.md?pixel)]()

soltysh added 6 commits May 12, 2016 22:24

ScheduledJob api leftovers

5ad78a9

ScheduledJob api leftovers - generated

7eda522

Adding github.com/robfig/cron for cron parsing

d9cfd61

ScheduledJob validation & defaults

ca0c4ab

ScheduledJob client

fa90da5

Enable multi-version tests in client tests

a78a6ec

googlebot added the cla: yes label May 13, 2016

k8s-github-robot assigned deads2k May 13, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 13, 2016

deads2k assigned caesarxuchao and unassigned deads2k May 13, 2016

deads2k reviewed May 13, 2016
View reviewed changes

soltysh mentioned this pull request May 13, 2016

Scheduledjob storage #25569

Merged

soltysh mentioned this pull request May 17, 2016

ScheduledJob tests #25737

Merged

soltysh closed this May 17, 2016

soltysh deleted the multiversion_client_tests branch September 12, 2016 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiversion client tests #25566

Multiversion client tests #25566

soltysh commented May 13, 2016

k8s-bot commented May 13, 2016

deads2k May 13, 2016

soltysh May 13, 2016

caesarxuchao commented May 13, 2016

soltysh commented May 13, 2016

caesarxuchao commented May 13, 2016

soltysh commented May 13, 2016

caesarxuchao commented May 13, 2016 •

edited

Loading

soltysh commented May 13, 2016

soltysh commented May 13, 2016

soltysh commented May 13, 2016

soltysh commented May 13, 2016

soltysh commented May 13, 2016

soltysh commented May 16, 2016

caesarxuchao commented May 16, 2016

		if serializer.Serializer == nil {
		if g.IsEnabledVersion(gv) {

Multiversion client tests #25566

Multiversion client tests #25566

Conversation

soltysh commented May 13, 2016

k8s-bot commented May 13, 2016

deads2k May 13, 2016

Choose a reason for hiding this comment

soltysh May 13, 2016

Choose a reason for hiding this comment

caesarxuchao commented May 13, 2016

soltysh commented May 13, 2016

caesarxuchao commented May 13, 2016

soltysh commented May 13, 2016

caesarxuchao commented May 13, 2016 • edited Loading

soltysh commented May 13, 2016

soltysh commented May 13, 2016

soltysh commented May 13, 2016

soltysh commented May 13, 2016

soltysh commented May 13, 2016

soltysh commented May 16, 2016

caesarxuchao commented May 16, 2016

caesarxuchao commented May 13, 2016 •

edited

Loading