-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Adding Conformance Guidelines document. #1867
Conversation
525a68e
to
1f63d12
Compare
/ok-to-test |
1 similar comment
/ok-to-test |
@WilliamDenniss @duglin @bradtopol Please review conformance documentation. |
The Kubernetes conformance test suite is a set of testcases, currently a | ||
subset of the integration/e2e tests, that the Architecture SIG has approved | ||
to define the core set of interoperable features that all Kubernetes | ||
deployments must support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conformance tests in the default profile should cover features that are:
- GA (not alpha or beta APIs, nor deprecated features)
- portable (not dependent on provider-specific capabilities or on the public internet)
- not an optional feature (e.g. not policy enforcement)
- non-privileged (neither root on nodes, network, nor cluster)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also:
- Cannot be flaky
- Cannot skip providers (offhand, I'd say there should be no Skip... directives)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in the Nucleus or Application layers.
https://github.com/kubernetes/community/blob/master/contributors/devel/architectural-roadmap.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would suggest a sentence immediately before or after along the lines of:
“A conformance test verifies the expected functionality works as a user might encounter it in the wild, and tests should begin by covering the most important and visible aspects of the function.”
We don’t really define them in this doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added required text. Thanks @bgrant0607 @smarterclayton
1f63d12
to
40555ce
Compare
/assign @bgrant0607 |
@bgrant0607 Please review. |
understand what features are being tested without having to look through | ||
the testcase's code directly. | ||
|
||
## Adding New Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please include this process / policy as the first point.
Contributors must write and submit e2e tests first (approved by owning Sigs). Once the new tests prove to be stable in CI runs, later create a followup PR to add the test to conformance. This approach also decouples the production of useful tests from their promotion to conformance.
@mithrav to review this PR. |
/cc @mithrav |
@AishSundar: GitHub didn't allow me to request PR reviews from the following users: mithrav. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
- the testcase must include a comment immediately before the | ||
`framework.ConformanceIt()` call that includes all of the required | ||
metadata about the test (see the [Test Metadata](#test-metadata) section) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also include these guidelines for the PR to promote a test to Conformance
- Please use "Promote xxx e2e test to Conformance" as template of your PR title
- Tag your PR with "/area conformance" label
- Send your PR to Sig-Architecture for review by adding "@kubernetes/sig-architecture-pr-reviews"
- CC your Sig, Sig-Architecture and me in your PR
- GA (not alpha or beta APIs, nor deprecated features) | ||
- portable (not dependent on provider-specific capabilities or on the public internet) | ||
- not an optional feature (e.g. not policy enforcement) | ||
- non-privileged (neither root on nodes, network, nor cluster) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should work with default settings for all configuration parameters (example the default list of admission plugins should not have to be tweaked for passing conformance).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also tests that are flaky should not be added to the conformance suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. thanks @dims.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there is significant overlap between this doc and portions of e2e-tests.md:
- https://github.com/kubernetes/community/blob/master/contributors/devel/e2e-tests.md#conformance-tests
- https://github.com/kubernetes/community/blob/master/contributors/devel/e2e-tests.md#defining-conformance-subset
I would ask you move/merge that content here and link to this doc from there
- portable (not dependent on provider-specific capabilities or on the public internet) | ||
- not an optional feature (e.g. not policy enforcement) | ||
- non-privileged (neither root on nodes, network, nor cluster) | ||
- cannot be flaky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does flaky mean in this context? pass N runs in a row? N% passes over M runs? N% passes over M runs against a single commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rewording to not mention as flaky, please suggest?
I have moved the Conformance sections from e2e testing document into this document and referred this doc in there.
``` | ||
/* | ||
Release : v1.9 | ||
Testname: Kubelet: log output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am late to this party but some level of consistency of testname would be appreciated. https://github.com/cncf/k8s-conformance/blob/master/docs/KubeConformance-1.9.md is pretty difficult to scan, eg:
"ServiceAccounts should allow opting out of API token automount"
followed by
"configmap-in-env-field"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This inconsistency happened due to partial porting of the tests. The test names should be in English eventually that are customer/human friendly.
/* | ||
Release : v1.9 | ||
Testname: Kubelet: log output | ||
Description: By default the stdout and stderr from the process being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also late to the party, but how are we enforcing that descriptions are kept up to date with the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The process should have checks and bounds such that when a conformance test is changes the author as well as reviewer pays attention to change the description accordingly. Automatically detecting a change to conformance tests to label them for sig arch review is much harder may be?
> By default the stdout and stderr from the process being executed in a pod MUST be sent to the pod's logs. | ||
|
||
|
||
Conformance test results, by provider and releases, can be viewed in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go under a separate header, it doesn't have anything to do the Sample Conformance Test header above. Maybe contributing test results with an additional callout to CNCF certification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
contributors/devel/README.md
Outdated
@@ -43,6 +43,9 @@ Guide](http://kubernetes.io/docs/admin/). | |||
* **Document Conventions** ([how-to-doc.md](how-to-doc.md)) | |||
Document style advice for contributors. | |||
|
|||
* **Conformance Tests Guidelines** ([conformance-guidelines.md](conformance-guidelines.md)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue this doc shouldn't just cover conformance test documentation. We're also trying to describe what a conformance test is, what the process is to promote a test to conformance, maybe even how to write a good one vs. a bad one, etc. Maybe this should be retitled and moved up closer to the "Testing" doc above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks for the review, please suggest if need be reworded,etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also please add a small edit to the contributors/guide/README.md to change the Testing subsection's "...there are three types of tests..." to "...there are multiple types of tests..." and add a bullet for "* Conformance: "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording/heading nits, I was hoping for more than just copy-paste of the existing content. Address these and @tpepper's suggestion and I'm good to /lgtm /approve
go run hack/e2e.go -- --provider=skeleton --test --test_args="--ginkgo.focus=\[Serial\].*\[Conformance\]" | ||
``` | ||
|
||
### Defining Conformance Subset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping you would merge these into the bulleted list above, merging or removing dupes where appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried my best to add them and rearrange them under the bullet points above. My bad, the reason for a cut and paste was to not lose content, it kind of defined what a conformance test is not where as the bullet points mentioned necessary attribute of the test.
Fixed @tpepper recommendation.
Please review and let me know if any recording and rearrangement is required.
|
||
It is impossible to define the entire space of Conformance tests without knowing | ||
the future, so instead, we define the compliment of conformance tests, below | ||
(`Please update this with companion PRs as necessary`): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence can go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
A conformance test verifies the expected functionality works as a user might encounter it in the wild, | ||
and tests should begin by covering the most important and visible aspects of the function. | ||
|
||
Conformance tests in the default profile should cover features that are: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe drop this bulleted list under a heading like "Conformance Test Requirements"
I want to be able to say something like "a test is eligible for promotion to conformance if it meets these requirements [link to bulleted list]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
This document will help people understand what features are being tested without having to look through | ||
the testcase's code directly. | ||
|
||
As each new release of Kubernetes provides new functionality, the subset of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put this under the heading "Conformance Test Version Skew Policy"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this section up after 'Conformance Test Requirements'
- A v1.1 cluster should pass v1.0, v1.1 conformance tests, and fail v1.2 | ||
conformance tests | ||
|
||
Conformance tests are designed to be run with no cloud provider configured. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this under the heading "Running Conformance Tests"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the contributor guide!
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spiffxp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm looks like a good baseline! Ty for all the iterations on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving approved review just because the little red x annoys me, I guess /lgtm or /approve doesn't necessarily alter my previous github review status
/hold cancel |
that corresponds to that version. Thus `v1.2` conformance tests would be run | ||
from the head of the `release-1.2` branch. eg: | ||
|
||
- A v1.3 development cluster should pass v1.1, v1.2 conformance tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "1.3 development cluster" mean? A cluster built from the master branch after the 1.2 release was cut but before the 1.3 release was cut?
Note that kubectl officially only supports one release of skew.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
where are the docs on kubectl skew? I wasn't aware it was that limited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the most obvious location, I know, but the origin of this doc predates kubernetes.io:
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/release/versioning.md#supported-releases-and-component-skew
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I say the same thing whenever I have to refer to that uri.
For example, a v1.3 master should work with v1.1, v1.2, and v1.3 nodes, and should work with v1.2, v1.3, and v1.4 clients.
These bullet points (written before docs were imported into k/community) were based on the "nodes" statement. Should we cut this back to just one release of skew?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will have a PR with this in it later today, trying to tighten up wording in a few other places as well
This PR create a Conformance Document that provides a general guidelines when documenting a test as part of conformance test suite.