[release-4.12] OCPBUGS-14260: Check for minimum OCP version #1644

aravindhp · 2023-06-06T21:14:20Z

OCPBUGS-4862 fixed an issue where deletion of a BYOH node object resulted in it hanging in the Ready,SchedulingDisabled state. This was fixed in OCP 4.12.3 and as a result any WMCO upgrades from 7.0.1 needs to be on versions greater than that. Otherwise BYOH node upgrades will fail and could potentially cause workload disruptions. To prevent that from happening stop WMCO 7.1.0 from running if the OCP version is below 4.12.3.

Fixes OCPBUGS-14260

openshift-ci-robot · 2023-06-06T21:14:26Z

@aravindhp: This pull request references Jira Issue OCPBUGS-14260, which is invalid:

expected the bug to target the "4.12.z" version, but no target version was set
expected Jira Issue OCPBUGS-14260 to depend on a bug targeting a version in 4.13.0, 4.13.z and in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but no dependents were found

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

OCPBUGS-4862 fixed an issue where deletion of a BYOH node objected resulted in it hanging in the Ready,SchedulingDisabled state. This was fixed in OCP 4.12.3 and as a result any WMCO upgrades from 7.0.1 needs to be on versions greater than that. Otherwise BYOH node upgrades will fail and could potentially cause workload disruptions. To prevent that from happening stop WMCO 7.1.0 from running if the OCP version is below 4.12.3.

Fixes OCPBUGS-14260

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.

openshift-ci · 2023-06-06T21:14:48Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

aravindhp · 2023-06-06T21:15:19Z

/approve cancel
/jira refresh

openshift-ci-robot · 2023-06-06T21:15:21Z

@aravindhp: This pull request references Jira Issue OCPBUGS-14260, which is invalid:

expected Jira Issue OCPBUGS-14260 to depend on a bug targeting a version in 4.13.0, 4.13.z and in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but no dependents were found

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/approve cancel
/jira refresh

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.

aravindhp · 2023-06-06T21:16:30Z

/label jira/valid-bug

This bug only affects OCP 4.12 / WMCO 7.0.1

aravindhp · 2023-06-06T21:18:07Z

/test gcp-e2e-operator

aravindhp · 2023-06-06T21:28:00Z

/jira refresh

openshift-ci-robot · 2023-06-06T21:28:03Z

@aravindhp: This pull request references Jira Issue OCPBUGS-14260, which is invalid:

expected Jira Issue OCPBUGS-14260 to depend on a bug targeting a version in 4.13.0, 4.13.z and in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but no dependents were found

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

Retaining the jira/valid-bug label as it was manually added.

In response to this:

/jira refresh

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.

aravindhp · 2023-06-06T23:19:46Z

/test gcp-e2e-operator

aravindhp · 2023-06-07T22:09:14Z

/test gcp-e2e-operator

aravindhp · 2023-06-07T22:55:00Z

/test gcp-e2e-operator

aravindhp · 2023-06-07T22:59:06Z

/test unit

sebsoto · 2023-06-08T12:44:37Z

pkg/cluster/config.go

+	// minOpenShiftVersion is the minimum required OCP version due to https://issues.redhat.com/browse/OCPBUGS-4862.
+	// Without this fix, BYOH node upgrades will fail.


nit: could call out this is the min required OCP version within 4.12.z

We are in the release-4.12 branch. Do we really need to call this out?

sebsoto · 2023-06-08T12:52:50Z

pkg/cluster/config.go

+	var openShiftVersion string
+	for _, update := range clusterVersion.Status.History {
+		if update.State == oconfig.CompletedUpdate {
+			// obtain the version from the last completed update
+			openShiftVersion = update.Version
+			break
+		}
+	}
+	return openShiftVersion, nil


This isn't checking if the version is missing, I suggest doing this instead:

