diff --git a/pkg/cvo/configuration/configuration.go b/pkg/cvo/configuration/configuration.go index 66782421e..947284bae 100644 --- a/pkg/cvo/configuration/configuration.go +++ b/pkg/cvo/configuration/configuration.go @@ -24,6 +24,11 @@ import ( const ClusterVersionOperatorConfigurationName = "cluster" +type configuration struct { + lastObservedGeneration int64 + desiredLogLevel operatorv1.LogLevel +} + type ClusterVersionOperatorConfiguration struct { queueKey string // queue tracks checking for the CVO configuration. @@ -37,8 +42,15 @@ type ClusterVersionOperatorConfiguration struct { started bool - desiredLogLevel operatorv1.LogLevel - lastObservedGeneration int64 + configuration configuration + + // Function to handle an update to the internal configuration. + // + // In the future, the desired configuration may be consumed via other controllers. + // This will require some sort of synchronization upon a configuration change. + // For the moment, the log level is the sole consumer of the configuration. + // Apply the log level directly here for simplicity for the time being. + handler func(configuration) error } func (config *ClusterVersionOperatorConfiguration) Queue() workqueue.TypedRateLimitingInterface[any] { @@ -80,9 +92,10 @@ func NewClusterVersionOperatorConfiguration(client operatorclientset.Interface, queue: workqueue.NewTypedRateLimitingQueueWithConfig[any]( workqueue.DefaultTypedControllerRateLimiter[any](), workqueue.TypedRateLimitingQueueConfig[any]{Name: "configuration"}), - client: client.OperatorV1alpha1().ClusterVersionOperators(), - factory: factory, - desiredLogLevel: desiredLogLevel, + client: client.OperatorV1alpha1().ClusterVersionOperators(), + factory: factory, + configuration: configuration{desiredLogLevel: desiredLogLevel}, + handler: func(c configuration) error { return applyLogLevel(c.desiredLogLevel) }, } } @@ -142,37 +155,14 @@ func (config *ClusterVersionOperatorConfiguration) sync(ctx context.Context, des } } - config.lastObservedGeneration = desiredConfig.Generation - config.desiredLogLevel = desiredConfig.Spec.OperatorLogLevel - if config.desiredLogLevel == "" { - config.desiredLogLevel = operatorv1.Normal - } - - currentLogLevel, notFound := loglevel.GetLogLevel() - if notFound { - klog.Warningf("The current log level could not be found; an attempt to set the log level to the desired level will be made") + config.configuration.lastObservedGeneration = desiredConfig.Generation + config.configuration.desiredLogLevel = desiredConfig.Spec.OperatorLogLevel + if config.configuration.desiredLogLevel == "" { + config.configuration.desiredLogLevel = operatorv1.Normal } - if !notFound && currentLogLevel == config.desiredLogLevel { - 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 %q: %w", config.desiredLogLevel, err) - } - - // E2E testing will be checking for existence or absence of these logs - switch config.desiredLogLevel { - case operatorv1.Normal: - klog.V(i.Normal).Infof("Successfully updated the log level from '%s' to 'Normal'", currentLogLevel) - case operatorv1.Debug: - klog.V(i.Debug).Infof("Successfully updated the log level from '%s' to 'Debug'", currentLogLevel) - case operatorv1.Trace: - klog.V(i.Trace).Infof("Successfully updated the log level from '%s' to 'Trace'", currentLogLevel) - case operatorv1.TraceAll: - klog.V(i.TraceAll).Infof("Successfully updated the log level from '%s' to 'TraceAll'", currentLogLevel) - default: - klog.Errorf("The CVO logging level has unexpected value '%s'", config.desiredLogLevel) - } + if config.handler != nil { + return config.handler(config.configuration) } return nil } diff --git a/pkg/cvo/configuration/configuration_test.go b/pkg/cvo/configuration/configuration_test.go index 8ed7602b5..eb5517097 100644 --- a/pkg/cvo/configuration/configuration_test.go +++ b/pkg/cvo/configuration/configuration_test.go @@ -17,17 +17,33 @@ import ( operatorexternalversions "github.com/openshift/client-go/operator/informers/externalversions" ) -func TestClusterVersionOperatorConfiguration_sync(t *testing.T) { +func TestClusterVersionOperatorConfiguration_APIServerSync(t *testing.T) { tests := []struct { name string - config operatorv1alpha1.ClusterVersionOperator - expectedConfig operatorv1alpha1.ClusterVersionOperator - internalConfig ClusterVersionOperatorConfiguration - expectedInternalConfig ClusterVersionOperatorConfiguration + config *operatorv1alpha1.ClusterVersionOperator + expectedConfig *operatorv1alpha1.ClusterVersionOperator + internalConfig configuration + expectedInternalConfig configuration + handlerFunctionCalled bool }{ + { + name: "the configuration resource does not exist in the cluster -> default configuration", + config: nil, + expectedConfig: nil, + internalConfig: configuration{ + lastObservedGeneration: 3, + desiredLogLevel: "Trace", + }, + expectedInternalConfig: configuration{ + lastObservedGeneration: 3, // TODO: Default to 0 + desiredLogLevel: "Trace", // TODO: Default to Normal + }, + // TODO: Apply the log level when the defaulting is implemented + handlerFunctionCalled: false, + }, { name: "first sync run correctly updates the status", - config: operatorv1alpha1.ClusterVersionOperator{ + config: &operatorv1alpha1.ClusterVersionOperator{ ObjectMeta: metav1.ObjectMeta{ Generation: 1, }, @@ -35,7 +51,7 @@ func TestClusterVersionOperatorConfiguration_sync(t *testing.T) { OperatorLogLevel: operatorv1.Normal, }, }, - expectedConfig: operatorv1alpha1.ClusterVersionOperator{ + expectedConfig: &operatorv1alpha1.ClusterVersionOperator{ ObjectMeta: metav1.ObjectMeta{ Generation: 1, }, @@ -46,18 +62,19 @@ func TestClusterVersionOperatorConfiguration_sync(t *testing.T) { ObservedGeneration: 1, }, }, - internalConfig: ClusterVersionOperatorConfiguration{ + internalConfig: configuration{ desiredLogLevel: operatorv1.Normal, lastObservedGeneration: 0, }, - expectedInternalConfig: ClusterVersionOperatorConfiguration{ + expectedInternalConfig: configuration{ desiredLogLevel: operatorv1.Normal, lastObservedGeneration: 1, }, + handlerFunctionCalled: true, }, { name: "sync updates observed generation correctly", - config: operatorv1alpha1.ClusterVersionOperator{ + config: &operatorv1alpha1.ClusterVersionOperator{ ObjectMeta: metav1.ObjectMeta{ Generation: 3, }, @@ -68,7 +85,7 @@ func TestClusterVersionOperatorConfiguration_sync(t *testing.T) { ObservedGeneration: 2, }, }, - expectedConfig: operatorv1alpha1.ClusterVersionOperator{ + expectedConfig: &operatorv1alpha1.ClusterVersionOperator{ ObjectMeta: metav1.ObjectMeta{ Generation: 3, }, @@ -79,18 +96,19 @@ func TestClusterVersionOperatorConfiguration_sync(t *testing.T) { ObservedGeneration: 3, }, }, - internalConfig: ClusterVersionOperatorConfiguration{ + internalConfig: configuration{ desiredLogLevel: operatorv1.Normal, lastObservedGeneration: 2, }, - expectedInternalConfig: ClusterVersionOperatorConfiguration{ + expectedInternalConfig: configuration{ desiredLogLevel: operatorv1.Normal, lastObservedGeneration: 3, }, + handlerFunctionCalled: true, }, { name: "sync updates desired log level correctly", - config: operatorv1alpha1.ClusterVersionOperator{ + config: &operatorv1alpha1.ClusterVersionOperator{ ObjectMeta: metav1.ObjectMeta{ Generation: 4, }, @@ -101,7 +119,7 @@ func TestClusterVersionOperatorConfiguration_sync(t *testing.T) { ObservedGeneration: 3, }, }, - expectedConfig: operatorv1alpha1.ClusterVersionOperator{ + expectedConfig: &operatorv1alpha1.ClusterVersionOperator{ ObjectMeta: metav1.ObjectMeta{ Generation: 4, }, @@ -112,18 +130,19 @@ func TestClusterVersionOperatorConfiguration_sync(t *testing.T) { ObservedGeneration: 4, }, }, - internalConfig: ClusterVersionOperatorConfiguration{ + internalConfig: configuration{ desiredLogLevel: operatorv1.Normal, lastObservedGeneration: 3, }, - expectedInternalConfig: ClusterVersionOperatorConfiguration{ + expectedInternalConfig: configuration{ desiredLogLevel: operatorv1.Trace, lastObservedGeneration: 4, }, + handlerFunctionCalled: true, }, { name: "number of not observed generations does not impact sync", - config: operatorv1alpha1.ClusterVersionOperator{ + config: &operatorv1alpha1.ClusterVersionOperator{ ObjectMeta: metav1.ObjectMeta{ Generation: 40, }, @@ -134,7 +153,7 @@ func TestClusterVersionOperatorConfiguration_sync(t *testing.T) { ObservedGeneration: 3, }, }, - expectedConfig: operatorv1alpha1.ClusterVersionOperator{ + expectedConfig: &operatorv1alpha1.ClusterVersionOperator{ ObjectMeta: metav1.ObjectMeta{ Generation: 40, }, @@ -145,131 +164,74 @@ func TestClusterVersionOperatorConfiguration_sync(t *testing.T) { ObservedGeneration: 40, }, }, - internalConfig: ClusterVersionOperatorConfiguration{ + internalConfig: configuration{ desiredLogLevel: operatorv1.Normal, lastObservedGeneration: 3, }, - expectedInternalConfig: ClusterVersionOperatorConfiguration{ + expectedInternalConfig: configuration{ desiredLogLevel: operatorv1.TraceAll, lastObservedGeneration: 40, }, + handlerFunctionCalled: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Initialize testing logic - client := operatorclientsetfake.NewClientset(&tt.config) - tt.internalConfig.client = client.OperatorV1alpha1().ClusterVersionOperators() - ctx, cancelFunc := context.WithDeadline(context.Background(), time.Now().Add(time.Minute)) - - // Run tested functionality - if err := tt.internalConfig.sync(ctx, &tt.config); err != nil { - t.Errorf("unexpected error %v", err) - } - - // Verify results - if tt.internalConfig.lastObservedGeneration != tt.expectedInternalConfig.lastObservedGeneration { - t.Errorf("unexpected 'lastObservedGeneration' value; wanted=%v, got=%v", tt.expectedInternalConfig.lastObservedGeneration, tt.internalConfig.lastObservedGeneration) - } - if tt.internalConfig.desiredLogLevel != tt.expectedInternalConfig.desiredLogLevel { - t.Errorf("unexpected 'desiredLogLevel' value; wanted=%v, got=%v", tt.expectedInternalConfig.desiredLogLevel, tt.internalConfig.desiredLogLevel) - } - - config, err := client.OperatorV1alpha1().ClusterVersionOperators().Get(ctx, "", metav1.GetOptions{}) - if err != nil { - t.Errorf("unexpected error %v", err) - } - if diff := cmp.Diff(tt.expectedConfig, *config, cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ManagedFields")); diff != "" { - t.Errorf("unexpected config (-want, +got) = %v", diff) - } - - // Shutdown created resources - cancelFunc() - }) - } -} - -func TestClusterVersionOperatorConfiguration_Sync(t *testing.T) { - tests := []struct { - name string - config *operatorv1alpha1.ClusterVersionOperator - expectedConfig *operatorv1alpha1.ClusterVersionOperator - }{ - { - name: "the configuration resource does not exist in the cluster -> ignore", - config: nil, - expectedConfig: nil, - }, - { - name: "Sync updates the ClusterVersionOperator resource", - config: &operatorv1alpha1.ClusterVersionOperator{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - Generation: 4, - }, - Spec: operatorv1alpha1.ClusterVersionOperatorSpec{ - OperatorLogLevel: operatorv1.Trace, - }, - Status: operatorv1alpha1.ClusterVersionOperatorStatus{ - ObservedGeneration: 3, - }, - }, - expectedConfig: &operatorv1alpha1.ClusterVersionOperator{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - Generation: 4, - }, - Spec: operatorv1alpha1.ClusterVersionOperatorSpec{ - OperatorLogLevel: operatorv1.Trace, - }, - Status: operatorv1alpha1.ClusterVersionOperatorStatus{ - ObservedGeneration: 4, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Initialize testing logic - var client *operatorclientsetfake.Clientset + client := operatorclientsetfake.NewClientset() if tt.config != nil { + tt.config.Name = ClusterVersionOperatorConfigurationName + tt.expectedConfig.Name = ClusterVersionOperatorConfigurationName client = operatorclientsetfake.NewClientset(tt.config) - } else { - client = operatorclientsetfake.NewClientset() } - factory := operatorexternalversions.NewSharedInformerFactoryWithOptions(client, time.Minute) - cvoConfiguration := NewClusterVersionOperatorConfiguration(client, factory) - defer cvoConfiguration.queue.ShutDown() + configController := NewClusterVersionOperatorConfiguration(client, factory) + + called := false + configController.handler = func(_ configuration) error { + called = true + return nil + } ctx, cancelFunc := context.WithDeadline(context.Background(), time.Now().Add(time.Minute)) - defer cancelFunc() - err := cvoConfiguration.Start(ctx) - if err != nil { + if err := configController.Start(ctx); err != nil { t.Errorf("unexpected error %v", err) } + configController.configuration = tt.internalConfig // Run tested functionality - err = cvoConfiguration.Sync(ctx, "ClusterVersionOperator/cluster") - if err != nil { + if err := configController.Sync(ctx, "key"); err != nil { t.Errorf("unexpected error %v", err) } // Verify results - config, err := client.OperatorV1alpha1().ClusterVersionOperators().Get(ctx, "cluster", metav1.GetOptions{}) + if configController.configuration.lastObservedGeneration != tt.expectedInternalConfig.lastObservedGeneration { + t.Errorf("unexpected 'lastObservedGeneration' value; wanted=%v, got=%v", tt.expectedInternalConfig.lastObservedGeneration, configController.configuration.lastObservedGeneration) + } + if configController.configuration.desiredLogLevel != tt.expectedInternalConfig.desiredLogLevel { + t.Errorf("unexpected 'desiredLogLevel' value; wanted=%v, got=%v", tt.expectedInternalConfig.desiredLogLevel, configController.configuration.desiredLogLevel) + } + + config, err := client.OperatorV1alpha1().ClusterVersionOperators().Get(ctx, ClusterVersionOperatorConfigurationName, metav1.GetOptions{}) if err != nil && !apierrors.IsNotFound(err) { t.Errorf("unexpected error %v", err) } - 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) - } + // Set nil to differentiate between nonexisting configurations + if apierrors.IsNotFound(err) { + config = nil + } + if diff := cmp.Diff(tt.expectedConfig, config, cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ManagedFields")); diff != "" { + t.Errorf("unexpected config (-want, +got) = %v", diff) + } + + if tt.handlerFunctionCalled != called { + t.Errorf("unexpected handler function execution; wanted=%v, got=%v", tt.handlerFunctionCalled, called) } + + // Shutdown created resources + cancelFunc() }) } } diff --git a/pkg/cvo/configuration/loglevel.go b/pkg/cvo/configuration/loglevel.go new file mode 100644 index 000000000..0e87f47dc --- /dev/null +++ b/pkg/cvo/configuration/loglevel.go @@ -0,0 +1,42 @@ +package configuration + +import ( + "fmt" + + "k8s.io/klog/v2" + + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/library-go/pkg/operator/loglevel" + + i "github.com/openshift/cluster-version-operator/pkg/internal" +) + +func applyLogLevel(level operatorv1.LogLevel) error { + currentLogLevel, notFound := loglevel.GetLogLevel() + if notFound { + klog.Warningf("The current log level could not be found; an attempt to set the log level to the desired level will be made") + } + + if !notFound && currentLogLevel == level { + 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(level); err != nil { + return fmt.Errorf("failed to set the log level to %q: %w", level, err) + } + + // E2E testing will be checking for existence or absence of these logs + switch level { + case operatorv1.Normal: + klog.V(i.Normal).Infof("Successfully updated the log level from '%s' to 'Normal'", currentLogLevel) + case operatorv1.Debug: + klog.V(i.Debug).Infof("Successfully updated the log level from '%s' to 'Debug'", currentLogLevel) + case operatorv1.Trace: + klog.V(i.Trace).Infof("Successfully updated the log level from '%s' to 'Trace'", currentLogLevel) + case operatorv1.TraceAll: + klog.V(i.TraceAll).Infof("Successfully updated the log level from '%s' to 'TraceAll'", currentLogLevel) + default: + klog.Errorf("The CVO logging level has unexpected value '%s'", level) + } + } + return nil +} diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index 6f186479f..441239125 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -290,7 +290,9 @@ func New( // make sure this is initialized after all the listers are initialized optr.upgradeableChecks = optr.defaultUpgradeableChecks() - optr.configuration = configuration.NewClusterVersionOperatorConfiguration(operatorClient, operatorInformerFactory) + if shouldReconcileCVOConfiguration(cvoGates, optr.hypershift) { + optr.configuration = configuration.NewClusterVersionOperatorConfiguration(operatorClient, operatorInformerFactory) + } return optr, nil } @@ -444,7 +446,9 @@ func (optr *Operator) Run(runContext context.Context, shutdownContext context.Co defer optr.queue.ShutDown() defer optr.availableUpdatesQueue.ShutDown() defer optr.upgradeableQueue.ShutDown() - defer optr.configuration.Queue().ShutDown() + if optr.configuration != nil { + defer optr.configuration.Queue().ShutDown() + } stopCh := runContext.Done() klog.Infof("Starting ClusterVersionOperator with minimum reconcile period %s", optr.minimumUpdateCheckInterval) @@ -457,6 +461,12 @@ func (optr *Operator) Run(runContext context.Context, shutdownContext context.Co return fmt.Errorf("caches never synchronized: %w", runContext.Err()) } + if optr.configuration != nil { + if err := optr.configuration.Start(runContext); err != nil { + return fmt.Errorf("unable to initialize the CVO configuration controller: %v", err) + } + } + // trigger the first cluster version reconcile always optr.queue.Add(optr.queueKey()) @@ -483,17 +493,13 @@ func (optr *Operator) Run(runContext context.Context, shutdownContext context.Co resultChannel <- asyncResult{name: "available updates"} }() - if optr.shouldReconcileCVOConfiguration() { + if optr.configuration != nil { resultChannelCount++ go func() { defer utilruntime.HandleCrash() - if err := optr.configuration.Start(runContext); err != nil { - utilruntime.HandleError(fmt.Errorf("unable to initialize the CVO configuration sync: %v", err)) - } else { - wait.UntilWithContext(runContext, func(runContext context.Context) { - optr.worker(runContext, optr.configuration.Queue(), optr.configuration.Sync) - }, time.Second) - } + wait.UntilWithContext(runContext, func(runContext context.Context) { + optr.worker(runContext, optr.configuration.Queue(), optr.configuration.Sync) + }, time.Second) resultChannel <- asyncResult{name: "cvo configuration"} }() } else { @@ -569,7 +575,9 @@ func (optr *Operator) Run(runContext context.Context, shutdownContext context.Co optr.queue.ShutDown() optr.availableUpdatesQueue.ShutDown() optr.upgradeableQueue.ShutDown() - optr.configuration.Queue().ShutDown() + if optr.configuration != nil { + optr.configuration.Queue().ShutDown() + } } } @@ -1070,7 +1078,7 @@ func (optr *Operator) HTTPClient() (*http.Client, error) { // shouldReconcileCVOConfiguration returns whether the CVO should reconcile its configuration using the API server. // // enabledFeatureGates must be initialized before the function is called. -func (optr *Operator) shouldReconcileCVOConfiguration() bool { +func shouldReconcileCVOConfiguration(enabledFeatureGates featuregates.CvoGateChecker, isHypershift bool) bool { // The relevant CRD and CR are not applied in HyperShift, which configures the CVO via a configuration file - return optr.enabledFeatureGates.CVOConfiguration() && !optr.hypershift + return enabledFeatureGates.CVOConfiguration() && !isHypershift }