Clarifies some of the instructions #24

timothysc · 2017-10-24T15:27:27Z

Updates the instructions to be more clear and to remove some of the ambiguity that folks have had.

Fixes #17
Fixes #6

/cc @mml @smarterclayton @aaronlevy @WilliamDenniss

dankohn · 2017-10-24T15:30:35Z

issues.md

-
-* https://github.com/kubernetes/kubernetes/issues/52822  
-* https://github.com/kubernetes/kubernetes/pull/52823  
+## At this time all known major issues have been fixed in the 1.8 e2e test suite.


I'm glad that these are all fixed by I think it's worthwhile to keep a historical record, at least for a little while, to demonstrate that SIG-architecture has been working with program participants to deal with their issues.

I'll update to denote [FiV-1.8].

@dankohn In thinking about this it makes more sense to me to have a single tracking issue in this repo that other issues can reference. Otherwise this file will require constant maintenance.

e.g. - #27

Let me know what you think and I'll fix up this PR to point to that tracking issue.

dankohn · 2017-10-24T15:31:55Z

Good start, but could you please update https://github.com/cncf/k8s-conformance/blob/master/reviewing.md as well to clarify the version requirements. That's the outstanding issue on #11 (comment)

timothysc · 2017-10-24T18:45:29Z

Good start, but could you please update https://github.com/cncf/k8s-conformance/blob/master/reviewing.md as well to clarify the version requirements. That's the outstanding issue on #11 (comment)

Yup, I'll update this PR soon-ish.

mml · 2017-10-24T20:48:33Z

instructions.md

-[sonobuoy](https://github.com/heptio/sonobuoy), and the standard way to run
-these in your cluster is with `curl -L https://raw.githubusercontent.com/cncf/k8s-conformance/master/sonobuoy-conformance-1.7.yaml | kubectl apply -f -`.
+[sonobuoy](https://github.com/heptio/sonobuoy/tree/latest), and the standard way to run
+these in your cluster is with `curl -L https://raw.githubusercontent.com/heptio/sonobuoy/latest/examples/conformance.yaml | kubectl apply -f -`.


Would prefer we kept this yaml file in this repo.

I'd prefer to point to the latest/example which will be kept up to date. The reason for the change was confusion that arose from #17 and #6 .

I don't understand why it's easier to keep the one in the sonobuoy repo up to date than this one. I agree that the config should be kept up to date, but I don't see why that means it can't be in this repo.

I think that our instructions should be under a cncf repo, and the sonobuoy config is part of those instructions. I would prefer that revisions to the YAML config undergo review here.

For example, the linked file includes a long list of resources and the systemd_logs plugin, which users don't need to run in order to generate conformance results.

I don't understand why it's easier to keep the one in the sonobuoy repo up to date than this one.

b/c it's the canonical source and actively maintained and kept up to date, I'd also be happy to trim the conformance one and call it cncf-conformance. If we keep it here, that means someone should maintain it and actively update and verify it. The reason for this PR is because there were two issues filed against the version in the repo and no answers were given.

timothysc · 2017-10-25T22:00:43Z

@dankohn minor update PTAL.

dankohn · 2017-10-27T06:00:55Z

LGTM

timothysc · 2017-10-27T15:08:59Z

@WilliamDenniss @mml PTAL - I've updated to address the comments.

mml · 2017-10-27T18:45:07Z

@timothysc Thanks. Appreciate keeping this config in the CNCF repo.

My only request is that we leave behind the v1.7 YAML file with v1.7 in its name and add the new one with the name v1.8. People running conformance tests should be expected to know which version they are trying to certify, and I think just going to "latest" is not likely to yield expected (or reproducible, after a certain while) results.

If we really want a "latest" version, how about a symlink to the 1.8 one?

mml · 2017-10-27T18:42:38Z

reviewing.md

@@ -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` version is no more than one minor version greater than the `server` version 


The guidance here is to make sure that "client" matches the directory they are putting their results into. So if they are certifying as being "kubernetes 1.7 conformant", they need to be running e2e tests built from 1.7.

Sorry I mis-read this now when I re-read it.. I'll swap back.. but the other comment below applies.

timothysc · 2017-10-27T19:31:25Z

@mml There were issues found with the tests in 1.7 that have been fixed in 1.8 and validated that they work properly in 1.7. e.g. 1.8 tests are in some ways more complete and correct for validating a 1.7 cluster than the 1.7 tests. I don't think they need to match given how upstream patching process works, which is why the instructions have changed.

…t are tracking. Signed-off-by: Timothy St. Clair <timothysc@gmail.com>

mml · 2017-10-27T20:12:10Z

@timothysc see #6 (comment) In general, we are not maintaining k8s with the property that the conformance tests in N.{M+1} cover everything in N.M. On the contrary, I expect turned down APIs, sooner or later, to simply disappear from the e2e suite.

mml · 2017-10-27T20:18:48Z

Also, regardless of the "one minor version" thing, the key check is in the match between the client version and the subdirectory. E.g., you can't submit results into the v1.8 directory using v1.6 client, even if your server is also v1.6.

timothysc · 2017-10-27T20:21:43Z

The statement should read: v1.8 tests are ok on a v1.7 cluster to determine conformance b/c I've been actively making certain they do and ensuring that proper version gates are in place on any api additions or changes. If anything, new tests are added to gain wider coverage, and other tests have been fixed.

mml · 2017-10-27T20:37:10Z

@timothysc I am happy to pursue the "N+1" argument separately. I think @bgrant0607 has some thoughts on skew testing and whether vM.{N+1} tests should be considered authoritative for vM.N.

So I am fine leaving in the N+1 guidance for now, but the reviewer needs to compare the client version (i.e. the test version) with the subdirectory (i.e. the version of kubernetes the PR author is claiming conformance with). The server version does not matter.

timothysc · 2017-10-27T20:54:28Z

It's part of an ongoing effort of this body, along with sig-testing, to ensure the consistency and track the issues of the e2e "Conformance" tests. I have little doubt that there may be skew over time that may force us to maintain multiple submission files. However, in this particular case, there are number of known defects that exist in the 1.7 tests that have been fixed in 1.8 in an effort to get this process started.

#27 contains a sub-set of some of the issues, and not all have been cross-linked. Going forwards, we will likely have to revision our submission files, but I would prefer not to link the 1.7 tests b/c of the known problems that have been fixed.

mjura

I've tested this change, works fine for me

timothysc · 2017-10-30T14:00:48Z

@dankohn - PTAL. I think all immediate concerns have been addressed.

dankohn · 2017-10-30T15:11:49Z

@timothysc What you have is good but I was hoping you would also update the reviewer instructions to address the question on #11

Or, does OpenShift need to re-run the conformance tests with the updated YAML?

cc @smarterclayton

mml · 2017-10-30T20:08:13Z

LGTM

Clarifies some of the instructions

dankohn reviewed Oct 24, 2017

View reviewed changes

mml suggested changes Oct 24, 2017

View reviewed changes

timothysc force-pushed the clearup branch from 810eab6 to 1a75ab5 Compare October 25, 2017 22:00

This was referenced Oct 27, 2017

conformance results for v1.7/hauwei. huawei-cloudnative/k8s-conformance#1

Closed

Conformance results for v1.7/hauwei. huawei-cloudnative/k8s-conformance#2

Closed

timothysc force-pushed the clearup branch from 1a75ab5 to bb9f069 Compare October 27, 2017 15:07

mml suggested changes Oct 27, 2017

View reviewed changes

Clarifies some of the instructions and properly links to versions tha…

34fcccb

…t are tracking. Signed-off-by: Timothy St. Clair <timothysc@gmail.com>

timothysc force-pushed the clearup branch from bb9f069 to 34fcccb Compare October 27, 2017 20:10

mjura approved these changes Oct 30, 2017

View reviewed changes

mjura mentioned this pull request Oct 30, 2017

Conformance results for v1.7/caasp (SUSE) #32

Merged

WilliamDenniss merged commit dc1e1e9 into cncf:master Oct 30, 2017

timothysc mentioned this pull request Oct 31, 2017

Client version matching server version doesn't seem like it captures conformance appropriately #6

Closed

BobyMCbobs pushed a commit that referenced this pull request Oct 23, 2023

Merge pull request #24 from timothysc/clearup

bae032b

Clarifies some of the instructions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarifies some of the instructions #24

Clarifies some of the instructions #24

timothysc commented Oct 24, 2017

dankohn Oct 24, 2017

timothysc Oct 24, 2017

timothysc Oct 25, 2017

timothysc Oct 25, 2017

dankohn Oct 25, 2017

dankohn commented Oct 24, 2017

timothysc commented Oct 24, 2017

mml Oct 24, 2017

timothysc Oct 25, 2017

mml Oct 25, 2017

timothysc Oct 25, 2017

timothysc commented Oct 25, 2017

dankohn commented Oct 27, 2017

timothysc commented Oct 27, 2017

mml commented Oct 27, 2017

mml Oct 27, 2017

timothysc Oct 27, 2017

timothysc commented Oct 27, 2017 •

edited

Loading

mml commented Oct 27, 2017

mml commented Oct 27, 2017

timothysc commented Oct 27, 2017

mml commented Oct 27, 2017

timothysc commented Oct 27, 2017

mjura left a comment

timothysc commented Oct 30, 2017

dankohn commented Oct 30, 2017

mml commented Oct 30, 2017

Clarifies some of the instructions #24

Clarifies some of the instructions #24

Conversation

timothysc commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dankohn commented Oct 24, 2017

timothysc commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc commented Oct 25, 2017

dankohn commented Oct 27, 2017

timothysc commented Oct 27, 2017

mml commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc commented Oct 27, 2017 • edited Loading

mml commented Oct 27, 2017

mml commented Oct 27, 2017

timothysc commented Oct 27, 2017

mml commented Oct 27, 2017

timothysc commented Oct 27, 2017

mjura left a comment

Choose a reason for hiding this comment

timothysc commented Oct 30, 2017

dankohn commented Oct 30, 2017

mml commented Oct 30, 2017

timothysc commented Oct 27, 2017 •

edited

Loading