OTA-1035: Allow testing `adm upgrade status` on mock data #1595

petr-muller · 2023-11-02T15:22:51Z

While oc adm upgrade status is under development, logic is client-side, and
behind feature gate, it is useful to avoid relying on an actual upgrading
cluster in an interesting state at hand.

This will also allow us to capture interesting "stuck" upgrade cases and
back-test further development on previously seen interesting states.

I have added a simple improvement commit to this PR (emiting <none> when a reason or message is empty) to illustrate:

$ OC_ENABLE_CMD_UPGRADE_STATUS=true ./oc --context build01 adm upgrade status
error: You must be logged in to the server (Unauthorized)
// my $KUBECONFIG does not have an authorized b01 context so I cannot test 

$ OC_ENABLE_CMD_UPGRADE_STATUS=true ./oc --context build01 adm upgrade status \
  --mock-clusteroperators=clusteroperators-clusteroperatorlist.yaml \
  --mock-clusterversion=clusterversion.yaml
The cluster version is not updating (Progressing=False).

  Reason: <none>
  Message: Cluster version is 4.14.1

openshift-ci-robot · 2023-11-02T15:22:54Z

@petr-muller: This pull request references OTA-1035 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 story to target the "4.15.0" version, but no target version was set.

Details

In response to this:

While oc adm upgrade status is under development, logic is client-side, and
behind feature gate, it is useful to avoid relying on an actual upgrading
cluster in an interesting state at hand.

This will also allow us to capture interesting "stuck" upgrade cases and
back-test further development on previously seen interesting states.

I have added a simple improvement commit to this PR (emiting <none> when a reason or message is empty) to illustrate:
$ OC_ENABLE_CMD_UPGRADE_STATUS=true ./oc --context build01 adm upgrade status
error: You must be logged in to the server (Unauthorized)
// my $KUBECONFIG does not have an authorized b01 context so I cannot test 

$ OC_ENABLE_CMD_UPGRADE_STATUS=true ./oc --context build01 adm upgrade status \
 --mock-clusteroperators=clusteroperators-clusteroperatorlist.yaml \
 --mock-clusterversion=clusterversion.yaml
The cluster version is not updating (Progressing=False).

 Reason: <none>
 Message: Cluster version is 4.14.1

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/test-infra repository.

petr-muller · 2023-11-07T15:33:13Z

/retest

wking · 2023-11-07T18:29:45Z

pkg/cli/admin/upgrade/status/status.go

+	// TODO: We can remove these flags once the idea about `oc adm upgrade status` stabilizes and the command
+	//       is promoted out of the OC_ENABLE_CMD_UPGRADE_STATUS feature gate
+	flags.StringVar(&o.mockCvPath, "mock-clusterversion", "", "Path to a YAML ClusterVersion object to use for testing (will be removed later).")
+	flags.StringVar(&o.mockOperatorsPath, "mock-clusteroperators", "", "Path to a YAML ClusterOperatorList to use for testing (will be removed later).")


Did we want to add some fixtures to this to unit test known configs vs. expected output? Because running go test ./pkg/cli/admin/upgrade/status and having it consume mock data other folks have written and confirm that I haven't broken the output other folks were expecting in those situations would be nice. Mainly to start building a corpus of situations we want to cover, and as a way to track how our output in those situations evolves over time.

Also, no worries if you think those fixtures sound like overkill, or if they sound useful in the future, but not worth picking up yet. I'm also fine with a middle ground where we start a directory like pkg/cli/admin/upgrade/status/fixtures/happy-mid-update, etc. with the mock content in it, even if we don't version-control expected output yet.

That sounds great, will do!

wking

Looks good to me. Adding a hold in case you want to tweak in response to this, but feel free to lift the hold without changes if none of that sounds right-now appealing ;)

/lgtm
/hold

petr-muller · 2023-11-08T13:57:52Z

/lgtm cancel

I want to address Trevor's suggestion, canceling lgtm to avoid mistakenly lifting the hold

petr-muller · 2023-11-09T15:33:39Z

: Run test unit - container test
: TestNewAppRunBuilds
: TestNewAppRunBuilds/successful_input_image_source_build_with_a_repository

