Merge pull request #4742 from umohnani8/logs

MCO-1016: Only update when there is an actual controller change

Author: openshift-merge-bot[bot]

cmd/machine-config-controller/start.go

@@ -201,7 +201,6 @@ func createControllers(ctx *ctrlcommon.ControllerContext) []ctrlcommon.Controlle
 		template.New(
 			rootOpts.templates,
 			ctx.InformerFactory.Machineconfiguration().V1().ControllerConfigs(),
-			ctx.InformerFactory.Machineconfiguration().V1().MachineConfigs(),
 			ctx.OpenShiftConfigKubeNamespacedInformerFactory.Core().V1().Secrets(),
 			ctx.ConfigInformerFactory.Config().V1().APIServers(),
 			ctx.ClientBuilder.KubeClientOrDie("template-controller"),

pkg/controller/render/render_controller.go

@@ -227,6 +227,12 @@ func (ctrl *Controller) updateMachineConfig(old, cur interface{}) {
 	oldMC := old.(*mcfgv1.MachineConfig)
 	curMC := cur.(*mcfgv1.MachineConfig)
 
+	// return if spec hasn't changed
+	if reflect.DeepEqual(oldMC.Spec, curMC.Spec) {
+		return
+	}
+	klog.V(4).Infof("MachineConfig %s updated", curMC.Name)
+
 	curControllerRef := metav1.GetControllerOf(curMC)
 	oldControllerRef := metav1.GetControllerOf(oldMC)
 	controllerRefChanged := !reflect.DeepEqual(curControllerRef, oldControllerRef)
@@ -237,7 +243,6 @@ func (ctrl *Controller) updateMachineConfig(old, cur interface{}) {
 
 	if curControllerRef != nil {
 		if pool := ctrl.resolveControllerRef(curControllerRef); pool != nil {
-			klog.V(4).Infof("MachineConfig %s updated", curMC.Name)
 			ctrl.enqueueMachineConfigPool(pool)
 			return
 		}
@@ -248,8 +253,6 @@ func (ctrl *Controller) updateMachineConfig(old, cur interface{}) {
 		klog.Errorf("error finding pools for machineconfig: %v", err)
 		return
 	}
-
-	klog.V(4).Infof("MachineConfig %s updated", curMC.Name)
 	for _, p := range pools {
 		ctrl.enqueueMachineConfigPool(p)
 	}

pkg/controller/render/render_controller_test.go

@@ -587,7 +587,7 @@ func TestMachineConfigsNoBailWithoutPool(t *testing.T) {
 	c.addMachineConfig(mc)
 	c.updateMachineConfig(mc, mc)
 	c.deleteMachineConfig(mc)
-	require.Len(t, queue, 3)
+	require.Len(t, queue, 2)
 }
 
 func TestGenerateMachineConfigValidation(t *testing.T) {

pkg/controller/template/template_controller.go

@@ -66,13 +66,11 @@ type Controller struct {
 	enqueueControllerConfig func(*mcfgv1.ControllerConfig)
 
 	ccLister mcfglistersv1.ControllerConfigLister
-	mcLister mcfglistersv1.MachineConfigLister
 
 	apiserverLister       configlistersv1.APIServerLister
 	apiserverListerSynced cache.InformerSynced
 
 	ccListerSynced        cache.InformerSynced
-	mcListerSynced        cache.InformerSynced
 	secretsInformerSynced cache.InformerSynced
 
 	queue workqueue.TypedRateLimitingInterface[string]
@@ -84,7 +82,6 @@ type Controller struct {
 func New(
 	templatesDir string,
 	ccInformer mcfginformersv1.ControllerConfigInformer,
-	mcInformer mcfginformersv1.MachineConfigInformer,
 	secretsInformer coreinformersv1.SecretInformer,
 	apiserverInformer configinformersv1.APIServerInformer,
 	kubeClient clientset.Interface,
@@ -111,12 +108,6 @@ func New(
 		DeleteFunc: ctrl.deleteControllerConfig,
 	})
 
-	mcInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
-		AddFunc:    ctrl.addMachineConfig,
-		UpdateFunc: ctrl.updateMachineConfig,
-		DeleteFunc: ctrl.deleteMachineConfig,
-	})
-
 	secretsInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc:    ctrl.addSecret,
 		UpdateFunc: ctrl.updateSecret,
@@ -133,9 +124,7 @@ func New(
 	ctrl.enqueueControllerConfig = ctrl.enqueue
 
 	ctrl.ccLister = ccInformer.Lister()
-	ctrl.mcLister = mcInformer.Lister()
 	ctrl.ccListerSynced = ccInformer.Informer().HasSynced
-	ctrl.mcListerSynced = mcInformer.Informer().HasSynced
 	ctrl.secretsInformerSynced = secretsInformer.Informer().HasSynced
 
 	ctrl.apiserverLister = apiserverInformer.Lister()
@@ -291,7 +280,7 @@ func (ctrl *Controller) Run(workers int, stopCh <-chan struct{}) {
 	defer utilruntime.HandleCrash()
 	defer ctrl.queue.ShutDown()
 
-	if !cache.WaitForCacheSync(stopCh, ctrl.ccListerSynced, ctrl.mcListerSynced, ctrl.secretsInformerSynced) {
+	if !cache.WaitForCacheSync(stopCh, ctrl.ccListerSynced, ctrl.secretsInformerSynced) {
 		return
 	}
 
@@ -314,8 +303,20 @@ func (ctrl *Controller) addControllerConfig(obj interface{}) {
 func (ctrl *Controller) updateControllerConfig(old, cur interface{}) {
 	oldCfg := old.(*mcfgv1.ControllerConfig)
 	curCfg := cur.(*mcfgv1.ControllerConfig)
-	klog.V(4).Infof("Updating ControllerConfig %s", oldCfg.Name)
-	ctrl.enqueueControllerConfig(curCfg)
+	if controllerConfigTriggerObjectChange(oldCfg, curCfg) {
+		klog.V(4).Infof("Updating ControllerConfig %s", oldCfg.Name)
+		ctrl.enqueueControllerConfig(curCfg)
+	}
+}
+
+func controllerConfigTriggerObjectChange(oldControllerConfig, newControllerConfig *mcfgv1.ControllerConfig) bool {
+	if oldControllerConfig.DeletionTimestamp != newControllerConfig.DeletionTimestamp {
+		return true
+	}
+	if !reflect.DeepEqual(oldControllerConfig.Spec, newControllerConfig.Spec) {
+		return true
+	}
+	return false
 }
 
 func (ctrl *Controller) deleteControllerConfig(obj interface{}) {
@@ -336,90 +337,6 @@ func (ctrl *Controller) deleteControllerConfig(obj interface{}) {
 	// TODO(abhinavdahiya): handle deletes.
 }
 
-func (ctrl *Controller) addMachineConfig(obj interface{}) {
-	mc := obj.(*mcfgv1.MachineConfig)
-	if mc.DeletionTimestamp != nil {
-		ctrl.deleteMachineConfig(mc)
-		return
-	}
-
-	if controllerRef := metav1.GetControllerOf(mc); controllerRef != nil {
-		cfg := ctrl.resolveControllerRef(controllerRef)
-		if cfg == nil {
-			return
-		}
-		klog.V(4).Infof("MachineConfig %s added", mc.Name)
-		ctrl.enqueueControllerConfig(cfg)
-		return
-	}
-
-	// No adopting.
-}
-
-func (ctrl *Controller) updateMachineConfig(_, cur interface{}) {
-	curMC := cur.(*mcfgv1.MachineConfig)
-
-	if controllerRef := metav1.GetControllerOf(curMC); controllerRef != nil {
-		cfg := ctrl.resolveControllerRef(controllerRef)
-		if cfg == nil {
-			return
-		}
-		klog.V(4).Infof("MachineConfig %s updated", curMC.Name)
-		ctrl.enqueueControllerConfig(cfg)
-		return
-	}
-
-	// No adopting.
-}
-
-func (ctrl *Controller) deleteMachineConfig(obj interface{}) {
-	mc, ok := obj.(*mcfgv1.MachineConfig)
-
-	if !ok {
-		tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
-		if !ok {
-			utilruntime.HandleError(fmt.Errorf("couldn't get object from tombstone %#v", obj))
-			return
-		}
-		mc, ok = tombstone.Obj.(*mcfgv1.MachineConfig)
-		if !ok {
-			utilruntime.HandleError(fmt.Errorf("tombstone contained object that is not a MachineConfig %#v", obj))
-			return
-		}
-	}
-
-	controllerRef := metav1.GetControllerOf(mc)
-	if controllerRef == nil {
-		// No controller should care about orphans being deleted.
-		return
-	}
-	cfg := ctrl.resolveControllerRef(controllerRef)
-	if cfg == nil {
-		return
-	}
-	klog.V(4).Infof("MachineConfig %s deleted.", mc.Name)
-	ctrl.enqueueControllerConfig(cfg)
-}
-
-func (ctrl *Controller) resolveControllerRef(controllerRef *metav1.OwnerReference) *mcfgv1.ControllerConfig {
-	// We can't look up by UID, so look up by Name and then verify UID.
-	// Don't even try to look up by Name if it's the wrong Kind.
-	if controllerRef.Kind != controllerKind.Kind {
-		return nil
-	}
-	cfg, err := ctrl.ccLister.Get(controllerRef.Name)
-	if err != nil {
-		return nil
-	}
-
-	if cfg.UID != controllerRef.UID {
-		// The controller we found with this Name is not the same one that the
-		// ControllerRef points to.
-		return nil
-	}
-	return cfg
-}
-
 func (ctrl *Controller) enqueue(config *mcfgv1.ControllerConfig) {
 	key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(config)
 	if err != nil {

pkg/controller/template/template_controller_test.go

@@ -46,7 +46,6 @@ type fixture struct {
 	apiserverclient *oseconfigfake.Clientset
 
 	ccLister []*mcfgv1.ControllerConfig
-	mcLister []*mcfgv1.MachineConfig
 
 	kubeactions []core.Action
 	actions     []core.Action
@@ -111,11 +110,10 @@ func (f *fixture) newController() *Controller {
 
 	i := informers.NewSharedInformerFactory(f.client, noResyncPeriodFunc())
 	c := New(templateDir,
-		i.Machineconfiguration().V1().ControllerConfigs(), i.Machineconfiguration().V1().MachineConfigs(), cinformer.Core().V1().Secrets(),
+		i.Machineconfiguration().V1().ControllerConfigs(), cinformer.Core().V1().Secrets(),
 		apiserverinformer.Config().V1().APIServers(), f.kubeclient, f.client)
 
 	c.ccListerSynced = alwaysReady
-	c.mcListerSynced = alwaysReady
 	c.apiserverListerSynced = alwaysReady
 	c.eventRecorder = &record.FakeRecorder{}
 
@@ -128,10 +126,6 @@ func (f *fixture) newController() *Controller {
 		i.Machineconfiguration().V1().ControllerConfigs().Informer().GetIndexer().Add(c)
 	}
 
-	for _, m := range f.mcLister {
-		i.Machineconfiguration().V1().MachineConfigs().Informer().GetIndexer().Add(m)
-	}
-
 	return c
 }
 
@@ -327,7 +321,6 @@ func TestDoNothing(t *testing.T) {
 	f.ccLister = append(f.ccLister, cc)
 	f.objects = append(f.objects, cc)
 	f.kubeobjects = append(f.kubeobjects, ps)
-	f.mcLister = append(f.mcLister, mcs...)
 	for idx := range mcs {
 		f.objects = append(f.objects, mcs[idx])
 	}
@@ -366,7 +359,6 @@ func TestRecreateMachineConfig(t *testing.T) {
 	f.objects = append(f.objects, cc)
 	f.kubeobjects = append(f.kubeobjects, ps)
 	for idx := 0; idx < len(mcs)-1; idx++ {
-		f.mcLister = append(f.mcLister, mcs[idx])
 		f.objects = append(f.objects, mcs[idx])
 	}
 
@@ -412,7 +404,6 @@ func TestUpdateMachineConfig(t *testing.T) {
 	f.kubeobjects = append(f.kubeobjects, ps)
 	f.objects = append(f.objects, cc)
 	for idx := range mcs {
-		f.mcLister = append(f.mcLister, mcs[idx])
 		f.objects = append(f.objects, mcs[idx])
 	}
 

test/e2e-bootstrap/bootstrap_test.go

@@ -467,7 +467,6 @@ func createControllers(ctx *ctrlcommon.ControllerContext) []ctrlcommon.Controlle
 		template.New(
 			templatesDir,
 			ctx.InformerFactory.Machineconfiguration().V1().ControllerConfigs(),
-			ctx.InformerFactory.Machineconfiguration().V1().MachineConfigs(),
 			ctx.OpenShiftConfigKubeNamespacedInformerFactory.Core().V1().Secrets(),
 			ctx.ConfigInformerFactory.Config().V1().APIServers(),
 			ctx.ClientBuilder.KubeClientOrDie("template-controller"),