OTA-1580: Add basic test for `oc adm upgrade status` CLI #29954

wking · 2025-07-17T20:45:06Z

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 status command.

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.

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).

It has a bit of a history:

The UpgradeStatus feature 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 UpdateStatus gate 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 UpgradeStatus promotion 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.

Thanks for the explanation! This situation is indeed unique.

-Original file line number
+Diff line change
@@ -0,0 +1,6 @@
+    # See the OWNERS docs: https://git.k8s.io/community/contributors/guide/owners.md
+    approvers:
+    - cluster-version-operator-test-case-approvers
+    reviewers:
+    - cluster-version-operator-test-case-reviewers

-Original file line number
+Diff line change
@@ -0,0 +1,24 @@
+    package adm_upgrade
+    import (
+    	g "github.com/onsi/ginkgo/v2"
+    	o "github.com/onsi/gomega"
+    	exutil "github.com/openshift/origin/test/extended/util"
+    	"k8s.io/kubernetes/test/e2e/framework"
+    )
+    var _ = g.Describe("[sig-cli][OCPFeatureGate:UpgradeStatus] oc adm upgrade status", func() {
+    	defer g.GinkgoRecover()
+    	f := framework.NewDefaultFramework("oc-adm-upgrade-status")
+    	f.SkipNamespaceCreation = true
+    	oc := exutil.NewCLIWithoutNamespace("oc-adm-upgrade-status").AsAdmin()
+    	g.It("reports correctly when the cluster is not updating", func() {
+    		cmd := oc.Run("adm", "upgrade", "status").EnvVar("OC_ENABLE_CMD_UPGRADE_STATUS", "true")
+    		out, err := cmd.Output()
+    		o.Expect(err).NotTo(o.HaveOccurred())
+    		o.Expect(out).To(o.Equal("The cluster is not updating."))
+    	})
+    })

-Original file line number
+Diff line change
@@ Expand Up / @@ -17,6 +17,7 @@ import ( @@
     	_ "github.com/openshift/origin/test/extended/builds"
     	_ "github.com/openshift/origin/test/extended/ci"
     	_ "github.com/openshift/origin/test/extended/cli"
+    	_ "github.com/openshift/origin/test/extended/cli/adm_upgrade"
     	_ "github.com/openshift/origin/test/extended/cloud_controller_manager"
     	_ "github.com/openshift/origin/test/extended/cluster"
     	_ "github.com/openshift/origin/test/extended/clusterversion"
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -92,15 +92,22 @@ type CLI struct { @@
     	// manifest files can be stored under directory tree
     	staticConfigManifestDir string
-    	token                string
-    	username             string
-    	globalArgs           []string
-    	commandArgs          []string
-    	finalArgs            []string
-    	namespacesToDelete   []string
-    	stdin                *bytes.Buffer
-    	stdout               io.Writer
-    	stderr               io.Writer
+    	token              string
+    	username           string
+    	globalArgs         []string
+    	commandArgs        []string
+    	finalArgs          []string
+    	namespacesToDelete []string
+    	stdin              *bytes.Buffer
+    	stdout             io.Writer
+    	stderr             io.Writer
+    	// env allows setting environment variables for the command (when nil, the environment
+    	// is inherited from the current process)
+    	env []string
+    	// addEnvVars allows adding environment variables on top of what is defined in env.
+    	addEnvVars map[string]string
     	verbose              bool
     	withoutNamespace     bool
     	withManagedNamespace bool
@@ Expand Down Expand Up / @@ -883,6 +890,26 @@ func (c *CLI) setOutput(out io.Writer) *CLI { @@
     	return c
     }
+    // Env sets the command's environment variables with the same semantics as exec.Cmd's Env property.
+    // https://pkg.go.dev/os/exec#Cmd
+    //
+    // EnvVar()-provided variables will be appended to whatever Env was set before.
+    func (c *CLI) Env(env ...string) *CLI {
+    	c.env = env
+    	return c
+    }
+    // EnvVar sets an environment variable for the command, appended to whatever Env is set on the CLI
+    // when eventually executed, or to environment inherited from the current process if Env() was not
+    // called.
+    func (c *CLI) EnvVar(name, value string) *CLI {
+    	if c.addEnvVars == nil {
+    		c.addEnvVars = make(map[string]string)
+    	}
+    	c.addEnvVars[name] = value
+    	return c
+    }
     // Run executes given OpenShift CLI command verb (iow. "oc <verb>").
     // This function also override the default 'stdout' to redirect all output
     // to a buffer and prepare the global flags such as namespace and config path.
@@ Expand Down Expand Up @@
     	// Redact any bearer token information from the log.
     	framework.Logf("Running '%s %s'", c.execPath, RedactBearerToken(strings.Join(c.finalArgs, " ")))
+    	cmd.Env = c.env
+    	if len(c.addEnvVars) > 0 {
+    		// This is a nil check to allow setting empty environment with Env()
+    		if cmd.Env == nil {
+    			cmd.Env = os.Environ()
+    		}
+    		for name, value := range c.addEnvVars {
+    			cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", name, value))
+    		}
+    	}
     	cmd.Stdout = stdOutBuff
     	cmd.Stderr = stdErrBuff
     	err := cmd.Start()
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -1575,6 +1575,10 @@ spec: @@
             container''s primary UID belongs to some groups in the image when scheduled
             node supports SupplementalGroupsPolicy it should NOT add SupplementalGroups
             to them [LinuxOnly]'
+      - featureGate: UpgradeStatus
+        tests:
+        - testName: '[sig-cli][OCPFeatureGate:UpgradeStatus] oc adm upgrade status reports
+            correctly when the cluster is not updating'
       - featureGate: UserNamespacesSupport
         tests:
         - testName: '[Suite:openshift/usernamespace] [sig-node] [FeatureGate:ProcMountType]
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTA-1580: Add basic test for `oc adm upgrade status` CLI #29954

Uh oh!

Diff view

Diff view

There are no files selected for viewing

wking Jul 17, 2025 •

edited

Loading

Uh oh!

xueqzhan Jul 18, 2025 •

edited

Loading

Uh oh!

petr-muller Jul 18, 2025

Uh oh!

xueqzhan Jul 18, 2025

Uh oh!

Uh oh!

OTA-1580: Add basic test for oc adm upgrade status CLI #29954

Uh oh!

OTA-1580: Add basic test for oc adm upgrade status CLI #29954

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

wking Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xueqzhan Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petr-muller Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

xueqzhan Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OTA-1580: Add basic test for `oc adm upgrade status` CLI #29954

OTA-1580: Add basic test for `oc adm upgrade status` CLI #29954

wking Jul 17, 2025 •

edited

Loading

xueqzhan Jul 18, 2025 •

edited

Loading