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

Add minimum version check #9

Closed
wants to merge 1 commit into from
Closed

Conversation

dankohn
Copy link
Contributor

@dankohn dankohn commented Oct 6, 2017

I'm looking for a few LGTMs from the WG before merging this.

@WilliamDenniss
Copy link
Member

LGTM

To avoid needing to revise every release, we could say Y >= N-1 where N is the current minor release of Kubernetes.

@smarterclayton
Copy link
Contributor

I'm not sure why the client and server have to exactly match. For instance I had expected to certify the opensource Kubernetes CLI against openshift given that the tests expose specific kubectl behaviors, and that also proves that the kubectl is properly interoperating with the server version.

@kitch
Copy link
Contributor

kitch commented Oct 17, 2017

@smarterclayton Agreed they don't HAVE to match to work. Although leaving the versioning requirement to be arbitrary seems like a tiny pandoras box. I'm all for the spec stating client/server should be equal just so it is not ambiguous.

@dankohn
Copy link
Contributor Author

dankohn commented Oct 18, 2017

@smarterclayton Could you please propose alternative language?

@smarterclayton
Copy link
Contributor

Possibly:

  1. The kubectl client version used to perform the conformance test must either:
    1. match the version of Kubernetes that provides the e2e test - verifying that an unmodified Kubernetes client can correctly interact with the provided Kubernetes implementation
    2. match the version of Kubernetes provided by the server - verifying that the combination of the provider's kubectl and the provider's Kubernetes implementation is conformant with the expected behavior of the test

My concern with 1.2. is that it is not verifying that unmodified client ecosystem code can interoperate with the provided Kubernetes server. 1.1. asserts that a server implementation conforms both to the direct e2e calls made by the server, AND that an unmodified kubectl can also interoperate. 1.2. does not test as extensively as 1.1. that the server provides the same behavior because the provider's kubectl may work around non-conformant behavior.

@smarterclayton
Copy link
Contributor

I'm ok with the minor version, just wanted to highlight that accepting a kubectl binary from the provider may mask other conformance issues (you must download our kubectl in order to pass the conformance tests feels... wrong).

@WilliamDenniss
Copy link
Member

Agree that "you must use our kubectl in order to pass the conformance tests" is wrong. Such a requirement would break interop, and thus goes against the goal of conformance.

Does your currently proposed text remove this risk, or do we need to tune it further?

@smarterclayton
Copy link
Contributor

It doesn't. I think in the spirit of being somewhat flexible to conforming implementations we would want "use of a kubectl binary built from the Kubernetes project source in the current minor version's release stream alongside the e2e test suite built by that release stream".

A loose reviewer standard would be to verify that the reported Git commit from the kubectl client version matches a commit in the upstream Kubernetes repository that is part of the current minor version release branch (i.e. the commit must satisfy git branch --contains CLIENT_REPORTED_COMMIT | grep release-MAJOR.MINOR).

A more strict reviewer standard would be to confirm that the conformance submitter is using an upstream binary (via downloading the binary from the appropriate release archive or building from Kube source). This feels somewhat onerous - I suppose if people abuse it it's easy to identify in their reproduction notes.

@dankohn
Copy link
Contributor Author

dankohn commented Oct 20, 2017

Note that we're trying to minimize the effort and judgment involved for CNCF staff reviewing applications.

That said, if we think your suggestion is the most thorough, it seems like we could add a flag to sonobuoy that would execute that git branch command and include it in the output.

@@ -1,5 +1,5 @@
# How to review conformance results

1. Make sure the list of files matches [the one specified here](https://github.com/cncf/k8s-conformance/blob/master/instructions.md#contents-of-the-pr)
2. Look at `version.txt`. Make sure the `client` and `server` versions match each other to the minor version level. Make sure this also matches the `vX.Y` subdirectory that the PR is in.
2. Look at `version.txt`. Make sure the `client` and `server` versions match each other to the minor version level. Make sure this also matches the `vX.Y` subdirectory that the PR is in. Also confirm that Y >= 7 (that is, certification is available for Kubernetes versions 1.7 and higher).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client minor version(s) can skew by 1 minor revision.
So 1.8 clients, and e2es, should properly exercise 1.7 clusters.

@dankohn
Copy link
Contributor Author

dankohn commented Oct 31, 2017

Closing unmerged in leiu if #33.

@dankohn dankohn closed this Oct 31, 2017
@dankohn dankohn deleted the dankohn-minimum-version branch November 3, 2017 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants