Skip to content

OTA-1637: ClusterOperators should not go Progressing only for cluster scaling #30297

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 10, 2025
Merged

OTA-1637: ClusterOperators should not go Progressing only for cluster scaling #30297

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8756d70
ClusterOperators should not go Progressing only for cluster scaling
hongkailiu Sep 23, 2025
c9c5fa5
Add exceptions for the violating COs
hongkailiu Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 54 additions & 3 deletions test/extended/machines/scale.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"
configv1 "github.com/openshift/api/config/v1"
configclient "github.com/openshift/client-go/config/clientset/versioned"
bmhelper "github.com/openshift/origin/test/extended/baremetal"
"github.com/stretchr/objx"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -186,12 +188,28 @@ func scaleMachineSet(name string, replicas int) error {
return nil
}

func getOperatorsNotProgressing(c configclient.Interface) map[string]metav1.Time {
operators, err := c.ConfigV1().ClusterOperators().List(context.Background(), metav1.ListOptions{})
o.Expect(err).NotTo(o.HaveOccurred())
result := map[string]metav1.Time{}
for _, operator := range operators.Items {
for _, condition := range operator.Status.Conditions {
if condition.Type == configv1.OperatorProgressing && condition.Status == configv1.ConditionFalse {
result[operator.Name] = condition.LastTransitionTime
}
}
}
return result
}

var _ = g.Describe("[sig-cluster-lifecycle][Feature:Machines][Serial] Managed cluster should", func() {

var (
c *kubernetes.Clientset
dc dynamic.Interface
helper *bmhelper.BaremetalTestHelper
c *kubernetes.Clientset
configClient configclient.Interface
dc dynamic.Interface
helper *bmhelper.BaremetalTestHelper
operatorsNotProgressing map[string]metav1.Time
)

g.BeforeEach(func() {
Expand All @@ -210,10 +228,43 @@ var _ = g.Describe("[sig-cluster-lifecycle][Feature:Machines][Serial] Managed cl
helper.Setup()
helper.DeployExtraWorker(0)
}

configClient, err = configclient.NewForConfig(cfg)
o.Expect(err).NotTo(o.HaveOccurred())
operatorsNotProgressing = getOperatorsNotProgressing(configClient)
})

g.AfterEach(func() {
helper.DeleteAllExtraWorkers()

except := func(co string) string {
switch co {
case "dns":
return "https://issues.redhat.com/browse/OCPBUGS-62623"
case "image-registry":
return "https://issues.redhat.com/browse/OCPBUGS-62626"
case "network":
return "https://issues.redhat.com/browse/OCPBUGS-62630"
case "node-tuning":
return "https://issues.redhat.com/browse/OCPBUGS-62632"
case "storage":
return "https://issues.redhat.com/browse/OCPBUGS-62633"
default:
return ""
}
}

// No cluster operator should leave Progressing=False only up to cluster scaling
// https://github.com/openshift/api/blob/61248d910ff74aef020492922d14e6dadaba598b/config/v1/types_cluster_operator.go#L163-L164
operatorsNotProgressingAfter := getOperatorsNotProgressing(configClient)
var violations []string
for operator, t1 := range operatorsNotProgressing {
t2, ok := operatorsNotProgressingAfter[operator]
if reason := except(operator); reason == "" && (!ok || t1.Unix() != t2.Unix()) {
violations = append(violations, operator)
}
}
o.Expect(violations).To(o.BeEmpty(), "those cluster operators left Progressing=False while cluster was scaling: %v", violations)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will become one of those test failures (if any) that becomes hard to assign to individual components. Since it is a single test that can impact multiple operators.

Could Expect be called in the for loop for and include the operator in the name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, nevermind I see it isn't a new test just new information regarding the test failure.

Copy link
Member Author

@hongkailiu hongkailiu Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review.
I thought about it too: "hard to assign to individual components".
The modified test is an extended test (under /test/extended).
I do not see how to insert junitapi.JUnitTestCase like what a monitortest does in CollectData and EvaluateTestsFromConstructedIntervals.

And yes, if it fails in the future, we have to check the error msg from expect and manually triage the OCPBugs.

})

// The 30m timeout is essentially required by the baremetal platform environment,
Expand Down