OTA-1580: Add basic test for oc adm upgrade status CLI
#29954
Conversation
|
@petr-muller: This pull request references OTA-1580 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. DetailsIn 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
/test e2e-gcp-ovn-techpreview |
petr-muller
force-pushed
the
ota-1580-test-oc-adm-upgrade-status
branch
from
dafa750 to
d21513e
Compare
|
/test e2e-gcp-ovn-techpreview |
|
/test e2e-gcp-ovn-techpreview |
2 similar comments
|
/test e2e-gcp-ovn-techpreview |
|
/test e2e-gcp-ovn-techpreview |
|
@petr-muller: This pull request references OTA-1580 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. DetailsIn 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. |
|
/retest-required |
|
/test ? |
|
@petr-muller: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn 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. |
|
/test e2e-gcp-ovn-techpreview |
|
/cc |
test/extended/util/client.go
Outdated
| // EnvVar sets an environment variable for the command, appended to the current process environment variables. | ||
| func (c *CLI) EnvVar(name, value string) *CLI { | ||
| if c.env == nil { | ||
| c.env = os.Environ() |
There was a problem hiding this comment.
nit: are process environment variables mutable (Setenv suggests yes?)? We might want to store the local-clobber options in the struct as a map[string]string. And then grab os.Environ() and inject the struct-property overrides within CLI.start right when we need them.
I'm fine if you don't want to address this nit. It seems unlikely that we'll be mutating environment variables within the e2e-suite process. And if we change our mind later, it would be an internal change within the CLI implementation; callers wouldn't have to adjust.
There was a problem hiding this comment.
The concern would be a Setenv done between EnvVar and Run getting "lost", right? Good catch, addressed.
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
Your approach to environment variables appending/overriding os.Environ makes sense to me. I left one nit inline, but I'm fine if you don't address the nit before this pull merges.
/lgtm
| "k8s.io/kubernetes/test/e2e/framework" | ||
| ) | ||
|
|
||
| var _ = g.Describe("[sig-cli][OCPFeatureGate:UpgradeStatus] oc adm upgrade status", func() { |
There was a problem hiding this comment.
I'm conflicted about the [OCPFeatureGate:UpgradeStatus] tag. On the one hand, the status subcommand is tech-preview. And the UpgradeStatus feature gate exists, although it's not all that clear in #1725 whether it's trying to cover the oc-side unctionality or the planned-but-abandoned server-side APIs (openshift/enhancements#1701). Using the UpgradeStatus gate to enable this test keeps us from failing default-feature-set CI if the test starts failing, and failing default-feature-set CI on a tech-preview feature isn't desired.
On the other hand, our docs point out that you can run the tech-preview status subcommand against a default-feature-set cluster:
Your cluster does not need to be a Technology Preview-enabled cluster in order for you to use the
oc adm upgrade statuscommand.
and as your EnvVar call in the test shows, the knob that enables the status subcommand is the OC_ENABLE_CMD_UPGRADE_STATUS environment variable in the oc process, not the UpgradeStatus feature-gate in the cluster oc is connecting to. And we want CI coverage to verify we're delivering the functionality our docs claim we're delivering. But we don't currently have a CI bucket for "here's some tech-preview client-side stuff we want to confirm works against default-feature-set clusters", and it's not clear if there's enough demand to be worth creating that kind of CI flavor. So which of these options do we like most?
a. [OCPFeatureGate:UpgradeStatus] gating here. Only test the tech-preview oc subcommand against tech-preview clusters. Expose ourselves to regressions using the tech-preview oc against default-feature-gate clusters, even though we doc that as supported.
b. No OCPFeatureGate gating here. Potentially break default-feature-gate CI if this tech-preview functionality acts up. Also maybe miss the API-approver tooling that uses this as a marker of whether the feature is GA-ready?
c. Build a new CI flavor that understands how to test tech-preview client-side functionality against a default-feature-gate cluster, in a way compatible with API-approver tooling around deciding if a feature is GA-ready.
None of those sound awesome to me, but given the stability I think we can deliver for this particular subcommand, I think I currently prefer (b), if we can find some way to convince API-approvers that the passing test results mean we're GA-ready, even though we aren't using the OCPFeatureGate test-case naming they'd been used to. I'd also be happy with (c), if someone was able to wave a wand and deliver it for free 😄 , but I'm guessing that delivering it would be non-trivial work.
I also don't think this needs to block merging. We can pivot between strategies as we go, if we decide to reevaluate the weighting of the different options.
There was a problem hiding this comment.
Not knowing the super details of Trevor's concern about using [OCPFeatureGate:UpgradeStatus], but do I understand correctly that this UpgradeStatus might not be the most accurate FG to use for this test? If so, and in case this FG graduates to default in the future, this test will start running against the default-feature-set cluster (given the removal of the FG), and we might all of sudden start testing new scenarios (FG test against default-feature-set)? Will that be the risk if we just let PR go as it is? TRT has no problem of approving this if the dev team lgtm it. But we need to understand the situation and dev team's decision. Also, as Trevor pointed out, we know that we are not testing something we are advertising (FG test against default-feature-set).
There was a problem hiding this comment.
It has a bit of a history:
- The
UpgradeStatusfeature gate was created for the development of UpdateStatus API in 4.16 - To explore what exactly should the API do, we built a CLI-side implementation which we released as TechPreview because it was fast and allowed us to ship something customers could provide us feedback for. We "technically" put it behind the UpdateStatus gate (I say "technically" cause the o/api gate and the actual o/oc gating mechanisms are disconnected) to track its TechPreview status. The idea was that for eventual GA we will replace the client-side logic with cluster-side one that the command would just read through an API
- The UpgradeStatus API idea got killed eventually
- The BU has demand to ship the CLI-side implementation and we have architect and director-level mandate to GA it in 4.20
- The CLI is still technically TechPreview, and to be promoted we need tests -> hence this effort. The
UpdateStatusgate lost its original purpose of shipping an API, its name fits and I may as well use it as a "promotion vehicle" because it has no other purpose. Its promotion equals this CLI promotion. - The
UpgradeStatuspromotion changes no other behavior in the cluster currently: all that functionality was already removed (there's one very small exception which we track in OTA-1578 which we know has to be removed before the promotion and track it that way).
The ideal outcome is the feature gets promoted and then we do not have the TP / Default skew anymore. If I could set up the jobs to exercise this easily (while still feeding into the promotion process) I would do so but there seems to be no way to do so. And while the TP/Default skew is a risk in theory, there is no known reason why it should be a large risk because the API surface to which the CLI is sensitive is all old stable apis and we use no server-side TP features.
There was a problem hiding this comment.
Thanks for the explanation! This situation is indeed unique.
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d21513e
New tests seen in this PR at sha: d21513e
|
petr-muller
force-pushed
the
ota-1580-test-oc-adm-upgrade-status
branch
from
d21513e to
26628e3
Compare
petr-muller
force-pushed
the
ota-1580-test-oc-adm-upgrade-status
branch
3 times, most recently
from
c178927 to
5cadb10
Compare
|
/test e2e-gcp-ovn-techpreview |
|
/test e2e-gcp-ovn-techpreview |
|
/approve |
openshift-ci
bot
added
the
approved
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 5cadb10
|
openshift-merge-robot
added
the
needs-rebase
- Add a new subdirectory to hold OTA-owned `oc adm upgrade` subcommand tests - Add a way to pass env vars to `oc` CLI to enable gated subcommand testing - When not updating, the command emits a simple message stating just that
petr-muller
force-pushed
the
ota-1580-test-oc-adm-upgrade-status
branch
from
5cadb10 to
85cad81
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/test e2e-gcp-ovn-techpreview |
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
Still looks good to me with the generated-file refresh :)
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw, petr-muller, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
|
Job Failure Risk Analysis for sha: 85cad81
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 85cad81
|
|
@petr-muller: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
openshift-merge-bot
bot
merged commit e1ae86a
into
openshift:main
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-tests |
8 participants
oc adm upgradesubcommand testsocCLI to enable gated subcommand testingThis has some overlap with #29831 (code placement, ownership, client support for enabling environment variables)