Merge pull request #1371 from jmencak/4.20-bootcmdline-conflict

NO-JIRA: Address false reports of bootcmdline conflicts

Author: openshift-merge-bot[bot]

pkg/operator/controller.go

@@ -731,6 +731,7 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
 	profile.Spec.Config.ProviderName = providerName
 	profile.Spec.Profile = computed.AllProfiles
 	profile.Status.Conditions = tunedpkg.InitializeStatusConditions()
+	delete(c.pc.state.bootcmdline, nodeName) // bootcmdline retrieved from node annotation is potentially stale, let it resync on node update
 
 	klog.V(2).Infof("syncProfile(): updating Profile %s [%s]", profile.Name, computed.TunedProfileName)
 	_, err = c.clients.Tuned.TunedV1().Profiles(ntoconfig.WatchNamespace()).Update(context.TODO(), profile, metav1.UpdateOptions{})
@@ -780,18 +781,17 @@ func (c *Controller) syncMachineConfig(labels map[string]string, profile *tunedv
 	name := GetMachineConfigNameForPools(pools)
 	klog.V(2).Infof("syncMachineConfig(): %v", name)
 
-	var bootcmdlineSet bool
 	nodes, err := c.pc.getNodesForPool(pools[0])
 	if err != nil {
 		return err
 	}
 
-	bootcmdline, bootcmdlineSet = c.pc.state.bootcmdline[profile.Name]
-	if !bootcmdlineSet {
-		klog.V(2).Infof("syncMachineConfig(): bootcmdline for %s not cached, sync canceled", profile.Name)
+	if ok := c.allNodesHaveBootcmdlineSet(nodes); !ok {
+		klog.V(2).Infof("syncMachineConfig(): bootcmdline for %s not cached for all nodes, sync canceled", profile.Name)
 		return nil
 	}
 
+	bootcmdline = c.pc.state.bootcmdline[profile.Name]
 	if ok := c.allNodesAgreeOnBootcmdline(nodes); !ok {
 		// Log an error and do not requeue, this is a configuration issue.
 		klog.Errorf("not all %d Nodes in MCP %v agree on bootcmdline: %s", len(nodes), pools[0].Name, bootcmdline)
@@ -848,6 +848,36 @@ func (c *Controller) syncMachineConfig(labels map[string]string, profile *tunedv
 	return nil
 }
 
+// allNodesHaveBootcmdlineSet returns true if all Nodes in slice 'nodes' have
+// their bootcmdline annotation (TunedBootcmdlineAnnotationKey) value cached in
+// the profilecalculator's cache.  The values in the cache are populated on Node
+// updates (regular ones or just resyncs) and removed from the cache on Profile
+// updates.  Note this is not a bullet-proof solution to false bootcmdline
+// conflicts, because we can still have races, such as regular k8s object
+// Node resync (or other independent Node updates) right after Profile update.
+// While the cached bootcmdline value will be deleted after Profile update, it
+// can still be populated by a stale value from Node's annotation because of the
+// Node's resync (or other independent updates) before the proper bootcmdline
+// is calculated by the TuneD pod and Node's (TunedBootcmdlineAnnotationKey)
+// annotation updated.  A bullet-proof solution would involve adding a new
+// annotation such as tuned.openshift.io/lastProfileObservedGen to the Node
+// tied to the Profile and comparing it to the Profile's generation prior to
+// updating a MachineConfig.  However, we want to avoid putting extra load
+// on the API server as much as possible and this simpler solution already
+// significantly reduces false reports of bootcmdline conflicts.
+func (c *Controller) allNodesHaveBootcmdlineSet(nodes []*corev1.Node) bool {
+	for _, node := range nodes {
+		if v, bootcmdlineSet := c.pc.state.bootcmdline[node.Name]; !bootcmdlineSet {
+			klog.V(3).Infof("allNodesHaveBootcmdlineSet(): bootcmdline not set for node %s", node.Name)
+			return false
+		} else {
+			klog.V(3).Infof("allNodesHaveBootcmdlineSet(): bootcmdline %q set for node %s", v, node.Name)
+		}
+	}
+
+	return true
+}
+
 // allNodesAgreeOnBootcmdline returns true if the current cached annotation 'TunedBootcmdlineAnnotationKey'
 // of all Nodes in slice 'nodes' has the same value.
 func (c *Controller) allNodesAgreeOnBootcmdline(nodes []*corev1.Node) bool {
@@ -881,6 +911,11 @@ func (c *Controller) syncMachineConfigHyperShift(nodePoolName string, profile *t
 		return fmt.Errorf("could not fetch a list of Nodes for NodePool %s: %v", nodePoolName, err)
 	}
 
+	if ok := c.allNodesHaveBootcmdlineSet(nodes); !ok {
+		klog.V(2).Infof("syncMachineConfigHyperShift(): bootcmdline for %s not cached for all nodes, sync canceled", profile.Name)
+		return nil
+	}
+
 	bootcmdline := c.pc.state.bootcmdline[profile.Name]
 	if ok := c.allNodesAgreeOnBootcmdline(nodes); !ok {
 		return fmt.Errorf("not all %d Nodes in NodePool %v agree on bootcmdline: %s", len(nodes), nodePoolName, bootcmdline)