Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove [Conformance] flag on some e2es #34140

Merged
merged 1 commit into from
Oct 11, 2016

Conversation

timothysc
Copy link
Member

@timothysc timothysc commented Oct 5, 2016

Downstream distributions that absorb the upstream tests would like to give their customers a standard mechanism to validate their clusters, post setup. As of today [Conformance] works for most things, but there are a known set of tests that vary due to opinionated differences around networking, security, etc... and providing a complete skip list can be cumbersome. To address this, we've simply modified the flag on some tests to [Conformance:Variant]. All existing behavior should be maintained.

Fixes: #34105


This change is Reviewable

@timothysc timothysc added this to the v1.5 milestone Oct 5, 2016
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Oct 5, 2016
@timothysc timothysc added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 5, 2016
@spxtr
Copy link
Contributor

spxtr commented Oct 5, 2016

I'm not a big fan. Is anyone against just removing the conformance tag from these tests?

@timothysc
Copy link
Member Author

/cc @kubernetes/sig-testing for comments.

@timothysc
Copy link
Member Author

I'm open to alternatives, but I don't think some of these tests should remove [Conformance] from an upstream perspective. It's just that they're not guaranteed to function downstream given the description above.

@spxtr
Copy link
Contributor

spxtr commented Oct 5, 2016

I'm also afraid that we're matching the regex \[Conformance\] which might be a silent problem.

@timothysc
Copy link
Member Author

We've federated out repos, I'm not entirely certain all the places to look.

@timothysc
Copy link
Member Author

@spxtr This is a priority for us, would [Variant] be ok?

@spxtr
Copy link
Contributor

spxtr commented Oct 6, 2016

Conformance:Variant or just Variant don't really say anything about the test, though. Are the tests themselves assuming too much about the cluster? If so, we should rework the tests or remove the conformance label, whichever is most appropriate.

We don't actually use the conformance label anywhere in the kubernetes repo, so the change doesn't make a big deal to us, but it smells.

@timothysc
Copy link
Member Author

timothysc commented Oct 10, 2016

Are the tests themselves assuming too much about the cluster?

I think there are opinionated differences in deployments that can not easily be resolved. The problem today is upstream only tests/validates 1 configuration, and downstream distributions (e.g. Openshift) have made other choices which have now made the test no longer a [Conformance] test for that environment.

So I'm ok with the following:

  1. Remove Conformance for those tests
  2. Adding a label of some kind to denote skipping [Variant], [Conformance:Variant], whatever.
  3. Some other solution which allows an easy -skip regex

@spxtr
Copy link
Contributor

spxtr commented Oct 10, 2016

Can you give an example of an opinionated difference? For instance, should provide unchanging, static URL paths for kubernetes api services seems like it just does a GET request to a few standard paths. Why doesn't that work on Openshift?

@spxtr
Copy link
Contributor

spxtr commented Oct 10, 2016

Is it possible to do some sort of SkipIfProviderIs("openshift") or something along those lines?

@timothysc
Copy link
Member Author

Can you give an example of an opinionated difference?

It("should proxy to cadvisor [Conformance:Variant]", func() { nodeProxyTest(f, prefix+"/proxy/nodes/", ":4194/containers/")

We lock down our node tightly to not expose any potential attack surface.

Is it possible to do some sort of SkipIfProviderIs("openshift") or something along those lines?

No, openshift is not a "provider" it is local

Moving the change to [Variant], keeps all the current testing conventions with little/no negative upstream impact, what is the logical argument against it?

@spxtr
Copy link
Contributor

spxtr commented Oct 10, 2016

The argument is just that Variant doesn't say anything specific about the test and it will be confusing for everyone else. What we need is some way to say "skip this test if we're on openshift".

@timothysc
Copy link
Member Author

timothysc commented Oct 10, 2016

The 3 options provided earlier...

  1. Remove Conformance for those tests
  2. Adding a label of some kind to denote downstream divergence and document, e.g [Variant], [Conformance:Variant], whatever, I don't really care what the name is. Conventions are whatever we decide, and can be applied to tests however @kubernetes/sig-testing sees fit... What does [Conformance] mean? It only means something to those who bought into the conventions. If we decide the the convention is to call it [ScoobyDoo] then we document it and move along. Obviously we try to pick a name that makes sense... If you don't like variant, please offer a suggestion that seems appropriate.
  3. Some other solution which allows an easy -skip regex, provider is not an option.

@spxtr
Copy link
Contributor

spxtr commented Oct 10, 2016

Why is provider not an option?

@timothysc
Copy link
Member Author

Why is provider not an option?

Provider in the tests, and other areas, is linked to cloud-provider, which openshift supports many. So provider has an known implementation, and connotation in the code.

@spxtr
Copy link
Contributor

spxtr commented Oct 10, 2016

It sounds like we should remove the conformance label, then. This comes up every now and then in @kubernetes/sig-testing.

@timothysc timothysc changed the title Add a extra [Conformance:Variant] flag on some e2es remove [Conformance] flag on some e2es Oct 10, 2016
@timothysc
Copy link
Member Author

It sounds like we should remove the conformance label, then.

Done.

@spxtr
Copy link
Contributor

spxtr commented Oct 10, 2016

I'm fine with this. Anyone from @kubernetes/sig-testing disagree? If not I'll LGTM tomorrow.

@spxtr
Copy link
Contributor

spxtr commented Oct 11, 2016

@k8s-bot gce e2e test this

#31561

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit eb29c61. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@timothysc
Copy link
Member Author

looks like infra-flakes.

@spxtr
Copy link
Contributor

spxtr commented Oct 11, 2016

@k8s-bot test this

kubernetes/test-infra#798

@spxtr spxtr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit eb29c61. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit ff43515 into kubernetes:master Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants