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 certified conformance mode and non-disruptive-conformance mode #885

Merged
merged 1 commit into from
Sep 17, 2019
Merged

Add certified conformance mode and non-disruptive-conformance mode #885

merged 1 commit into from
Sep 17, 2019

Conversation

johnSchnake
Copy link
Contributor

What this PR does / why we need it:
With K8s v1.16, disruptive tests may not be part of the conformance
suite. As a result, we need to ensure that user workloads are
protected by default while still allowing for CNCF certification
test runs.

In this PR we:

  • rename the default conformance mode to be non-disruptive-conformance
    and modified the skip list appropriately.
  • added a certified-conformance mode which does not provide a skip list
    and will run even the disruptive tests
  • tweaked the naming to use lower case values by default to be more
    consistent with our other flags/naming

Which issue(s) this PR fixes
Fixes #877
Fixes #875

Special notes for your reviewer:
I could have written the skip list in a slightly different way, naming the tests individually, but instead chose a higher level term that filtered both and, IMO, seems more clear why they may be skipped.

However, we need to talk about this issue in general because we are actually in a situation now where the E2E_SKIP value is dependent on the version of the cluster :( I think we can get away with this because the only real risks are:

  • someone uses this version of Sonobuoy with an older cluster
    • the non-disruptive-conformance run should hit all the conformance tests anyways since the NoExecuteTaintManager tests were not part of conformance on older clusters
  • someone uses this version of Sonobuoy with a newer cluster
    • the non-disruptive-conformance run may skip the NoExecuteTaintManager tests, even if the tests get updated somehow to not be disruptive or other NoExecuteTaintManager tests which are non-disruptive get added to Conformance. The only downside here is that a few tests don't get run, but since these are non-certification runs it doesn't matter much.

If there is a situation I've failed to think of, they can still set the e2e-focus/e2e-skip values on the CLI or the env vars in the YAML directly so we are not a blocker.

Release note:

Default `sonobuoy run` commands will run a subset of the `Conformance` tests which are safe to run on clusters with production workloads (avoids running known disruptive tests). To generate results for CNCF certification, you must run `sonobuoy run --mode=certified-conformance`

@johnSchnake johnSchnake requested a review from zubron September 17, 2019 15:10
@codecov-io
Copy link

codecov-io commented Sep 17, 2019

Codecov Report

Merging #885 into master will increase coverage by 0.05%.
The diff coverage is 11.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
+ Coverage   47.34%   47.39%   +0.05%     
==========================================
  Files          75       75              
  Lines        5025     5034       +9     
==========================================
+ Hits         2379     2386       +7     
- Misses       2508     2512       +4     
+ Partials      138      136       -2
Impacted Files Coverage Δ
pkg/client/mode.go 0% <0%> (ø) ⬆️
pkg/client/defaults.go 0% <0%> (ø) ⬆️
cmd/sonobuoy/app/args.go 89.69% <100%> (ø) ⬆️
cmd/sonobuoy/app/status.go 59.43% <0%> (+1.88%) ⬆️
pkg/plugin/aggregation/aggregator.go 75% <0%> (+3.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb43587...e29de35. Read the comment docs.

Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

I agree with the approach you've taken here 👍

As we chatted about earlier, I think the only case where users might not be protected is if they are using an older version of Sonobuoy with a newer cluster/conformance image but as you said, this would most likely be a testing scenario and the risk of this having significant impact would be low.

As we've already discussed, we should communicate this to users, encourage everyone to upgrade and explain the new conformance steps. Other than that I think what you've added is the best approach we can take. Thanks!

pkg/client/mode.go Outdated Show resolved Hide resolved
With K8s v1.16, disruptive tests may not be part of the conformance
suite. As a result we need to ensure that user workloads are
protected by default while still allowing for CNCF certification
test runs.

In this PR we:
 - rename the default `conformance` mode to be `non-disruptive-conformance`
and modified the skip list appropriately.
 - added a `certified-conformance` mode which does not provide a skip list
and will run even the disruptive tests
 - tweaked the naming to use lower case values by default to be more
consistent with our other flags/naming

Fixes #877
Fixes #875

Signed-off-by: John Schnake <[email protected]>
@johnSchnake johnSchnake merged commit 90a586f into vmware-tanzu:master Sep 17, 2019
@johnSchnake johnSchnake deleted the certificationMode branch September 17, 2019 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants