OCPBUGS-44328: Fix desired before sync_worker's work is initialized by hongkailiu · Pull Request #1104 · openshift/cluster-version-operator

hongkailiu · 2024-11-06T16:30:45Z

This PR is generated by

$ git cherry-pick a411e9da061c6ba1b71bb2b1ce5972a8e0c03307 6906bbfb9d075450e93680cdf8ccfc51d4e62c24 e3a05d439a609e1eb5fc3c63c70a7825ee48f41f a1ee1ca6c050b752c622264c6903823210671334 ae4e180b19aa4f67f2a53785813ff040ee1690da

and then fix the backported unit test TestCVO_UpgradePayloadStillInitializing by the following editting:

git --no-pager diff 
diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go
index c195e26c..9f060195 100644
--- a/pkg/cvo/cvo_scenarios_test.go
+++ b/pkg/cvo/cvo_scenarios_test.go
@@ -33,9 +33,18 @@ import (
 )
 
 var architecture string
+var sortedCaps = configv1.ClusterVersionCapabilitySets[configv1.ClusterVersionCapabilitySetCurrent]
+var sortedKnownCaps = configv1.KnownClusterVersionCapabilities
 
 func init() {
        architecture = runtime.GOARCH
+
+       sort.Slice(sortedCaps, func(i, j int) bool {
+               return sortedCaps[i] < sortedCaps[j]
+       })
+       sort.Slice(sortedKnownCaps, func(i, j int) bool {
+               return sortedKnownCaps[i] < sortedKnownCaps[j]
+       })
 }
 
 func setupCVOTest(payloadDir string) (*Operator, map[string]apiruntime.Object, *fake.Clientset, *dynamicfake.FakeDynamicClient, func()) {
diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go
index 22ec05b9..eb2fb500 100644
--- a/pkg/cvo/sync_worker.go
+++ b/pkg/cvo/sync_worker.go
@@ -191,11 +191,6 @@ type SyncWorker struct {
 
        clusterProfile string
 
-       // alwaysEnableCapabilities is a list of cluster capabilities which should
-       // always be implicitly enabled.
-       // This contributes to whether or not some manifests are included for reconciliation.
-       alwaysEnableCapabilities []configv1.ClusterVersionCapability
-
        // initializedFunc is only for the unit-test purpose
        initializedFunc func() bool
 }

where alwaysEnableCapabilities is removed because it is not used in 4.15 and the variables sortedCaps and sortedKnownCaps are added because they are used in the backported unit test.

Note that e94580f is not picked to keep the manual work simple. Otherwise it would lead to more manual work such as fixing using constants from the new API like configv1.ClusterVersionCapabilityIngress which is not available for 4.15.

If the CVO pod gets restarted after a user sends a cluster upgrade request with `--to-image`, the desired should not ALWAYS be taken from `.Spec.DesiredUpdate`, e.g., when the targeting payload is blocked on some precondition check failure. The user request will be written to `cv.spec.desiredUpdate` where only the `image` field is set, i.e., the `Version` field is empty. If the CVO gets restarted, then 1. The desied [1] from `cv.spec.desiredUpdate` will be used to set `w.status.Actual` [2] and then applied to `cv.status.desired`. 2. When the next `optr.sync()` happens, the precondition check may become non-blocking [3] as the `Version` field is empty now. 3. CVO will then start an upgrade that should have been blocked. [1]. https://github.com/openshift/cluster-version-operator/blob/1995380b6d755c29b926b846a64ca0039002c2cf/pkg/cvo/cvo.go#L680 [2]. https://github.com/openshift/cluster-version-operator/blob/1995380b6d755c29b926b846a64ca0039002c2cf/pkg/cvo/sync_worker.go#L502 [3]. https://github.com/openshift/cluster-version-operator/blob/1995380b6d755c29b926b846a64ca0039002c2cf/pkg/payload/precondition/clusterversion/rollback.go#L57

hongkailiu · 2024-11-06T17:32:55Z

/test-required

hongkailiu · 2024-11-06T18:23:30Z

/test unit

hongkailiu · 2024-11-07T01:31:30Z

/retest-required

openshift-ci · 2024-11-07T03:18:43Z

@hongkailiu: all tests passed!

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

hongkailiu · 2024-11-07T14:09:59Z

/jira cherrypick OCPBUGS-43964

openshift-ci-robot · 2024-11-07T14:11:12Z

@hongkailiu: Jira Issue OCPBUGS-43964 has been cloned as Jira Issue OCPBUGS-44328. Will retitle bug to link to clone.
/retitle OCPBUGS-44328: [wip][release-4.15] OCPBUGS-TODO: Fix desired before sync_worker's work is initialized

Details

In response to this:

/jira cherrypick OCPBUGS-43964

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.

openshift-ci-robot · 2024-11-07T14:11:23Z

@hongkailiu: This pull request references Jira Issue OCPBUGS-44328, which is valid. The bug has been moved to the POST state.

7 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.z) matches configured target version for branch (4.15.z)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)
release note text is set and does not match the template
dependent bug Jira Issue OCPBUGS-43964 is in the state Verified, which is one of the valid states (VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA))
dependent Jira Issue OCPBUGS-43964 targets the "4.16.z" version, which is one of the valid target versions: 4.16.0, 4.16.z
bug has dependents

Requesting review from QA contact:
/cc @jiajliu

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

Details

In response to this:

This PR is generated by

$ git cherry-pick a411e9da061c6ba1b71bb2b1ce5972a8e0c03307 6906bbfb9d075450e93680cdf8ccfc51d4e62c24 e3a05d439a609e1eb5fc3c63c70a7825ee48f41f a1ee1ca6c050b752c622264c6903823210671334 ae4e180b19aa4f67f2a53785813ff040ee1690da

and then fix the backported unit test TestCVO_UpgradePayloadStillInitializing by the following editting:

git --no-pager diff 
diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go
index c195e26c..9f060195 100644
--- a/pkg/cvo/cvo_scenarios_test.go
+++ b/pkg/cvo/cvo_scenarios_test.go
@@ -33,9 +33,18 @@ import (
)

var architecture string
+var sortedCaps = configv1.ClusterVersionCapabilitySets[configv1.ClusterVersionCapabilitySetCurrent]
+var sortedKnownCaps = configv1.KnownClusterVersionCapabilities

func init() {
       architecture = runtime.GOARCH
+
+       sort.Slice(sortedCaps, func(i, j int) bool {
+               return sortedCaps[i] < sortedCaps[j]
+       })
+       sort.Slice(sortedKnownCaps, func(i, j int) bool {
+               return sortedKnownCaps[i] < sortedKnownCaps[j]
+       })
}

func setupCVOTest(payloadDir string) (*Operator, map[string]apiruntime.Object, *fake.Clientset, *dynamicfake.FakeDynamicClient, func()) {
diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go
index 22ec05b9..eb2fb500 100644
--- a/pkg/cvo/sync_worker.go
+++ b/pkg/cvo/sync_worker.go
@@ -191,11 +191,6 @@ type SyncWorker struct {

       clusterProfile string

-       // alwaysEnableCapabilities is a list of cluster capabilities which should
-       // always be implicitly enabled.
-       // This contributes to whether or not some manifests are included for reconciliation.
-       alwaysEnableCapabilities []configv1.ClusterVersionCapability
-
       // initializedFunc is only for the unit-test purpose
       initializedFunc func() bool
}

where alwaysEnableCapabilities is removed because it is not used in 4.15 and the variables sortedCaps and sortedKnownCaps are added because they are used in the backported unit test.

Note that e94580f is not picked to keep the manual work simple. Otherwise it would lead to more manual work such as fixing using constants from the new API like configv1.ClusterVersionCapabilityIngress.

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.

hongkailiu · 2024-11-07T14:13:19Z

/jira refresh

openshift-ci-robot · 2024-11-07T14:13:23Z

@hongkailiu: This pull request references Jira Issue OCPBUGS-44328, which is valid.

7 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.z) matches configured target version for branch (4.15.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
release note text is set and does not match the template
dependent bug Jira Issue OCPBUGS-43964 is in the state Verified, which is one of the valid states (VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA))
dependent Jira Issue OCPBUGS-43964 targets the "4.16.z" version, which is one of the valid target versions: 4.16.0, 4.16.z
bug has dependents

