OTA-209: Add sync logic to the CVO configuration controller #1170
Conversation
This file contains 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
These constants can be simply used when logging across the project instead
of specifying the level by a number. The descriptions and the values will
also guide developers when deciding what log level to choose.
For example:
```go
import "github.com/openshift/cluster-version-operator/pkg/internal"
klog.V(internal.Debug).Infof("")
```
The descriptions of the values are extracted from the used API type [1].
[1]: https://github.com/openshift/api/blob/744790f2cff777b1bb29bdccce10e4e84cff0a69/operator/v1/types.go#L94-L109
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
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DavidHurta 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 |
openshift-ci
bot
added
the
approved
|
/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
|
@DavidHurta: 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. |
|
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
| @@ -83,6 +94,13 @@ func (config *ClusterVersionOperatorConfiguration) Start(ctx context.Context) er | |||
| } | |||
| } | |||
|
|
|||
| if currentLogLevel, notFound := loglevel.GetLogLevel(); notFound { | |||
There was a problem hiding this comment.
Could we do it when the config instance was initialized?
cluster-version-operator/pkg/cvo/configuration/configuration.go
Lines 69 to 78 in 3aa8e04
| desiredConfig.Status.ObservedGeneration = desiredConfig.Generation | ||
| _, err := config.client.UpdateStatus(ctx, desiredConfig, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to update the ClusterVersionOperator resource, err=%w", err) |
There was a problem hiding this comment.
Suggested change
| return fmt.Errorf("Failed to update the ClusterVersionOperator resource, err=%w", err) | |
| return fmt.Errorf("failed to update the ClusterVersionOperator resource: %w", err) |
| @@ -43,12 +51,15 @@ func (config *ClusterVersionOperatorConfiguration) clusterVersionOperatorEventHa | |||
| return cache.ResourceEventHandlerFuncs{ | |||
| AddFunc: func(_ interface{}) { | |||
| config.queue.Add(config.queueKey) | |||
| klog.V(i.Debug).Infof("ClusterVersionOperator resource was added; queuing a configuration sync") | |||
There was a problem hiding this comment.
nit: this sounds more natural to me.
Suggested change
| klog.V(i.Debug).Infof("ClusterVersionOperator resource was added; queuing a configuration sync") | |
| klog.V(i.Debug).Infof("ClusterVersionOperator resource was added; queuing a sync") |
| klog.V(i.Debug).Infof("No need to update the current CVO log level '%s'; it is already set to the desired value", currentLogLevel) | ||
| } else { | ||
| if err := loglevel.SetLogLevel(config.desiredLogLevel); err != nil { | ||
| return fmt.Errorf("Failed to set the log level to '%s', err=%w", config.desiredLogLevel, err) |
There was a problem hiding this comment.
Suggested change
| return fmt.Errorf("Failed to set the log level to '%s', err=%w", config.desiredLogLevel, err) | |
| return fmt.Errorf("failed to set the log level to %q: %w", config.desiredLogLevel, err) |
| } | ||
| klog.V(i.Debug).Infof("Successfully updated the log level from '%s' to '%s'", currentLogLevel, config.desiredLogLevel) | ||
|
|
||
| // E2E testing will be checking for existence or absence of these logs |
There was a problem hiding this comment.
Is it temporary?
I would not want our users to see those in logging otherwise.
I am thinking to output those line with a guarding from an environment variable that is set where the test runs such as
if os.Getenv("CI")=="true" {
...
}WDYT?
There was a problem hiding this comment.
I would not want our users to see those in logging otherwise.
I actually think we should have the log - if we expose log level configuration in user-facing resource (and expect user to interact with it), then IMO it makes sense for CVO to acknowledge it in its log? Its not even a debug log, it is straight "I saw user to do something and now I behave differently" - that sounds useful to me?
There was a problem hiding this comment.
I think that Petr may have the impression that the comment is regarding the line 151. And not the following lines, which are a bit clunky:
klog.V(i.Normal).Infof("The CVO logging level is set to the 'Normal' log level or above")
klog.V(i.Debug).Infof("The CVO logging level is set to the 'Debug' log level or above")
klog.V(i.Trace).Infof("The CVO logging level is set to the 'Trace' log level or above")
klog.V(i.TraceAll).Infof("The CVO logging level is set to the 'TraceAll' log level or above")I think that the usage of the CI environment variable here is a great idea. I forgot about it.
There was a problem hiding this comment.
My comment is about the logs for e2e test as David clarified. :)
I am trying to avoid showing logs that are only for our own testing (if possible at all).
Comment on lines
+259
to
+268
| switch { | ||
| case apierrors.IsNotFound(err) && tt.expectedConfig == nil: | ||
| case apierrors.IsNotFound(err) && tt.expectedConfig != nil: | ||
| t.Errorf("expected config to be '%v', got NotFound", *tt.expectedConfig) | ||
| case config != nil && tt.expectedConfig == nil: | ||
| t.Errorf("expected config to be NotFound, got '%v'", *config) | ||
| case config != nil && tt.expectedConfig != nil: | ||
| if diff := cmp.Diff(*tt.expectedConfig, *config, cmpopts.IgnoreFields(operatorv1alpha1.ClusterVersionOperator{}, "ObjectMeta")); diff != "" { | ||
| t.Errorf("unexpected config (-want, +got) = %v", diff) | ||
| } |
There was a problem hiding this comment.
Just choose any part that you like:
Suggested change
| switch { | |
| case apierrors.IsNotFound(err) && tt.expectedConfig == nil: | |
| case apierrors.IsNotFound(err) && tt.expectedConfig != nil: | |
| t.Errorf("expected config to be '%v', got NotFound", *tt.expectedConfig) | |
| case config != nil && tt.expectedConfig == nil: | |
| t.Errorf("expected config to be NotFound, got '%v'", *config) | |
| case config != nil && tt.expectedConfig != nil: | |
| if diff := cmp.Diff(*tt.expectedConfig, *config, cmpopts.IgnoreFields(operatorv1alpha1.ClusterVersionOperator{}, "ObjectMeta")); diff != "" { | |
| t.Errorf("unexpected config (-want, +got) = %v", diff) | |
| } | |
| switch { | |
| case apierrors.IsNotFound(err) && tt.expectedConfig != nil: | |
| t.Errorf("expected config to be '%v', got NotFound", *tt.expectedConfig) | |
| case err == nil: | |
| if diff := cmp.Diff(tt.expectedConfig, config, cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ManagedFields")); diff != "" { | |
| t.Errorf("unexpected config (-want, +got) = %v", diff) | |
| } | |
| } |
| if err != nil { | ||
| t.Errorf("unexpected error %v", err) | ||
| } | ||
| if diff := cmp.Diff(tt.expectedConfig, *config, cmpopts.IgnoreFields(operatorv1alpha1.ClusterVersionOperator{}, "ObjectMeta")); diff != "" { |
There was a problem hiding this comment.
nit: ignore the necessary only?
Suggested change
| if diff := cmp.Diff(tt.expectedConfig, *config, cmpopts.IgnoreFields(operatorv1alpha1.ClusterVersionOperator{}, "ObjectMeta")); diff != "" { | |
| if diff := cmp.Diff(tt.expectedConfig, *config, cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ManagedFields")); diff != "" { |
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
| desiredConfig.Status.ObservedGeneration = desiredConfig.Generation | ||
| _, err := config.client.UpdateStatus(ctx, desiredConfig, metav1.UpdateOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to update the ClusterVersionOperator resource, err=%w", err) |
| 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).
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.