var openShiftVersion string for _, update := range clusterVersion.Status.History { if update.State == oconfig.CompletedUpdate { // obtain the version from the last completed update return update.Version } } return "", fmt.Errorf("no completed updated was found")

sebsoto · 2023-06-08T13:03:10Z

pkg/cluster/config.go

+	if len(openShiftVersion) == 0 {
+		return fmt.Errorf("cluster version not set")
+	}


Instead of doing this check, it would be more comprehensive to add the v prefix and then use semver.isValid to check that the version is formatted properly.

Alternatively you could do this within getOpenShiftVersion, renaming it to getOpenShiftSemver. That way it is guaranteed that getOpenShiftSemver is returning a valid semver, and the code within this function doesn't need to be as defensive.

getOpenShiftSemver would be overloading a function and is not a good pattern. I prefer to keep the validation in one spot and making code more defensive is fine.

sebsoto · 2023-06-08T13:06:47Z

pkg/cluster/config_test.go

+			if (err != nil) != tt.wantErr {
+				assert.Errorf(t, err, "error = %v, wantErr %v", err, tt.wantErr)


Don't you need to do something like this to check if theres an error when we want it, and not when don't?

assert.Equal(t,( err != nil ), tt.wantErr)

It actually has the same effect but I like you version better.

aravindhp · 2023-06-08T15:43:12Z

/retitle OCPBUGS-14260: Check for minimum OCP version

aravindhp

Thanks for the review, @sebsoto. I have addressed your comments.

aravindhp · 2023-06-08T15:44:11Z

pkg/cluster/config.go

+	// minOpenShiftVersion is the minimum required OCP version due to https://issues.redhat.com/browse/OCPBUGS-4862.
+	// Without this fix, BYOH node upgrades will fail.


We are in the release-4.12 branch. Do we really need to call this out?

aravindhp · 2023-06-08T16:04:56Z

pkg/cluster/config.go

+	if len(openShiftVersion) == 0 {
+		return fmt.Errorf("cluster version not set")
+	}


getOpenShiftSemver would be overloading a function and is not a good pattern. I prefer to keep the validation in one spot and making code more defensive is fine.

aravindhp · 2023-06-08T16:41:31Z

pkg/cluster/config_test.go

+			if (err != nil) != tt.wantErr {
+				assert.Errorf(t, err, "error = %v, wantErr %v", err, tt.wantErr)


It actually has the same effect but I like you version better.

openshift-ci · 2023-06-08T17:21:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sebsoto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sebsoto]

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

alinaryan

LGTM other than commit message

's/fixed an issue where
deletion of a BYOH node objected/fixed an issue where
deletion of a BYOH node object/'

https://issues.redhat.com/browse/OCPBUGS-4862 fixed an issue where deletion of a BYOH node object resulted in it hanging in the "Ready,SchedulingDisabled" state. This was fixed in OCP 4.12.3 and as a result any WMCO upgrades from 7.0.1 needs to be on versions greater than that. Otherwise BYOH node upgrades will fail and could potentially cause workload disruptions. To prevent that from happening stop WMCO 7.1.0 from running if the OCP version is below 4.12.3. This check is ignored in CI runs as CI clusters do not advertise Z streams.

openshift-ci-robot · 2023-06-08T18:31:10Z

@aravindhp: This pull request references Jira Issue OCPBUGS-14260, which is invalid:

expected Jira Issue OCPBUGS-14260 to depend on a bug targeting a version in 4.13.0, 4.13.z and in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but no dependents were found

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Retaining the jira/valid-bug label as it was manually added.

In response to this:

OCPBUGS-4862 fixed an issue where deletion of a BYOH node object resulted in it hanging in the Ready,SchedulingDisabled state. This was fixed in OCP 4.12.3 and as a result any WMCO upgrades from 7.0.1 needs to be on versions greater than that. Otherwise BYOH node upgrades will fail and could potentially cause workload disruptions. To prevent that from happening stop WMCO 7.1.0 from running if the OCP version is below 4.12.3.

Fixes OCPBUGS-14260

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.

aravindhp · 2023-06-08T18:31:28Z

LGTM other than commit message

's/fixed an issue where deletion of a BYOH node objected/fixed an issue where deletion of a BYOH node object/'

@alinaryan fixed.

alinaryan · 2023-06-08T19:44:50Z

/lgtm

aravindhp · 2023-06-08T19:45:50Z

/retitle [release-4.12] OCPBUGS-14260: Check for minimum OCP version

openshift-ci-robot · 2023-06-08T22:36:54Z

/retest-required

Remaining retests: 0 against base HEAD d4ffecd and 2 for PR HEAD 2a77c4d in total

aravindhp · 2023-06-09T04:39:15Z

/retest-required

DaemonSet scale down failure in e2e-aws-upgrade

aravindhp · 2023-06-09T16:19:03Z

/retest-required

 INFO[2023-06-09T05:44:41Z] pod pending for more than 30m0s: containers have not started in 30m0.001030114s: ci-scheduling-dns-wait, place-entrypoint, release, sidecar: 
* Container release is not ready with reason PodInitializing
* Container sidecar is not ready with reason PodInitializing
Found 2 events for Pod release-latest:
* 0x : Successfully assigned ci-op-0xcr7jbh/release-latest to ip-10-0-163-85.ec2.internal
* 6x kubelet: Failed to create pod sandbox: rpc error: code = DeadlineExceeded desc = context deadline exceeded 
INFO[2023-06-09T05:44:41Z] Ran for 1h2m4s                               
ERRO[2023-06-09T05:44:41Z] Some steps failed:                           
ERRO[2023-06-09T05:44:41Z] 
  * could not run steps: step [release:latest] failed: release "release-latest" failed: pod pending for more than 30m0s: containers have not started in 30m0.001030114s: ci-scheduling-dns-wait, place-entrypoint, release, sidecar: 
* Container release is not ready with reason PodInitializing
* Container sidecar is not ready with reason PodInitializing
Found 2 events for Pod release-latest:
* 0x : Successfully assigned ci-op-0xcr7jbh/release-latest to ip-10-0-163-85.ec2.internal
* 6x kubelet: Failed to create pod sandbox: rpc error: code = DeadlineExceeded desc = context deadline exceeded

🤷🏽‍♂️

openshift-ci · 2023-06-09T18:51:25Z

@aravindhp: all tests passed!

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

openshift-ci-robot · 2023-06-09T18:56:15Z

@aravindhp: Jira Issue OCPBUGS-14260: All pull requests linked via external trackers have merged:

openshift/windows-machine-config-operator#1644

Jira Issue OCPBUGS-14260 has been moved to the MODIFIED state.

In response to this:

OCPBUGS-4862 fixed an issue where deletion of a BYOH node object resulted in it hanging in the Ready,SchedulingDisabled state. This was fixed in OCP 4.12.3 and as a result any WMCO upgrades from 7.0.1 needs to be on versions greater than that. Otherwise BYOH node upgrades will fail and could potentially cause workload disruptions. To prevent that from happening stop WMCO 7.1.0 from running if the OCP version is below 4.12.3.

Fixes OCPBUGS-14260

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.

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jun 6, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 6, 2023

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

openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2023

openshift-ci bot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Jun 6, 2023

aravindhp removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Jun 6, 2023

aravindhp force-pushed the OCPBUGS-14260 branch from 37774d8 to 6670efc Compare June 6, 2023 23:18

aravindhp force-pushed the OCPBUGS-14260 branch from 6670efc to 8fe5a54 Compare June 7, 2023 22:08

aravindhp force-pushed the OCPBUGS-14260 branch from 8fe5a54 to f9c71a1 Compare June 7, 2023 22:54

sebsoto requested changes Jun 8, 2023

View reviewed changes

openshift-ci bot changed the title ~~OCPBUGS-14260: [cluster] Check for minimum OCP version~~ OCPBUGS-14260: Check for minimum OCP version Jun 8, 2023

aravindhp force-pushed the OCPBUGS-14260 branch from f9c71a1 to 591aa06 Compare June 8, 2023 16:42

aravindhp commented Jun 8, 2023

View reviewed changes

sebsoto approved these changes Jun 8, 2023

View reviewed changes

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

alinaryan reviewed Jun 8, 2023

View reviewed changes

aravindhp force-pushed the OCPBUGS-14260 branch from 591aa06 to 2a77c4d Compare June 8, 2023 18:31

openshift-ci bot assigned alinaryan Jun 8, 2023

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

aravindhp marked this pull request as ready for review June 8, 2023 19:45

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2023

openshift-ci bot changed the title ~~OCPBUGS-14260: Check for minimum OCP version~~ [release-4.12] OCPBUGS-14260: Check for minimum OCP version Jun 8, 2023

openshift-ci bot requested review from mansikulkarni96 and sebsoto June 8, 2023 19:45

openshift-merge-robot merged commit cfb3eef into openshift:release-4.12 Jun 9, 2023
13 checks passed

aravindhp deleted the OCPBUGS-14260 branch June 9, 2023 22:45

		// minOpenShiftVersion is the minimum required OCP version due to https://issues.redhat.com/browse/OCPBUGS-4862.
		// Without this fix, BYOH node upgrades will fail.

		if (err != nil) != tt.wantErr {
		assert.Errorf(t, err, "error = %v, wantErr %v", err, tt.wantErr)

[release-4.12] OCPBUGS-14260: Check for minimum OCP version #1644

[release-4.12] OCPBUGS-14260: Check for minimum OCP version #1644

Conversation

aravindhp commented Jun 6, 2023 • edited

openshift-ci-robot commented Jun 6, 2023

openshift-ci bot commented Jun 6, 2023

aravindhp commented Jun 6, 2023

openshift-ci-robot commented Jun 6, 2023

aravindhp commented Jun 6, 2023

aravindhp commented Jun 6, 2023

aravindhp commented Jun 6, 2023

openshift-ci-robot commented Jun 6, 2023

aravindhp commented Jun 6, 2023

aravindhp commented Jun 7, 2023

aravindhp commented Jun 7, 2023

aravindhp commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindhp commented Jun 8, 2023

aravindhp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 8, 2023

alinaryan left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 8, 2023

aravindhp commented Jun 8, 2023

alinaryan commented Jun 8, 2023

aravindhp commented Jun 8, 2023

openshift-ci-robot commented Jun 8, 2023

aravindhp commented Jun 9, 2023

aravindhp commented Jun 9, 2023

openshift-ci bot commented Jun 9, 2023

openshift-ci-robot commented Jun 9, 2023

aravindhp commented Jun 6, 2023 •

edited