WIP: features: Add a TestOCAdminUpgradeTechPreview feature gate #2337
Conversation
This feature gate is only tied to the CI suite (hence the "Test..." prefix), and enables the 'tests' image to exercise tech-preview 'oc adm upgrade ...' command-line functionality when run against this cluster. This is different from many feature gates, where the feature gate covers both "implement the functionality" (e.g. API properties in CustomResourceDefinitions or in-cluster controller behavior" and "run the tests that exercise that functionality". Because the functionality is in the command line, we can exercise tech-preview command-line behavior even against default feature set clusters, as we point out in docs [1]: The oc adm upgrade recommend command is a Technology Preview feature only... Your cluster does not need to be a Technology Preview-enabled cluster in order for you to use the oc adm upgrade recommend command. By including the new feature gate in the default feature set, default-feature-set CI runs will exercise the tech-preview functionality, and confirm that the documented "works vs. default clusters" claim remains accurate. There's a chance that instability in the tech-preview 'oc' functionality destabilizes CI results for those default feature-set clusters, but we can mitigate that risk by running more tests before merging changes, and backstop with very-easy-to-approve reverts and removals of any unstable tests if necessary to recover the default-CI job signal. An alternative approach would be to define a new CI-job flavor between the current "tech-preview features against tech-preview clusters" and "GA features against GA clusters" to run "tech-preview features against GA clusters". But I currently expect these tech-preview command-line features to be rare and stable enough for it to not be worth the investment of building out a new job flavor. We can revisit that approach if it does turn out to cause problems for default-CI job signal. I've gone with the generic TestOCAdminUpgradeTechPreview name because there are currently several tech-preview subcommands under 'oc adm upgrade ...', including OC_ENABLE_CMD_UPGRADE_STATUS, OC_ENABLE_CMD_UPGRADE_ROLLBACK, and OC_ENABLE_CMD_UPGRADE_RECOMMEND. If API approvers accept the idea that tech-preview 'oc adm upgrade ...' functionality should be tested against default-feature-set clusters, then it seems like they'd rather not bother reviewing each tech-preview idea and agreeing that it to should be tested against default-feature-set clusters. But if the API approvers do want to review this decision on a feature-by-feature basis, I could pivot to a more specific name. [1]: https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/updating_clusters/performing-a-cluster-update#update-upgrading-oc-adm-upgrade-recommend_updating-cluster-cli
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @wking! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XS
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wking 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 |
|
@wking: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| reportProblemsToJiraComponent("oc / update"). | ||
| contactPerson("wking"). | ||
| productScope(ocpSpecific). | ||
| enhancementPR(legacyFeatureGateWithoutEnhancement). |
There was a problem hiding this comment.
verify is mad about the lack of enhancement, but I'm going to leave it alone for now, while I wait for API reviewers to decide how they feel about the general idea of a feature-gate to cover origin test-case enablement.
The CI suite should be able to run tests without a gate existing in o/api, did you try this? At the moment, the code doesn't behave as David expected, but I'm discussing this with the TRT folks this afternoon and this PR should resolve the issue of "unknown gates tests should be running". Which I think should mean that you don't need a gate. Also, last I checked Sippy was just looking at the test output and isn't importing o/api to determine which gates do/do not exist, so as long as your JUnit is configured correctly, sippy should filter on the gate name as expected
Why is testing against tech preview clusters insufficient? Our requirement generally for promoting features to the default set is based on their ability to perform against tech preview clusters. If they work there, we assume that they also work on default. So this argument isn't congruent with our normal flow 🤔 |
This feature gate is only tied to the CI suite (hence the
Test...prefix), and enables thetestsimage to exercise tech-preview 'oc adm upgrade ...' command-line functionality when run against this cluster. This is different from many feature gates, where the feature gate covers both "implement the functionality" (e.g. API properties in CustomResourceDefinitions or in-cluster controller behavior" and "run the tests that exercise that functionality". Because the functionality is in the command line, we can exercise tech-preview command-line behavior even against default feature set clusters, as we point out in docs:By including the new feature gate in the default feature set, default-feature-set CI runs will exercise the tech-preview functionality, and confirm that the documented "works vs. default clusters" claim remains accurate.
There's a chance that instability in the tech-preview
ocfunctionality destabilizes CI results for those default feature-set clusters, but we can mitigate that risk by running more tests before merging changes, and backstop with very-easy-to-approve reverts and removals of any unstable tests if necessary to recover the default-CI job signal.An alternative approach would be to define a new CI-job flavor between the current "tech-preview features against tech-preview clusters" and "GA features against GA clusters" to run "tech-preview features against GA clusters". But I currently expect these tech-preview command-line features to be rare and stable enough for it to not be worth the investment of building out a new job flavor. We can revisit that approach if it does turn out to cause problems for default-CI job signal.
I've gone with the generic
TestOCAdminUpgradeTechPreviewname because there are currently several tech-preview subcommands underoc adm upgrade ..., includingOC_ENABLE_CMD_UPGRADE_STATUS,OC_ENABLE_CMD_UPGRADE_ROLLBACK, andOC_ENABLE_CMD_UPGRADE_RECOMMEND. If API approvers accept the idea that tech-preview 'oc adm upgrade ...' functionality should be tested against default-feature-set clusters, then it seems like they'd rather not bother reviewing each tech-preview idea and agreeing that it to should be tested against default-feature-set clusters. But if the API approvers do want to review this decision on a feature-by-feature basis, I could pivot to a more specific name.