KEP-5468: Invariant Testing

[BenTheElder]

• One-line PR description: Introduce invariant testing KEP.

• Issue link: Invariant Testing #5468

• Other comments: This process has a truly annoying amount of redundancy and boilerplate. We really need to think about how we can streamline things a bit. I spent far more time on this KEP than finding an agreeable solution to the problem and implementing it.

/cc @pohly @aojea @richabanker

[BenTheElder]

will update this pending above discussion about the alpha/beta/stable lifecycle not really making sense for this KEP https://github.com/kubernetes/enhancements/pull/5514/files#r2323831978

[BenTheElder]

maybe @jpbetz ?

richa per #5468 (comment)

patrick and antonio as fellow SIG Testing TLs

[BenTheElder]

There is a draft of the implementation in kubernetes/kubernetes#133394, though I intend to capture all necessary details here.

Mostly here we want to formalize what sort of "run after all other tests" is permitted and how we're approaching that.

[pohly]

/assign

[aojea]

LGTM

there are some open comments, but we have been discussing this for a long time and I think describes pretty well the overall idea, of course when moving to execution we may find some unexpected issues that we need to reconsider, but I think is important to get this early in the cycle to get feedback before the large features start to get merged and avoid to get perfection in the KEP

[richabanker]

/lgtm
from instrumentation side

[richabanker]

/hold

for other comments to be resolved

[BenTheElder]

This is failing because we don't have the PRR approver metadata yet.

Will reach out when we sort api-machinery, I posted in their slack last week, reached out to a lead today.

[pohly]

Looks good overall, some nits and suggestions.

[pohly]

Nit: you could make the README.md easier to review if you dropped comments which are no longer needed.

[BenTheElder]

/shrug, this is an artifact of the KEP process, this KEP is not complete.
that these instructions are super verbose .... is another matter

[pohly]

You can keep those which are still relevant. But below you have filled out entire sections with "not applicable" while keeping the comments. Those comments could be removed because the section is complete.

As it stands now, the README.md is mostly obsolete comments with very little actual content.

[pohly]

Suggested change

	> Owners for this metric: <github-handles>
	> Owners for this metric: <github-handles and/or SIG>

[BenTheElder]

I think we really need a list of specific owners of the feature, assigning SIGs to resolve bugs does not work for most SIGs. Maybe a few SIGs are extremely well staffed, but in general we need to require that a list of multiple owners is responsible for a feature.

[BenTheElder]

cc @aojea

[pohly]

I'm worried that we'll end up with situations where individual owners have moved on. Whom should a problem be escalated to when that happens?

Listing the SIG would also be useful for /sig ... in the issue that we ask contributors to file.

Let's make that <github-handles and SIG(s)>?

[BenTheElder]

That makes sense, I just think they need to not be interchangeable and that a list of owners should be required. But also documenting the SIG makes sense.

Especially when this is first introduced for a feature there should be some specific owners with context that can help ensure there are not issues. Similar to the underlying metrics (which currently list a single owner and SIG).

I also think we should consider as we approve these requiring some minimum number of owners, but I don't know what to set it to and I do want to make sure we start testing out the initial invariant early in the cycle, 1.35 will be a short cycle and we also may risk holding up declarative validation if we wind up having to revert this test.

I have pretty high confidence in the current implementation but ... unknown unknowns until we actually start soaking it.

[pohly]

So do you want to add the "and SIG" or "and SIG(s)". If not, then I can LGTM.

[pohly]

Never mind, it's not ready for other reasons (PRR), as you noted in #5514 (comment).

[BenTheElder]

I added the SIG (though we may still revisit the exact formatting/language), and I took a stab at PRR by way of just marking it direct to stable ...

[BenTheElder]

Last time I did one of these "non-production" KEPs the process was a bit different ...

[BenTheElder]

/cc @jpbetz

[vrutkovs]

ReportAfterSuite seems to be used mostly for custom reports, so it's not designed to run more tests.

In OpenShift we use two test annotations: [Early] and [Late] and split tests in the suite, running them in necessary order. Have this alternative been considered?

[BenTheElder]

ReportAfterSuite seems to be used mostly for custom reports, so it's not designed to run more tests.

Yeah, we're not running actual tests here though, we're only fetching metrics (which we already have an option to do after the test suite) and validating some of them, if not for the metric fetch part, it fits as a custom report. The metrics fetch part is cheap and quick anyhow so 🤷‍♂️

We explicitly do not wish to permit anything more expensive than this, or any mutable actions, or any intra-test ordering, only checking cluster status after-suite and reporting if it's invalid.

IMHO as a general rule, tests that need to be ordered are a huge anti-pattern especially in shared cluster e2e environment, and between the SIG Testing leads we've agreed to not permit this.

We actually considered doing this entirely outside of ginkgo just as part of TestMain() and having an env opt in/out, but leveraging the test selection logic to decide if it should run simplifies getting it enabled in the right jobs.

In OpenShift we use two test annotations: [Early] and [Late] and split tests in the suite, running them in necessary order. Have this alternative been considered?

This is an interesting option, but it would be more involved for our case, since we are using ginkgo's stock runner / scheduling and otherwise have no reason to rewrite this, we plan to keep randomized order for all tests other than this special invariant check.

I think it would make sense if we planned to have more tests with special ordering, but we don't.

[aojea]

I really prefer to avoid to customize ginkgo, since that will add more surface to cover and to maintain, and we got a lot of troubles because of that when we had to migrate from v1 to v2 ... this is pretty simple and decoupled

[vrutkovs]

The metrics fetch part is cheap and quick anyhow so 🤷‍♂️

I wonder if there is a way to limit the code to stay cheap and quick?

we plan to keep randomized order for all tests other than this special invariant check

Sure, the test list can be additionally shuffled to ensure tests don't depend on special order. I acknowledge it may be using advanced ginkgo features, but it doesn't seem like a significant ginkgo modification to me.

In any case, this idea is worth adding to the alternative list imo

[BenTheElder]

I think this is more of an implementation detail than an alternative to the overall KEP, the alternatives section of a KEP is meant to cover approaches versus more detailed implementation details. See the listed alternatives we have currently. (e.g., instead of having a test case we could collect this metric and monitor it out of band)

[BenTheElder]

I do agree this is worth considering.

[aojea]

/lgtm
/approve

I think we have an overall agreement on how to proceed, and the proposed implementation is encapsulated enough that we can always revisit without causing much problems, but at one point we need to execute and start to find the unknown unknowns ... 🤞

[pohly]

/lgtm
/approve

[jpbetz]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, BenTheElder, jpbetz, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS [jpbetz]
• keps/sig-testing/OWNERS [BenTheElder,aojea,pohly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[BenTheElder]

/hold-cancel
thanks all!

[BenTheElder]

/hold cancel
...