Requesting review from QA contact:
/cc @jiajliu

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

Details

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 openshift-eng/jira-lifecycle-plugin repository.

wking · 2024-11-09T17:11:17Z

/retitle OCPBUGS-44328: Fix desired before sync_worker's work is initialized

wking

/lgtm
/label backport-risk-assessed

openshift-ci · 2024-11-09T17:13:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hongkailiu, 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:

~~OWNERS~~ [wking]

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

evakhoni · 2024-11-10T17:51:41Z

so the only change outside of the unit test, is the alwaysEnableCapabilities in sync worker, which anyway weren't there in 4.15 right? if such, no real change in behavior compared to the parent fix right?

hongkailiu · 2024-11-12T16:45:58Z

so the only change outside of the unit test, is the alwaysEnableCapabilities in sync worker, which anyway weren't there in 4.15 right? if such, no real change in behavior compared to the parent fix right?

Correct and correct.

evakhoni · 2024-11-18T15:45:14Z

ok then, verified.
/label qe-approved
/cc @jiajliu
for approval tnx

openshift-ci-robot · 2024-11-18T15:47:08Z

@hongkailiu: This pull request references Jira Issue OCPBUGS-44328, which is valid.

7 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.z) matches configured target version for branch (4.15.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
release note text is set and does not match the template
dependent bug Jira Issue OCPBUGS-43964 is in the state Closed (Done-Errata), which is one of the valid states (VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA))
dependent Jira Issue OCPBUGS-43964 targets the "4.16.z" version, which is one of the valid target versions: 4.16.0, 4.16.z
bug has dependents

Requesting review from QA contact:
/cc @evakhoni

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

Details

In response to this:

This PR is generated by

$ git cherry-pick a411e9da061c6ba1b71bb2b1ce5972a8e0c03307 6906bbfb9d075450e93680cdf8ccfc51d4e62c24 e3a05d439a609e1eb5fc3c63c70a7825ee48f41f a1ee1ca6c050b752c622264c6903823210671334 ae4e180b19aa4f67f2a53785813ff040ee1690da

and then fix the backported unit test TestCVO_UpgradePayloadStillInitializing by the following editting:

git --no-pager diff 
diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go
index c195e26c..9f060195 100644
--- a/pkg/cvo/cvo_scenarios_test.go
+++ b/pkg/cvo/cvo_scenarios_test.go
@@ -33,9 +33,18 @@ import (
)

var architecture string
+var sortedCaps = configv1.ClusterVersionCapabilitySets[configv1.ClusterVersionCapabilitySetCurrent]
+var sortedKnownCaps = configv1.KnownClusterVersionCapabilities

func init() {
       architecture = runtime.GOARCH
+
+       sort.Slice(sortedCaps, func(i, j int) bool {
+               return sortedCaps[i] < sortedCaps[j]
+       })
+       sort.Slice(sortedKnownCaps, func(i, j int) bool {
+               return sortedKnownCaps[i] < sortedKnownCaps[j]
+       })
}

