addressing review feedback

Signed-off-by: Michael Shitrit <mshitrit@redhat.com>

Author: mshitrit

controllers/cluster/upgrade_checker.go

@@ -6,6 +6,7 @@ import (
 	"github.com/go-logr/logr"
 	gerrors "github.com/pkg/errors"
 
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -14,12 +15,19 @@ import (
 	clusterversion "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
 )
 
+const (
+	// currentMachineConfigAnnotationKey is used to fetch current targetConfigVersionHash
+	currentMachineConfigAnnotationKey = "machineconfiguration.openshift.io/currentConfig"
+	// desiredMachineConfigAnnotationKey is used to indicate the version a node should be updating to
+	desiredMachineConfigAnnotationKey = "machineconfiguration.openshift.io/desiredConfig"
+)
+
 // UpgradeChecker checks if the cluster is currently under upgrade.
 // error should be thrown if it can't reliably determine if it's under upgrade or not.
 type UpgradeChecker interface {
 	// Check if the cluster is currently under upgrade.
 	// error should be thrown if it can't reliably determine if it's under upgrade or not.
-	Check() (bool, error)
+	Check(nodesToBeRemediated []corev1.Node) (bool, error)
 }
 
 type openshiftClusterUpgradeStatusChecker struct {
@@ -31,7 +39,11 @@ type openshiftClusterUpgradeStatusChecker struct {
 // force implementation of interface
 var _ UpgradeChecker = &openshiftClusterUpgradeStatusChecker{}
 
-func (o *openshiftClusterUpgradeStatusChecker) Check() (bool, error) {
+func (o *openshiftClusterUpgradeStatusChecker) Check(nodesToBeRemediated []corev1.Node) (bool, error) {
+	if o.isNodeUpgrading(nodesToBeRemediated) {
+		return true, nil
+	}
+
 	cvs, err := o.clusterVersionsClient.List(context.Background(), metav1.ListOptions{})
 	if err != nil {
 		return false, gerrors.Wrap(err, "failed to check for Openshift cluster upgrade status")
@@ -47,13 +59,27 @@ func (o *openshiftClusterUpgradeStatusChecker) Check() (bool, error) {
 	return false, nil
 }
 
+func (o *openshiftClusterUpgradeStatusChecker) isNodeUpgrading(nodesToBeRemediated []corev1.Node) bool {
+	// We can identify an HCP (Hosted Control Plane) cluster upgrade by checking annotations of the CP node.
+	for _, node := range nodesToBeRemediated {
+		if len(node.Annotations) == 0 || len(node.Annotations[desiredMachineConfigAnnotationKey]) == 0 {
+			continue
+		}
+
+		if node.Annotations[currentMachineConfigAnnotationKey] != node.Annotations[desiredMachineConfigAnnotationKey] {
+			return true
+		}
+	}
+	return false
+}
+
 type noopClusterUpgradeStatusChecker struct {
 }
 
 // force implementation of interface
 var _ UpgradeChecker = &noopClusterUpgradeStatusChecker{}
 
-func (n *noopClusterUpgradeStatusChecker) Check() (bool, error) {
+func (n *noopClusterUpgradeStatusChecker) Check([]corev1.Node) (bool, error) {
 	return false, nil
 }
 

controllers/nodehealthcheck_controller.go

@@ -69,11 +69,6 @@ const (
 
 	// RemediationControlPlaneLabelKey is the label key to put on remediation CRs for control plane nodes
 	RemediationControlPlaneLabelKey = "remediation.medik8s.io/isControlPlaneNode"
-
-	// currentMachineConfigAnnotationKey is used to fetch current targetConfigVersionHash
-	currentMachineConfigAnnotationKey = "machineconfiguration.openshift.io/currentConfig"
-	// desiredMachineConfigAnnotationKey is used to indicate the version a node should be updating to
-	desiredMachineConfigAnnotationKey = "machineconfiguration.openshift.io/desiredConfig"
 )
 
 var (
@@ -394,26 +389,13 @@ func (r *NodeHealthCheckReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 }
 
 func (r *NodeHealthCheckReconciler) isClusterUpgrading(nodesToBeRemediated []v1.Node) bool {
-	clusterUpgrading, err := r.ClusterUpgradeStatusChecker.Check()
+	clusterUpgrading, err := r.ClusterUpgradeStatusChecker.Check(nodesToBeRemediated)
 	if err != nil {
 		// if we can't reliably tell if the cluster is upgrading then just continue with remediation.
 		// TODO finer error handling may help to decide otherwise here.
 		r.Log.Error(err, "failed to check if the cluster is upgrading. Proceed with remediation as if it is not upgrading")
 	}
-	return clusterUpgrading || r.isHostedControlPlaneUpgrading(nodesToBeRemediated)
-}
-
-func (r *NodeHealthCheckReconciler) isHostedControlPlaneUpgrading(nodesToBeRemediated []v1.Node) bool {
-	for _, node := range nodesToBeRemediated {
-		if !nodes.IsControlPlane(&node) || len(node.Annotations) == 0 || len(node.Annotations[desiredMachineConfigAnnotationKey]) == 0 {
-			continue
-		}
-
-		if node.Annotations[currentMachineConfigAnnotationKey] != node.Annotations[desiredMachineConfigAnnotationKey] {
-			return true
-		}
-	}
-	return false
+	return clusterUpgrading
 }
 
 func (r *NodeHealthCheckReconciler) checkNodeConditions(nodes []v1.Node, nhc *remediationv1alpha1.NodeHealthCheck) (notMatchingNodes, soonMatchingNodes, matchingNodes []v1.Node, requeueAfter *time.Duration) {

controllers/nodehealthcheck_controller_test.go

@@ -1687,11 +1687,13 @@ var _ = Describe("Node Health Check CR", func() {
 		})
 
 		When("Nodes are candidates for remediation and cluster is upgrading", func() {
+			BeforeEach(func() {
+				clusterUpgradeRequeueAfter = 5 * time.Second
+				setupObjects(1, 2, true)
+			})
 			When("non HCP Upgrade", func() {
 				BeforeEach(func() {
-					clusterUpgradeRequeueAfter = 5 * time.Second
 					upgradeChecker.Upgrading = true
-					setupObjects(1, 2, true)
 				})
 
 				AfterEach(func() {
@@ -1722,49 +1724,31 @@ var _ = Describe("Node Health Check CR", func() {
 
 			})
 			When("HCP Upgrade", func() {
-				var unhealthyCPNodeName string
+				var currentMachineConfigAnnotationKey = "machineconfiguration.openshift.io/currentConfig"
+				var desiredMachineConfigAnnotationKey = "machineconfiguration.openshift.io/desiredConfig"
 				BeforeEach(func() {
-					clusterUpgradeRequeueAfter = 5 * time.Second
-					objects = newNodes(1, 0, true, true)
-					cpNode := objects[0]
-					objects = append(objects, newNodes(0, 2, false, false)...)
-					objects = append(objects, underTest)
-					unhealthyCPNodeName = cpNode.GetName()
-					cpNodeAnnotations := map[string]string{}
-					if cpNode.GetAnnotations() != nil {
-						cpNodeAnnotations = cpNode.GetAnnotations()
-					}
-					cpNodeAnnotations[currentMachineConfigAnnotationKey] = "fakeVersion1"
-					cpNodeAnnotations[desiredMachineConfigAnnotationKey] = "fakeVersion2"
-					cpNode.SetAnnotations(cpNodeAnnotations)
-
-					//Mock PDB in order to allow creation of CP remediation
-					etcdNs := &v1.Namespace{
-						ObjectMeta: metav1.ObjectMeta{
-							Name: "openshift-etcd",
-						},
-						Spec: v1.NamespaceSpec{},
-					}
-					err := k8sClient.Create(context.Background(), etcdNs)
-					if err != nil {
-						Expect(errors.IsAlreadyExists(err)).To(BeTrue())
-					}
-
-					pdb := &policyv1.PodDisruptionBudget{}
-					pdb.Namespace = etcdNs.Name
-					pdb.Name = "mockPDB"
-					Expect(k8sClient.Create(context.TODO(), pdb)).To(Succeed())
-					pdb.Status.DisruptionsAllowed = 1
-					Expect(k8sClient.Status().Update(context.TODO(), pdb)).To(Succeed())
+					//Use a real upgrade checker instead of mock
+					prevUpgradeChecker := nhcReconciler.ClusterUpgradeStatusChecker
+					nhcReconciler.ClusterUpgradeStatusChecker = ocpUpgradeChecker
 					DeferCleanup(func() {
-						Expect(k8sClient.Delete(context.TODO(), pdb)).To(Succeed())
+						nhcReconciler.ClusterUpgradeStatusChecker = prevUpgradeChecker
 					})
 
+					//Simulate HCP Upgrade on the unhealthy node
+					upgradingNode := objects[0]
+					upgradingNodeAnnotations := map[string]string{}
+					if upgradingNode.GetAnnotations() != nil {
+						upgradingNodeAnnotations = upgradingNode.GetAnnotations()
+					}
+					upgradingNodeAnnotations[currentMachineConfigAnnotationKey] = "fakeVersion1"
+					upgradingNodeAnnotations[desiredMachineConfigAnnotationKey] = "fakeVersion2"
+					upgradingNode.SetAnnotations(upgradingNodeAnnotations)
+
 				})
 
 				It("doesn't not remediate but requeues reconciliation and updates status", func() {
 
-					cr := findRemediationCRForNHC(unhealthyCPNodeName, underTest)
+					cr := findRemediationCRForNHC(unhealthyNodeName, underTest)
 					Expect(cr).To(BeNil())
 
 					Expect(*underTest.Status.HealthyNodes).To(Equal(0))
@@ -1774,20 +1758,16 @@ var _ = Describe("Node Health Check CR", func() {
 					Expect(underTest.Status.Reason).ToNot(BeEmpty())
 
 					By("stopping upgrade and waiting for requeue")
-					nodeList := v1.NodeList{}
-					Expect(k8sClient.List(context.TODO(), &nodeList)).To(Succeed())
-					for _, node := range nodeList.Items {
-						if len(node.GetAnnotations()) == 0 {
-							continue
-						}
-						if node.GetAnnotations()[currentMachineConfigAnnotationKey] != node.GetAnnotations()[desiredMachineConfigAnnotationKey] {
-							node.Annotations[currentMachineConfigAnnotationKey] = node.GetAnnotations()[desiredMachineConfigAnnotationKey]
-							Expect(k8sClient.Update(context.TODO(), &node)).To(Succeed())
-						}
+					unhealthyNode := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: unhealthyNodeName}}
+					Expect(k8sClient.Get(context.TODO(), client.ObjectKeyFromObject(unhealthyNode), unhealthyNode)).To(Succeed())
+					if unhealthyNode.GetAnnotations()[currentMachineConfigAnnotationKey] != unhealthyNode.GetAnnotations()[desiredMachineConfigAnnotationKey] {
+						// Simulating upgrade complete.
+						unhealthyNode.Annotations[currentMachineConfigAnnotationKey] = unhealthyNode.GetAnnotations()[desiredMachineConfigAnnotationKey]
+						Expect(k8sClient.Update(context.TODO(), unhealthyNode)).To(Succeed())
 					}
 
 					time.Sleep(10 * time.Second)
-					cr = findRemediationCRForNHC(unhealthyCPNodeName, underTest)
+					cr = findRemediationCRForNHC(unhealthyNodeName, underTest)
 					Expect(cr).ToNot(BeNil())
 
 					Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(underTest), underTest)).To(Succeed())

controllers/suite_test.go

@@ -120,9 +120,13 @@ var (
 	ctx        context.Context
 	cancel     context.CancelFunc
 
-	upgradeChecker *fakeClusterUpgradeChecker
-	fakeTime       *time.Time
-	fakeRecorder   *record.FakeRecorder
+	upgradeChecker    *fakeClusterUpgradeChecker
+	ocpUpgradeChecker cluster.UpgradeChecker
+
+	fakeTime     *time.Time
+	fakeRecorder *record.FakeRecorder
+
+	nhcReconciler *NodeHealthCheckReconciler
 )
 
 func TestAPIs(t *testing.T) {
@@ -238,15 +242,17 @@ var _ = BeforeSuite(func() {
 
 	mhcEvents := make(chan event.GenericEvent)
 	fakeRecorder = record.NewFakeRecorder(1000)
-	err = (&NodeHealthCheckReconciler{
+	ocpUpgradeChecker, _ = cluster.NewClusterUpgradeStatusChecker(k8sManager, cluster.Capabilities{IsOnOpenshift: true})
+	nhcReconciler = &NodeHealthCheckReconciler{
 		Client:                      k8sManager.GetClient(),
 		Log:                         k8sManager.GetLogger().WithName("test reconciler"),
 		Recorder:                    fakeRecorder,
 		ClusterUpgradeStatusChecker: upgradeChecker,
 		MHCChecker:                  mhcChecker,
 		MHCEvents:                   mhcEvents,
 		Capabilities:                caps,
-	}).SetupWithManager(k8sManager)
+	}
+	err = nhcReconciler.SetupWithManager(k8sManager)
 	Expect(err).NotTo(HaveOccurred())
 
 	err = (&MachineHealthCheckReconciler{
@@ -286,7 +292,7 @@ type fakeClusterUpgradeChecker struct {
 // force implementation of interface
 var _ cluster.UpgradeChecker = &fakeClusterUpgradeChecker{}
 
-func (c *fakeClusterUpgradeChecker) Check() (bool, error) {
+func (c *fakeClusterUpgradeChecker) Check([]v1.Node) (bool, error) {
 	return c.Upgrading, c.Err
 }