-
Notifications
You must be signed in to change notification settings - Fork 420
OTA-1581: Promote oc adm upgrade status to general availability
#2063
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
OTA-1581: Promote oc adm upgrade status to general availability
#2063
Conversation
|
@petr-muller: This pull request references OTA-1581 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@petr-muller: This pull request references OTA-1581 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/hold |
|
@petr-muller: This pull request references OTA-1581 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/uncc @deads2k @ingvagabund |
This is a request for an [exception](https://github.com/openshift/enhancements/blob/master/dev-guide/featuresets.md#what-if-i-dont-have-automated-testing-reporting-into-component-readiness) in the feature promotion process. Proposed promotion process: 1. Promote `UpgradeStatus` gate here. This does not actually put any functional change into customers' hands. 2. Promotion will allow tests tagged with `UpgradeStatus` to run in Default update jobs 3. Update-time tests will execute and feed results into Sippy feature view 4. The feature view will need to be monitored manually to meet the criteria otherwise enforced by `verify-feature-promotion` 5. Once that happens, _actually_ promote the feature through openshift/oc#2063 Why is exception needed: The feature promoted under `UpgradeStatus` gate identifier is actually entirely client-side implementation of the `oc adm upgrade status` command. Promoting the feature in o/api has no effect on any functionality in the cluster; nothing reads and depends on this identifier in the cluster (as of Jul 23, with the exception of the about to be removed behavior, tracked in OTA-1578 / openshift/cluster-version-operator#1216 expected to merge soon). The actual "promotion" of the feature would be done in the o/oc repository with a PR such as openshift/oc#2063 (intentionally `/hold` for now because of its "feature promotion" semantics). We wanted to utilize the `UpgradeStatus` gate to pursue the data-driven promotion process, but the process does not seem to accommodate features tied to OCP updates. Features are expected to accumulate test coverage in TechPreview (TP) jobs, but there are no TP jobs that exercise a cluster update. Minor version update TP cannot exist by definition (`TechPreviewNoUpgrade`). Patch version update jobs can exist in theory, but is likely that OCP cannot realistically maintain such jobs because teams are allowed to make update-breaking changes in TP (such as promotion where a v1alpha1 API will be replaced with a v1 one) until the version is GA. That means update-related features cannot easily accumulate sufficient test data in available TechPreview jobs (we may need to address this gap in the future because the exception process proposed here will not succeed for cluster-side behavior). For this feature specifically, running in TechPreview jobs would actually increase risk of problems after promotion. The conditional behavior is entirely client-side; testing it against TP clusters carries a risk of inadvertedly depending on some cluster-side TP-only feature. Such feature could be missing in Default clusters and the client-side behavior can break when run against a Default cluster. For CLI-side features, it seems more appropriate to test TP behavior against a Default cluster instead. This is possible, but non-TP jobs will not execute tests tagged with a TP feature gate name such as `FeatureGate`. How are regressions prevented: - Promoting `UpgradeStatus` in o/api has no risks because it does not change any behavior except test selection - Running candidate CLI behavior against Default clusters actually provides better quality signal than against TP clusters - Tests tagged with `UpgradeStatus` will need better scrutiny before merge because they will run in Default jobs and not just TP jobs - The actual promotion in o/oc is still gated by data evidence of passing tests, in spirit with the usual promotion process
OTA uses `UpgradeStatus` feature gate name to identify the CLI-side functionality described in OTA-1577 (`oc adm upgrade status` command). The `UpgradeStatus` feature gate does not enable any cluster-side functionality (neither API changes or any controller code). The actual functionality will be enabled in `oc` through openshift/oc#2063. The `UpgradeStatus` feature gate thus serves only as a vehicle to perform the data-driven promotion decision by tying the appropriate test coverage to this name so that the promotion criteria can be automatically checked. Therefore, this PR serves as a formal decision record only, and after it merges, it means openshift/oc#2063 is allowed to merge.
OTA uses `UpgradeStatus` feature gate name to identify the CLI-side functionality described in OTA-1577 (`oc adm upgrade status` command). The `UpgradeStatus` feature gate does not enable any cluster-side functionality (neither API changes or any controller code). The actual functionality will be enabled in `oc` through openshift/oc#2063. The `UpgradeStatus` feature gate thus serves only as a vehicle to perform the data-driven promotion decision by tying the appropriate test coverage to this name so that the promotion criteria can be automatically checked. Therefore, this PR serves as a formal decision record only, and after it merges, it means openshift/oc#2063 is allowed to merge.
- Stop gating the command on an env variable presence - Hide the mock input flag
46522d3 to
cb33bf8
Compare
oc adm upgrade statusoc adm upgrade status to general availability
|
@petr-muller: This pull request references OTA-1581 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/hold cancel |
Co-authored-by: Hongkai Liu <[email protected]>
|
/label tide/merge-method-squash |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, petr-muller The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@petr-muller: The following tests 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. |
f114b17
into
openshift:main
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-cli |
|
[ART PR BUILD NOTIFIER] Distgit: ose-tools |
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-deployer |
|
[ART PR BUILD NOTIFIER] Distgit: ose-cli-artifacts |
openshift/oc#2063 promoted `oc adm upgrade status` and removed the conditional check of the `OC_ENABLE_CMD_UPGRADE_STATUS` envvar, so the tests should no longer set it either.
…mmits Originally, the change-log generation focused on merge commits, which are common in many repositories. For example, in this 'oc' repository: $ git log --graph --oneline -10 * 559c67e (HEAD -> main, origin/main) Merge pull request openshift#2083 from wking/oc-adm-upgrade-recommend-certificate-authority |\ | * b98b2e1 pkg/cli/admin/inspectalerts: Pass through --certificate-authority, etc. * | e7900fe Merge pull request openshift#2082 from ardaguclu/code-rabbit-test |\ \ | |/ |/| | * ffe752e Remove mentioning about IRC channel from README * | f114b17 OTA-1581: Promote `oc adm upgrade status` to general availability (openshift#2063) * | 1c54ec0 Merge pull request openshift#2077 from hongkailiu/OTA-1601 |\ \ | * | 5f1dbb8 Move not-lines-related table.Write out of the loop | * | a352662 Truncate long reasons | * | 11f9f47 Reproduce the long reason issue | * | 970f883 status: improve line break handling for CO Message 4299014 (Add squash-merge support into oc adm release info, 2022-04-26, openshift#1116), and mentioned the insights operator: $ git clone --depth 10 --branch master https://github.com/openshift/insights-operator $ cd insights-operator $ git log --graph --oneline -5 * da45333 (HEAD -> master, origin/master, origin/HEAD) OCPBUGS-60611: virt launcher logs gatherer (openshift#1110) * 737b59b fix: incorrect anonymization of domains (openshift#1111) * b5185da feat: add permissions to gather clusterrole (openshift#1106) * 3099eb0 feat: Allow on-demand gathering during initial periodic run (openshift#1109) * 7f8b40d RFE-4152: Add missing readonlyRootFilesystem (openshift#1104) But some repositories also use a mixture of real merges and squash or rebase merges: $ git clone --depth 20 --branch main https://github.com/openshift/operator-framework-operator-controller $ cd operator-framework-operator-controller $ git log --graph --oneline -11 * 9319f22a (HEAD -> main, origin/main, origin/HEAD) UPSTREAM: <carry>: Ensure unique name for bad-catalog tests * 9b50498a UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * 2ed1a5c8 UPSTREAM: <carry>: Migrate single/own namespace tests * 0c4f4303 UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin * 28299f38 UPSTREAM: <carry>: remove obsolete owners * 5aa7897f UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests * 0bb19537 UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * e9e3220f UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation * a7d35272 Merge pull request openshift#453 from openshift-bot/synchronize-upstream |\ | * d93f9d16 UPSTREAM: <drop>: configure the commit-checker | * 9a69e8ed UPSTREAM: <drop>: remove upstream GitHub configuration Before this commit, the logic would find the merge commits, assume that all content came in via merge commits, and not mention the other changes: $ git log --first-parent --merges --oneline a7d3527 Merge pull request openshift#453 from openshift-bot/synchronize-upstream 4cae159 Merge pull request openshift#449 from openshift-bot/synchronize-upstream 82f63dc Merge pull request openshift#444 from openshift-bot/synchronize-upstream 6f91d84 Merge pull request openshift#435 from openshift-bot/synchronize-upstream With this commit, we'll continue to include a line for each of those merges. But we'll also include the count of any elided, non-merge, first-parent commits: $ git log --first-parent --no-merges --oneline | wc -l 16 Folks interested in what exactly was elided will have to click through to 'Full changelog' or otherwise go digging. But at least they'll get the count of elided commits to know that there is more information there to see behind that click.
…mmits Originally, the change-log generation focused on merge commits, which are common in many repositories. For example, in this 'oc' repository: $ git log --graph --oneline -10 * 559c67e (HEAD -> main, origin/main) Merge pull request openshift#2083 from wking/oc-adm-upgrade-recommend-certificate-authority |\ | * b98b2e1 pkg/cli/admin/inspectalerts: Pass through --certificate-authority, etc. * | e7900fe Merge pull request openshift#2082 from ardaguclu/code-rabbit-test |\ \ | |/ |/| | * ffe752e Remove mentioning about IRC channel from README * | f114b17 OTA-1581: Promote `oc adm upgrade status` to general availability (openshift#2063) * | 1c54ec0 Merge pull request openshift#2077 from hongkailiu/OTA-1601 |\ \ | * | 5f1dbb8 Move not-lines-related table.Write out of the loop | * | a352662 Truncate long reasons | * | 11f9f47 Reproduce the long reason issue | * | 970f883 status: improve line break handling for CO Message 4299014 (Add squash-merge support into oc adm release info, 2022-04-26, openshift#1116), and mentioned the insights operator: $ git clone --depth 10 --branch master https://github.com/openshift/insights-operator $ cd insights-operator $ git log --graph --oneline -5 * da45333 (HEAD -> master, origin/master, origin/HEAD) OCPBUGS-60611: virt launcher logs gatherer (openshift#1110) * 737b59b fix: incorrect anonymization of domains (openshift#1111) * b5185da feat: add permissions to gather clusterrole (openshift#1106) * 3099eb0 feat: Allow on-demand gathering during initial periodic run (openshift#1109) * 7f8b40d RFE-4152: Add missing readonlyRootFilesystem (openshift#1104) But some repositories also use a mixture of real merges and squash or rebase merges: $ git clone --depth 20 --branch main https://github.com/openshift/operator-framework-operator-controller $ cd operator-framework-operator-controller $ git log --graph --oneline -11 * 9319f22a (HEAD -> main, origin/main, origin/HEAD) UPSTREAM: <carry>: Ensure unique name for bad-catalog tests * 9b50498a UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * 2ed1a5c8 UPSTREAM: <carry>: Migrate single/own namespace tests * 0c4f4303 UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin * 28299f38 UPSTREAM: <carry>: remove obsolete owners * 5aa7897f UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests * 0bb19537 UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * e9e3220f UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation * a7d35272 Merge pull request openshift#453 from openshift-bot/synchronize-upstream |\ | * d93f9d16 UPSTREAM: <drop>: configure the commit-checker | * 9a69e8ed UPSTREAM: <drop>: remove upstream GitHub configuration Before this commit, the logic would find the merge commits, assume that all content came in via merge commits, and not mention the other changes: $ git log --first-parent --merges --oneline a7d3527 Merge pull request openshift#453 from openshift-bot/synchronize-upstream 4cae159 Merge pull request openshift#449 from openshift-bot/synchronize-upstream 82f63dc Merge pull request openshift#444 from openshift-bot/synchronize-upstream 6f91d84 Merge pull request openshift#435 from openshift-bot/synchronize-upstream With this commit, we'll continue to include a line for each of those merges. But we'll also include the count of any elided, non-merge, first-parent commits: $ git log --first-parent --no-merges --oneline | wc -l 16 Folks interested in what exactly was elided will have to click through to 'Full changelog' or otherwise go digging. But at least they'll get the count of elided commits to know that there is more information there to see behind that click.
…mmits Originally, the change-log generation focused on merge commits, which are common in many repositories. For example, in this 'oc' repository: $ git log --graph --oneline -10 * 559c67e (HEAD -> main, origin/main) Merge pull request openshift#2083 from wking/oc-adm-upgrade-recommend-certificate-authority |\ | * b98b2e1 pkg/cli/admin/inspectalerts: Pass through --certificate-authority, etc. * | e7900fe Merge pull request openshift#2082 from ardaguclu/code-rabbit-test |\ \ | |/ |/| | * ffe752e Remove mentioning about IRC channel from README * | f114b17 OTA-1581: Promote `oc adm upgrade status` to general availability (openshift#2063) * | 1c54ec0 Merge pull request openshift#2077 from hongkailiu/OTA-1601 |\ \ | * | 5f1dbb8 Move not-lines-related table.Write out of the loop | * | a352662 Truncate long reasons | * | 11f9f47 Reproduce the long reason issue | * | 970f883 status: improve line break handling for CO Message 4299014 (Add squash-merge support into oc adm release info, 2022-04-26, openshift#1116), and mentioned the insights operator: $ git clone --depth 10 --branch master https://github.com/openshift/insights-operator $ cd insights-operator $ git log --graph --oneline -5 * da45333 (HEAD -> master, origin/master, origin/HEAD) OCPBUGS-60611: virt launcher logs gatherer (openshift#1110) * 737b59b fix: incorrect anonymization of domains (openshift#1111) * b5185da feat: add permissions to gather clusterrole (openshift#1106) * 3099eb0 feat: Allow on-demand gathering during initial periodic run (openshift#1109) * 7f8b40d RFE-4152: Add missing readonlyRootFilesystem (openshift#1104) But some repositories also use a mixture of real merges and squash or rebase merges: $ git clone --depth 20 --branch main https://github.com/openshift/operator-framework-operator-controller $ cd operator-framework-operator-controller $ git log --graph --oneline -11 * 9319f22a (HEAD -> main, origin/main, origin/HEAD) UPSTREAM: <carry>: Ensure unique name for bad-catalog tests * 9b50498a UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * 2ed1a5c8 UPSTREAM: <carry>: Migrate single/own namespace tests * 0c4f4303 UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin * 28299f38 UPSTREAM: <carry>: remove obsolete owners * 5aa7897f UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests * 0bb19537 UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * e9e3220f UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation * a7d35272 Merge pull request openshift#453 from openshift-bot/synchronize-upstream |\ | * d93f9d16 UPSTREAM: <drop>: configure the commit-checker | * 9a69e8ed UPSTREAM: <drop>: remove upstream GitHub configuration Before this commit, the logic would find the merge commits, assume that all content came in via merge commits, and not mention the other changes: $ git log --first-parent --merges --oneline a7d3527 Merge pull request openshift#453 from openshift-bot/synchronize-upstream 4cae159 Merge pull request openshift#449 from openshift-bot/synchronize-upstream 82f63dc Merge pull request openshift#444 from openshift-bot/synchronize-upstream 6f91d84 Merge pull request openshift#435 from openshift-bot/synchronize-upstream With this commit, we'll continue to include a line for each of those merges. But we'll also include the count of any elided, non-merge, first-parent commits: $ git log --first-parent --no-merges --oneline | wc -l 16 Folks interested in what exactly was elided will have to click through to 'Full changelog' or otherwise go digging. But at least they'll get the count of elided commits to know that there is more information there to see behind that click.
openshift/oc#2063 promoted `oc adm upgrade status` and removed the conditional check of the `OC_ENABLE_CMD_UPGRADE_STATUS` envvar, so the tests should no longer set it either.
The
UpgradeStatusfeature gate became eligible for promotion (openshift/api#2418 by passing the ci/prow/verify-feature-promotion check.