The above really does not seem like it relates to my change?

/test unit

petr-muller · 2023-11-09T15:36:53Z

/hold cancel

openshift-ci · 2023-11-09T18:46:45Z

@petr-muller: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-metal-ipi-ovn-ipv6	`57e9844`	link	false	`/test e2e-metal-ipi-ovn-ipv6`

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

wking

/lgtm

openshift-ci · 2023-11-10T19:18:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking

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:

~~pkg/cli/admin/upgrade/OWNERS~~ [wking]

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

openshift-bot · 2023-11-13T09:12:53Z

[ART PR BUILD NOTIFIER]

This PR has been included in build openshift-enterprise-cli-container-v4.15.0-202311130831.p0.g1d4a9af.assembly.stream for distgit openshift-enterprise-cli.
All builds following this will include this PR.

Catching the docs up with 10c41f5 (add alerts to update health in oc adm upgrade status, 2024-04-25, openshift#1740). Also drop the enumeration that started when the file was created in 57e9844 (`adm upgrade status`: add fixture test, 2023-11-09, openshift#1595). Folks who care how many inputs and outputs there currently are can count for themselves, and this content is easier to maintain without having to bump later entry numbers when inserting an earlier entry.

petr-muller added 2 commits November 2, 2023 16:08

adm update status: add mocking input capabilities

d88dcb2

adm update status: emit <none> on empty reason or message

b6558a1

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 2, 2023

openshift-ci bot requested review from deads2k and mfojtik November 2, 2023 15:33

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2023

wking reviewed Nov 7, 2023

View reviewed changes

wking approved these changes Nov 7, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2023

openshift-ci bot assigned wking Nov 7, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2023

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2023

adm upgrade status: add fixture test

57e9844

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 9, 2023

petr-muller force-pushed the oc-adm-upgrade-mock-inputs-ota-1035 branch from ebcb7bc to 57e9844 Compare November 9, 2023 15:41

wking approved these changes Nov 10, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2023

openshift-merge-bot bot merged commit 1d4a9af into openshift:master Nov 10, 2023

wking mentioned this pull request Jan 14, 2025

NO-ISSUE: admin/upgrade/status/examples/README: Document *-alerts.json #1959

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTA-1035: Allow testing `adm upgrade status` on mock data #1595

OTA-1035: Allow testing `adm upgrade status` on mock data #1595

Uh oh!

petr-muller commented Nov 2, 2023

Uh oh!

openshift-ci-robot commented Nov 2, 2023 •

edited by openshift-ci bot

Loading

Uh oh!

petr-muller commented Nov 7, 2023

Uh oh!

wking Nov 7, 2023

Uh oh!

petr-muller Nov 8, 2023

Uh oh!

wking left a comment

Uh oh!

petr-muller commented Nov 8, 2023

Uh oh!

petr-muller commented Nov 9, 2023 •

edited

Loading

Uh oh!

petr-muller commented Nov 9, 2023

Uh oh!

openshift-ci bot commented Nov 9, 2023

Uh oh!

wking left a comment

Uh oh!

openshift-ci bot commented Nov 10, 2023

Uh oh!

openshift-bot commented Nov 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

OTA-1035: Allow testing adm upgrade status on mock data #1595

OTA-1035: Allow testing adm upgrade status on mock data #1595

Uh oh!

Conversation

petr-muller commented Nov 2, 2023

Uh oh!

openshift-ci-robot commented Nov 2, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petr-muller commented Nov 7, 2023

Uh oh!

wking Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

petr-muller Nov 8, 2023

Choose a reason for hiding this comment

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

petr-muller commented Nov 8, 2023

Uh oh!

petr-muller commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petr-muller commented Nov 9, 2023

Uh oh!

openshift-ci bot commented Nov 9, 2023

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 10, 2023

Uh oh!

openshift-bot commented Nov 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

OTA-1035: Allow testing `adm upgrade status` on mock data #1595

OTA-1035: Allow testing `adm upgrade status` on mock data #1595

openshift-ci-robot commented Nov 2, 2023 •

edited by openshift-ci bot

Loading

petr-muller commented Nov 9, 2023 •

edited

Loading