-
Notifications
You must be signed in to change notification settings - Fork 47
OCPCLOUD-3042: Create control-plane-machine-set-tests-ext command for origin e2e #363
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
Conversation
|
@sunzhaohua2: This pull request references OCPQE-28828 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 spike 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. |
811abcd to
de60708
Compare
mdbooth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just learning about this tool from reading about your review, so if my comments make no sense just let me know! Looks cool.
| }) | ||
|
|
||
| var err error | ||
| framework.GlobalFramework, err = framework.NewFramework() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in suite_test.go we're calling a framework module method and using the returned value to set a framework module global.
Could we rename framework.NewFramework() to framework.InitFramework() and have it:
- only return error
- set its internal global directly
?
|
|
||
| ext.AddSuite(e.Suite{ | ||
| Name: "cpmso/periodic", | ||
| Qualifiers: []string{`!labels.exists(l, l == "Periodic")`}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks different to an example expressions in the enhancement:
"(test.tags.suite==\"fips\" || test.name.contains(\"fips\")) && !test.labels.has(\"Disruptive\")",
From that I'd have expected something like:
`test.labels.has("Periodic")`
Have we tested it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdbooth thanks for your review. yes, I have tested, I refered to https://github.com/openshift/machine-api-operator/blob/main/cmd/machine-api-tests-ext/main.go#L37
List test cases
$ bin/control-plane-machine-set-tests-ext list tests --suite cpmso/presubmit
[
{
"name": "ControlPlaneMachineSet Operator With an active ControlPlaneMachineSet and the provider spec of index 1 is not as expected should rolling update replace the outdated machine",
"labels": {
"PreSubmit": {}
},
"resources": {
"isolation": {}
},
"source": "openshift:payload:cluster-control-plane-machine-set-operator",
"codeLocations": [
"/Users/zhsun/go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/presubmit.go:34",
"/Users/zhsun/go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/presubmit.go:35",
"/Users/zhsun/go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/presubmit.go:39",
"/Users/zhsun/go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/presubmit.go:40",
"/Users/zhsun/go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/presubmit.go:44",
"/Users/zhsun/go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/presubmit.go:45",
"/Users/zhsun/go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:129"
],
"lifecycle": "blocking",
"environmentSelector": {}
},
...
Run test suites
$ bin/control-plane-machine-set-tests-ext run-suite cpmso/presubmit
Running Suite: - /Users/zhsun/go/src/github.com/openshift/cluster-control-plane-machine-set-operator
=====================================================================================================
Random Seed: 1753839740 - will randomize all specs
Will run 1 of 1 specs
------------------------------
ControlPlaneMachineSet Operator With an active ControlPlaneMachineSet and ControlPlaneMachineSet is updated to set MachineNamePrefix [OCPFeatureGate:CPMSMachineNamePrefix] and the provider spec of index 1 is not as expected should rolling update replace the outdated machine [PreSubmit]
/Users/zhsun/go/src/github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/helpers/cases.go:129
STEP: Waiting for the cluster operators to stabilise (minimum availability time: 1m0s, timeout: 10m0s, polling interval: 10s) @ 07/30/25 09:42:21.898
STEP: Checking the control plane machine set exists @ 07/30/25 09:42:22.173
STEP: Checking the control plane machine set is active @ 07/30/25 09:42:22.41
STEP: Updating the machine name prefix of the control plane machine set to "master-prefix" @ 07/30/25 09:42:23.06
STEP: Updating the provider spec of the control plane machine at index 1 @ 07/30/25 09:42:25.214
STEP: Waiting for the index 1 to be replaced @ 07/30/25 09:42:25.959
STEP: Checking the number of control plane machines never goes above 4 replicas @ 07/30/25 09:42:25.959
STEP: Checking that other indexes (not 1) do not have 2 replicas @ 07/30/25 09:42:25.961
STEP: Waiting for the updated replicas to equal desired replicas @ 07/30/25 09:42:25.962
STEP: Checking that index 1 has 2 replicas @ 07/30/25 09:42:26.39
|
@sunzhaohua2: This pull request references OCPCLOUD-3042 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.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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/remove-lifecycle stale |
|
/testwith openshift/cluster-control-plane-machine-set-operator/main/e2e-aws-ovn openshift/origin#30452 |
|
/retest-required |
|
/retest |
damdo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sunzhaohua2
i think we need to slightly move the pkg to a separate module if possible
| replace ( | ||
| github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20251001123353-fd5b1fb35db1 | ||
| go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc => go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.53.0 | ||
| k8s.io/api => github.com/openshift/kubernetes/staging/src/k8s.io/api v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/apiextensions-apiserver => github.com/openshift/kubernetes/staging/src/k8s.io/apiextensions-apiserver v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/apimachinery => github.com/openshift/kubernetes/staging/src/k8s.io/apimachinery v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/apiserver => github.com/openshift/kubernetes/staging/src/k8s.io/apiserver v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/cli-runtime => github.com/openshift/kubernetes/staging/src/k8s.io/cli-runtime v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/client-go => github.com/openshift/kubernetes/staging/src/k8s.io/client-go v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/cloud-provider => github.com/openshift/kubernetes/staging/src/k8s.io/cloud-provider v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/cluster-bootstrap => github.com/openshift/kubernetes/staging/src/k8s.io/cluster-bootstrap v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/code-generator => github.com/openshift/kubernetes/staging/src/k8s.io/code-generator v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/component-base => github.com/openshift/kubernetes/staging/src/k8s.io/component-base v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/component-helpers => github.com/openshift/kubernetes/staging/src/k8s.io/component-helpers v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/controller-manager => github.com/openshift/kubernetes/staging/src/k8s.io/controller-manager v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/cri-api => github.com/openshift/kubernetes/staging/src/k8s.io/cri-api v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/cri-client => github.com/openshift/kubernetes/staging/src/k8s.io/cri-client v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/csi-translation-lib => github.com/openshift/kubernetes/staging/src/k8s.io/csi-translation-lib v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/dynamic-resource-allocation => github.com/openshift/kubernetes/staging/src/k8s.io/dynamic-resource-allocation v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/endpointslice => github.com/openshift/kubernetes/staging/src/k8s.io/endpointslice v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/externaljwt => github.com/openshift/kubernetes/staging/src/k8s.io/externaljwt v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/kube-aggregator => github.com/openshift/kubernetes/staging/src/k8s.io/kube-aggregator v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/kube-controller-manager => github.com/openshift/kubernetes/staging/src/k8s.io/kube-controller-manager v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/kube-proxy => github.com/openshift/kubernetes/staging/src/k8s.io/kube-proxy v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/kube-scheduler => github.com/openshift/kubernetes/staging/src/k8s.io/kube-scheduler v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/kubectl => github.com/openshift/kubernetes/staging/src/k8s.io/kubectl v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/kubelet => github.com/openshift/kubernetes/staging/src/k8s.io/kubelet v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/kubernetes => github.com/openshift/kubernetes v1.30.1-0.20251017123720-96593f323733 | ||
| k8s.io/metrics => github.com/openshift/kubernetes/staging/src/k8s.io/metrics v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/mount-utils => github.com/openshift/kubernetes/staging/src/k8s.io/mount-utils v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/pod-security-admission => github.com/openshift/kubernetes/staging/src/k8s.io/pod-security-admission v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/sample-apiserver => github.com/openshift/kubernetes/staging/src/k8s.io/sample-apiserver v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/sample-cli-plugin => github.com/openshift/kubernetes/staging/src/k8s.io/sample-cli-plugin v0.0.0-20251017123720-96593f323733 | ||
| k8s.io/sample-controller => github.com/openshift/kubernetes/staging/src/k8s.io/sample-controller v0.0.0-20251017123720-96593f323733 | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't want this in the main module, as this becomes a nightmare to bump every release while also keeping all the dependencies happy.
The idea we had is to follow this approach: openshift/machine-api-operator#1423
Let me know if that makes sense and if we can trial that
| build: operator tests-ext ## Build all binaries | ||
|
|
||
| operator: ## Build main operator binary | ||
| go build -o bin/manager ./cmd/control-plane-machine-set-operator | ||
|
|
||
| tests-ext: ## Build tests extension binary | ||
| go build -o bin/control-plane-machine-set-tests-ext ./cmd/control-plane-machine-set-tests-ext | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These will need to change if we move this to a separate module
| extensionRegistry.Register(ext) | ||
|
|
||
| root := &cobra.Command{ | ||
| Long: "cluster control plane machine set Operator tests extension for OpenShift", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Long: "cluster control plane machine set Operator tests extension for OpenShift", | |
| Long: "cluster-control-plane-machine-set-operator tests extension for OpenShift", |
| go build -o bin/manager ./cmd/control-plane-machine-set-operator | ||
|
|
||
| tests-ext: ## Build tests extension binary | ||
| go build -o bin/control-plane-machine-set-tests-ext ./cmd/control-plane-machine-set-tests-ext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use the cluster-control-plane-machine-set-operator-ext for naming?
| FROM registry.ci.openshift.org/ocp/4.21:base-rhel9 | ||
| COPY --from=builder /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/bin/manager . | ||
| COPY --from=builder /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/manifests manifests | ||
| COPY --from=builder /go/src/github.com/openshift/cluster-control-plane-machine-set-operator/bin/control-plane-machine-set-tests-ext.gz . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use the cluster-control-plane-machine-set-operator-ext for naming?
|
/assign @damdo @RadekManak /cc @vr4manta |
|
@sunzhaohua2: 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. |
| }, | ||
| Qualifiers: []string{`labels.exists(l, l == "Disruptive")`}, | ||
| }) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openshift/disruptive allows limited parallelism, but cpms tests must run serially, this causes resource conflicts and test failures in job https://storage.googleapis.com/test-platform-results/logs/multi-pr-openshift-origin-30452-openshift-cluster-control-plane-machine-set-operator-363-e2e-gcp-disruptive/1985909492008620032/build-log.txt
I will remove this suite, there is no suitable parent suite for cpms tests.
|
/close |
|
@RadekManak: Closed this PR. 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 kubernetes-sigs/prow repository. |
Per design of Openshift Test Extensions create a Control Plane MachineSet Operator OpenShift tests extension.
Get extension usage
Get extension definition info
List test cases
Run test suites
make e2e