func setupCVOTest(payloadDir string) (*Operator, map[string]apiruntime.Object, *fake.Clientset, *dynamicfake.FakeDynamicClient, func()) {
diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go
index 22ec05b9..eb2fb500 100644
--- a/pkg/cvo/sync_worker.go
+++ b/pkg/cvo/sync_worker.go
@@ -191,11 +191,6 @@ type SyncWorker struct {

       clusterProfile string

-       // alwaysEnableCapabilities is a list of cluster capabilities which should
-       // always be implicitly enabled.
-       // This contributes to whether or not some manifests are included for reconciliation.
-       alwaysEnableCapabilities []configv1.ClusterVersionCapability
-
       // initializedFunc is only for the unit-test purpose
       initializedFunc func() bool
}

where alwaysEnableCapabilities is removed because it is not used in 4.15 and the variables sortedCaps and sortedKnownCaps are added because they are used in the backported unit test.

Note that e94580f is not picked to keep the manual work simple. Otherwise it would lead to more manual work such as fixing using constants from the new API like configv1.ClusterVersionCapabilityIngress which is not available for 4.15.

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.

hongkailiu · 2024-11-18T16:00:00Z

/cherrypick release-4.14

openshift-cherrypick-robot · 2024-11-18T16:00:03Z

@hongkailiu: once the present PR merges, I will cherry-pick it on top of release-4.14 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-4.14

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.

jiajliu · 2024-11-19T00:37:45Z

/label cherry-pick-approved

openshift-ci-robot · 2024-11-19T00:42:04Z

@hongkailiu: Jira Issue OCPBUGS-44328: All pull requests linked via external trackers have merged:

openshift/cluster-version-operator#1104

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

Details

In response to this:

This PR is generated by

$ git cherry-pick a411e9da061c6ba1b71bb2b1ce5972a8e0c03307 6906bbfb9d075450e93680cdf8ccfc51d4e62c24 e3a05d439a609e1eb5fc3c63c70a7825ee48f41f a1ee1ca6c050b752c622264c6903823210671334 ae4e180b19aa4f67f2a53785813ff040ee1690da

and then fix the backported unit test TestCVO_UpgradePayloadStillInitializing by the following editting:

git --no-pager diff 
diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go
index c195e26c..9f060195 100644
--- a/pkg/cvo/cvo_scenarios_test.go
+++ b/pkg/cvo/cvo_scenarios_test.go
@@ -33,9 +33,18 @@ import (
)

var architecture string
+var sortedCaps = configv1.ClusterVersionCapabilitySets[configv1.ClusterVersionCapabilitySetCurrent]
+var sortedKnownCaps = configv1.KnownClusterVersionCapabilities

func init() {
       architecture = runtime.GOARCH
+
+       sort.Slice(sortedCaps, func(i, j int) bool {
+               return sortedCaps[i] < sortedCaps[j]
+       })
+       sort.Slice(sortedKnownCaps, func(i, j int) bool {
+               return sortedKnownCaps[i] < sortedKnownCaps[j]
+       })
}

func setupCVOTest(payloadDir string) (*Operator, map[string]apiruntime.Object, *fake.Clientset, *dynamicfake.FakeDynamicClient, func()) {
diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go
index 22ec05b9..eb2fb500 100644
--- a/pkg/cvo/sync_worker.go
+++ b/pkg/cvo/sync_worker.go
@@ -191,11 +191,6 @@ type SyncWorker struct {

       clusterProfile string

-       // alwaysEnableCapabilities is a list of cluster capabilities which should
-       // always be implicitly enabled.
-       // This contributes to whether or not some manifests are included for reconciliation.
-       alwaysEnableCapabilities []configv1.ClusterVersionCapability
-
       // initializedFunc is only for the unit-test purpose
       initializedFunc func() bool
}

where alwaysEnableCapabilities is removed because it is not used in 4.15 and the variables sortedCaps and sortedKnownCaps are added because they are used in the backported unit test.

Note that e94580f is not picked to keep the manual work simple. Otherwise it would lead to more manual work such as fixing using constants from the new API like configv1.ClusterVersionCapabilityIngress which is not available for 4.15.

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.

openshift-cherrypick-robot · 2024-11-19T00:42:50Z

@hongkailiu: new pull request created: #1109

Details

In response to this:

/cherrypick release-4.14

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.

openshift-bot · 2024-11-19T01:33:00Z

[ART PR BUILD NOTIFIER]

Distgit: cluster-version-operator
This PR has been included in build cluster-version-operator-container-v4.15.0-202411190104.p0.gfbb41e8.assembly.stream.el9.
All builds following this will include this PR.

openshift-merge-robot · 2024-11-19T08:36:44Z

Fix included in accepted release 4.15.0-0.nightly-2024-11-19-015320

hongkailiu added 6 commits November 6, 2024 10:54

Fix unit tests

d64e3a2

Add test to cover the case of stillInitializing == true

d46e2b9

Better naming of variables and functions

1319338

Improve some wording and simplify unit tests

b22036b

Fix the backported unit test

e8258e0

hongkailiu changed the title ~~Bk 415~~ [wip][release-4.15] OCPBUGS-TODO: Fix desired before sync_worker's work is initialized Nov 6, 2024

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 Nov 6, 2024

openshift-ci Bot requested review from LalatenduMohanty and petr-muller November 6, 2024 16:31

openshift-ci Bot changed the title ~~[wip][release-4.15] OCPBUGS-TODO: Fix desired before sync_worker's work is initialized~~ OCPBUGS-44328: [wip][release-4.15] OCPBUGS-TODO: Fix desired before sync_worker's work is initialized Nov 7, 2024

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 Nov 7, 2024

openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Nov 7, 2024

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Nov 7, 2024

openshift-ci Bot requested a review from jiajliu November 7, 2024 14:11

hongkailiu changed the title ~~OCPBUGS-44328: [wip][release-4.15] OCPBUGS-TODO: Fix desired before sync_worker's work is initialized~~ OCPBUGS-44328: [release-4.15] OCPBUGS-TODO: Fix desired before sync_worker's work is initialized Nov 7, 2024

openshift-ci Bot changed the title ~~OCPBUGS-44328: [release-4.15] OCPBUGS-TODO: Fix desired before sync_worker's work is initialized~~ OCPBUGS-44328: Fix desired before sync_worker's work is initialized Nov 9, 2024

wking approved these changes Nov 9, 2024

View reviewed changes

openshift-ci Bot added the backport-risk-assessed Indicates a PR to a release branch has been evaluated and considered safe to accept. label Nov 9, 2024

openshift-ci Bot assigned wking Nov 9, 2024

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2024

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2024

openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label Nov 18, 2024

openshift-ci Bot requested a review from evakhoni November 18, 2024 15:47

openshift-ci Bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Nov 19, 2024

openshift-merge-bot Bot merged commit fbb41e8 into openshift:release-4.15 Nov 19, 2024

openshift-cherrypick-robot mentioned this pull request Nov 19, 2024

[release-4.14] OCPBUGS-44704: Fix desired before sync_worker's work is initialized #1109

Merged

Uh oh!

Conversation

hongkailiu commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hongkailiu commented Nov 6, 2024

Uh oh!

hongkailiu commented Nov 6, 2024

Uh oh!

hongkailiu commented Nov 7, 2024

Uh oh!

openshift-ci Bot commented Nov 7, 2024

Uh oh!

hongkailiu commented Nov 7, 2024

Uh oh!

openshift-ci-robot commented Nov 7, 2024

Uh oh!

openshift-ci-robot commented Nov 7, 2024

Uh oh!

hongkailiu commented Nov 7, 2024

Uh oh!

openshift-ci-robot commented Nov 7, 2024

Uh oh!

wking commented Nov 9, 2024

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Nov 9, 2024

Uh oh!

evakhoni commented Nov 10, 2024

Uh oh!

hongkailiu commented Nov 12, 2024

Uh oh!

evakhoni commented Nov 18, 2024

Uh oh!

openshift-ci-robot commented Nov 18, 2024

Uh oh!

hongkailiu commented Nov 18, 2024

Uh oh!

openshift-cherrypick-robot commented Nov 18, 2024

Uh oh!

jiajliu commented Nov 19, 2024

Uh oh!

openshift-ci-robot commented Nov 19, 2024

Uh oh!

openshift-cherrypick-robot commented Nov 19, 2024

Uh oh!

openshift-bot commented Nov 19, 2024

Uh oh!

openshift-merge-robot commented Nov 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hongkailiu commented Nov 6, 2024 •

edited

Loading