OTA-209: Add sync logic to the CVO configuration controller #1170
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
the
jira/valid-reference
|
@DavidHurta: This pull request references OTA-209 which is a valid jira issue. 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. |
openshift-ci
bot
added
do-not-merge/work-in-progress
|
/retest-required |
|
Manually testing on a Default feature set:Checking for occurrences of relevant strings. All as expected. Checking for warnings. The majority is caused by the Checking for errors. No relevant errors to the PR. No relevant resources to be found as expected: DevPreviewNoUpgrade feature set:Set the feature set to Checking for occurrences of relevant strings (the desired log level was changed manually multiple times as can be seen in the logs). All as expected. Checking for warnings. The majority is caused by the Checking for errors. All as expected. Summary:All is as expected. The warnings are caused by the |
|
@DavidHurta: This pull request references OTA-209 which is a valid jira issue. 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. |
DavidHurta
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cluster-version-operator/1170/pull-ci-openshift-cluster-version-operator-main-e2e-agnostic-ovn/1902012244305121280 does not seem to be caused by the PR. |
|
/cc |
|
/cc |
hongkailiu
reviewed
Mar 19, 2025
petr-muller
reviewed
Mar 21, 2025
| } | ||
|
|
||
| // Shutdown created resources | ||
| cancelFunc() |
There was a problem hiding this comment.
nit: why not defer immediately after creation? it's idiomatic and will survive eg someone adding an early return or something
There was a problem hiding this comment.
Done in 26e1ac3
No specific reason. A habit from different languages. I'll ensure being more idiomatic in the future 👍
There was a problem hiding this comment.
Improved the structure of the calls in e630eec
| klog.Infof("ClusterVersionOperator configuration has been synced") | ||
| func (config *ClusterVersionOperatorConfiguration) sync(ctx context.Context, desiredConfig *operatorv1alpha1.ClusterVersionOperator) error { | ||
| if desiredConfig.Status.ObservedGeneration != desiredConfig.Generation { | ||
| desiredConfig.Status.ObservedGeneration = desiredConfig.Generation |
There was a problem hiding this comment.
We need to be careful about modifying objects in the informer cache, which you must never modify:
Basically whatever you get from the informer cache (= anything from the lister), you must not modify. Typically you make a deepcopy, and modify that. It is good practice to document whether pointers point to modifiable data or not in the function signatures (so sync would tell "you must not modify desiredConfig, it poitns to informer cache).
There was a problem hiding this comment.
There was a problem hiding this comment.
NIT: I would do it like this:
1 keep return config.sync(ctx, desiredConfig).
2. make a copy only when needed:
config.sync(ctx, desiredConfig) {
if desiredConfig.Status.ObservedGeneration != desiredConfig.Generation {
dup := desiredConfig.DeepCopy()
// then change the dup and update with it
}
// afterwards, we still use desiredConfig for the read only actions
}
|
/lgtm /hold To give Petr a chance to take a look, or I convince you to remove Otherwise, feel free to cancel. |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest-required |
|
/hold |
|
/test e2e-agnostic-operator-devpreview |
|
/retest |
|
LGTM. I guess you still want to squash. |
DavidHurta
force-pushed
the
cvo-configuration-controller-add-sync-logic
branch
from
0f5d0d7 to
49c5a74
Compare
|
/unhold |
DavidHurta
force-pushed
the
cvo-configuration-controller-add-sync-logic
branch
from
49c5a74 to
c4d7cd3
Compare
|
/remove-label no-qe |
|
/retest |
1 similar comment
|
/retest |
petr-muller
approved these changes
Apr 17, 2025
|
/retest |
A desired log level in the ClusterVersionOperator object can have an empty string value. Respect it, and treat it as the Normal level.
|
@DavidHurta: The following test 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. |
petr-muller
approved these changes
May 2, 2025
|
/skip |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DavidHurta, hongkailiu, petr-muller 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:
Approvers can indicate their approval by writing |
|
/unhold I am confident that the latest commit addressed the https://issues.redhat.com/browse/OTA-209?focusedId=27066665&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-27066665 QE verification. @JianLi-RH, based on your evaluation, feel free to approve the PR or verify that the feedback was addressed. |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@DavidHurta: This pull request references OTA-209 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 either version "4.20." or "openshift-4.20.", but it targets "openshift-4.19" instead. 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. |
|
/override ci/prow/e2e-agnostic-ovn-upgrade-out-of-change There's just a single unrelated failure |
|
@petr-muller: Overrode contexts on behalf of petr-muller: ci/prow/e2e-agnostic-ovn-upgrade-out-of-change 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. |
|
|
|
[ART PR BUILD NOTIFIER] Distgit: cluster-version-operator |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add synchronization logic to the CVO configuration controller to change the CVO's log level depending on the resource's spec and update the
observedGenerationfield of the resource.