always enable ClusterTrusbBundles feature gates in tests that both focus it and enable all alpha APIs #34451
Conversation
|
@stlaz: GitHub didn't allow me to request PR reviews from the following users: liggit. 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-sigs/prow repository. |
k8s-ci-robot
added
cncf-cla: yes
The alpha-features test enables all the APIs but as ClusterTrustBundles move to beta, we explicitly need to enable these FGs.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stlaz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
area/release-eng
stlaz
changed the title
stlaz
changed the title
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @aojea |
|
/assign @dims |
|
I haven't sent that email which is in that doc because I was hoping to get feedback from @BenTheElder first. The underlying idea isn't new. We just need to continue with converting jobs. |
|
Thanks for weighing in. I agree on that direction and will be happy to ack splitting / adjusting these jobs in the future. Would you agree that until we get there, this PR is a reasonable step to unblock the promotion of the existing focus tests? |
Huh? Beta features should not be covered by alpha jobs.
Agreed. If you need a CI job to cover tests for these, it should be stood up. The shared alpha features job (and beta and ..) just ensures that we haven't totally broken the cluster when these are on, it may or may not provide coverage and we generally expect features with special requirements to stand up their own CI. |
|
I see the failures in the jobs from the linked PR: We should probably not be running |
|
For comparison: the kind alpha jobs will not run [Feature] tag |
If we want to block this on moving that direction, ok... just wanting to understand what we're telling feature authors to do for their tests. Will we ack standup of a new e2e job for every distinct feature? Can we actually afford that? |
Ok, we can stage that change as a first PR to master. Do we already have jobs that will work like we want in place:
If so, I think that would work pretty seamlessly for any features that didn't require special setup beyond API and gate enablement |
Er, with the rest of the context though, you generally don't need one, because it's only gated by the gate, and not on some additional setup (which would be an arbitrarily named cc @pohly @aojea, we should finish the transition to labels instead of regex and make sure we opt in/out of the alpha label appropriately. |
I'm talking to aojea about that more when he's back (he's out today), I think the answer is "it's a mess but that's roughly what we should have"
Right, I'm also reviewing the doc linked above. We don't want SIGs to have to setup jobs for every feature, just the ones that require special setup, though we do expect feature developers to ensure that coverage is happening (instead of just assuming someone else checked this for them) even though we do plan to work on improving the state of alpha/beta jobs. The responsibility for ensuring that tests for your features run needs to be shared I think, but that doesn't mean we should spin up a job for every feature, just taking the time to check that it is and work with us if it's not (like here!) That test case doesn't seem to be feature-gate tagged currently, let's fix that, and look at correcting the jobs to filter on the alpha/beta labels correctly. |
ok, opened kubernetes/kubernetes#130567 which I think does what is expected That doc also says "Note that at the moment, not all jobs filter out tests with Alpha or Beta requirements like that. Therefore all tests with such a requirement also have to be annotated with a [Feature] tag. This restriction will be lifted once migration of jobs to --filter-label is completed." ... so... I'm not sure if kubernetes/kubernetes#130567 will break or be blocked by e2e job adjustments. I'm very sensitive to the timing of this and the runway to 1.33 code freeze... I want to move test config in the right direction but I can't afford to block folks on this either. If we can resolve this the "right" way ~today, let's do that, and if not, can we proceed with this PR? |
|
looks like required presubmit https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/130567/pull-kubernetes-e2e-kind-ipv6/1896997483410624512 needs to exclude alpha e2es |
Which I have been trying to push forward, with mixed results, mostly because I can't do it alone. The email I mentioned is a more public call to action for an issue that will soon be one year old.
We can't go that fast. Defining a test with only For now, adding That PR shows what we want at the end and can't have right now, as the failures in https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/130567/pull-kubernetes-e2e-gce/1896997483054108672 show. |
Are you suggesting they edit the alpha and beta jobs definitions to enable and focus on tests for features in alpha and beta, or that they spin up dedicated jobs? |
We should either:
I'm not suggesting that the few of us here (or you specifically) should do this for all of CI, but we can do it for release / PR blocking and rip the bandaid off for the rest after PSA-ing the org. (This is also why I think we should do smaller, targeted PSAs, since this one would be [Action Required]) |
... I actually thought we'd planned something like this with the label transition already, but I see we only do like I actually think we could bulk update jobs that are not obviously testing alpha/beta features to exclude |
Something like that seems plausible to me for features which don't have any special requirements other than "enable alpha apis and gates". Could the same thing work for beta? I tweaked kubernetes/kubernetes#130567 in that direction and opened a POC stacked follow-up that promotes to beta at kubernetes/kubernetes#130572 If we can edit job definitions of the generic alpha + beta jobs to get them green on both of those PRs without dropping test coverage, I'd suggest we merge those changes |
| @@ -907,7 +907,7 @@ presubmits: | |||
| kubetest2 ec2 \ | |||
| --stage https://dl.k8s.io/ci/fast/ \ | |||
| --version $VERSION \ | |||
| --feature-gates="AllAlpha=true,EventedPLEG=false,StorageVersionAPI=true,APIServerIdentity=true" \ | |||
| --feature-gates="AllAlpha=true,ClusterTrustBundle=true,ClusterTrustBundleProjection=true,EventedPLEG=false,StorageVersionAPI=true,APIServerIdentity=true" \ | |||
There was a problem hiding this comment.
Can we instead set skip-regex to have [Beta] as a comparable stopgap, combined with kubernetes/kubernetes#130567 ?
I don't think the intent should be to run these tests in the alpha job as a beta feature
There was a problem hiding this comment.
not sure about the [Alpha] or [Beta] tagging since that will change selection of existing tests using featuregates in ways I'm unsure of
Using generic "Feature:Alpha" and "Feature:Beta" is net-new more incremental step I could reason about better ... I opened https://github.com/kubernetes/test-infra/pull/34456/files to add those focus / skips to the generic jobs I could find that were currently explicitly referencing the ClusterTrustBundle feature as a way to straddle onto making kubernetes/kubernetes#130572 and kubernetes/kubernetes#130567 work without dropping test coverage
There was a problem hiding this comment.
use a standardized [Feature:Alpha] automatically as a bridge / stop-gap
We have that, just not for focus/skip. It was meant to be used with --label-filter. kubernetes/kubernetes#130580 (opened today as alternative to kubernetes/kubernetes#130567) makes it available also to jobs using focus/filter.
just finish switching to label filter for critical shared jobs and PSA the org about the action required to update any other jobs (and if nobody response then ... those jobs must not be maintained)
I'm arriving at a similar conclusion. I was even considering to convert critical jobs to --label-filter and then label feature-gate-only tests so that they run in unconverted jobs. If they then fail, job owners either (finally) take notice or they don't and we can consider deleting the jobs. But with kubernetes/kubernetes#130580 such unconverted jobs wouldn't break like that - or rather, not break at all.
This may be good (no breakage) and bad (they don't get updated and we continue to depend on regexp in those jobs).
|
/assign |
SergeyKanzhelev
moved this from Triage
to PRs - Needs Reviewer
in SIG Node CI/Test Board
SergeyKanzhelev
moved this from PRs - Needs Reviewer
to PRs - Needs Approver
in SIG Node CI/Test Board
|
/close |
|
@stlaz: Closed this PR. 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-sigs/prow repository. |
github-project-automation
bot
moved this from PRs - Needs Approver
to Done
in SIG Node CI/Test Board
The alpha-features test enables all the APIs but as ClusterTrustBundles move to beta, we explicitly need to enable these FGs.
fixes tests in kubernetes/kubernetes#128499
The PR also explicitly adds CTB featuregates to jobs that focus the CTB tests and enable all alpha APIs.
/cc @enj @liggitt