Merge pull request #362 from mshitrit/support-hcp-upgrade

Support hcp upgrade

Author: openshift-merge-bot[bot]

controllers/cluster/upgrade_checker.go

@@ -2,11 +2,11 @@ package cluster
 
 import (
 	"context"
-	"errors"
 
 	"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"
@@ -15,16 +15,19 @@ import (
 	clusterversion "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
 )
 
-var unsupportedUpgradeCheckerErr = errors.New(
-	"the cluster doesn't have any upgrade state representation." +
-		" Currently only OpenShift/OKD is supported")
+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 {
@@ -36,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")
@@ -52,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

@@ -249,7 +249,7 @@ func (r *NodeHealthCheckReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	updateRequeueAfter(&result, requeueAfter)
 
 	// TODO consider setting Disabled condition?
-	if r.isClusterUpgrading() {
+	if r.isClusterUpgrading(matchingNodes) {
 		msg := "Postponing potential remediations because of ongoing cluster upgrade"
 		log.Info(msg)
 		commonevents.NormalEvent(r.Recorder, nhc, utils.EventReasonRemediationSkipped, msg)
@@ -388,8 +388,8 @@ func (r *NodeHealthCheckReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	return result, nil
 }
 
-func (r *NodeHealthCheckReconciler) isClusterUpgrading() bool {
-	clusterUpgrading, err := r.ClusterUpgradeStatusChecker.Check()
+func (r *NodeHealthCheckReconciler) isClusterUpgrading(nodesToBeRemediated []v1.Node) bool {
+	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.

controllers/nodehealthcheck_controller_test.go

@@ -1689,38 +1689,95 @@ var _ = Describe("Node Health Check CR", func() {
 		When("Nodes are candidates for remediation and cluster is upgrading", func() {
 			BeforeEach(func() {
 				clusterUpgradeRequeueAfter = 5 * time.Second
-				upgradeChecker.Upgrading = true
 				setupObjects(1, 2, true)
 			})
+			When("non HCP Upgrade", func() {
+				BeforeEach(func() {
+					upgradeChecker.Upgrading = true
+				})
 
-			AfterEach(func() {
-				upgradeChecker.Upgrading = false
-			})
+				AfterEach(func() {
+					upgradeChecker.Upgrading = false
+				})
 
-			It("doesn't not remediate but requeues reconciliation and updates status", func() {
-				cr := findRemediationCRForNHC(unhealthyNodeName, underTest)
-				Expect(cr).To(BeNil())
+				It("doesn't not remediate but requeues reconciliation and updates status", func() {
+					cr := findRemediationCRForNHC(unhealthyNodeName, underTest)
+					Expect(cr).To(BeNil())
 
-				Expect(*underTest.Status.HealthyNodes).To(Equal(0))
-				Expect(*underTest.Status.ObservedNodes).To(Equal(0))
-				Expect(underTest.Status.UnhealthyNodes).To(BeEmpty())
-				Expect(underTest.Status.Phase).To(Equal(v1alpha1.PhaseEnabled))
-				Expect(underTest.Status.Reason).ToNot(BeEmpty())
+					Expect(*underTest.Status.HealthyNodes).To(Equal(0))
+					Expect(*underTest.Status.ObservedNodes).To(Equal(0))
+					Expect(underTest.Status.UnhealthyNodes).To(BeEmpty())
+					Expect(underTest.Status.Phase).To(Equal(v1alpha1.PhaseEnabled))
+					Expect(underTest.Status.Reason).ToNot(BeEmpty())
 
-				By("stopping upgrade and waiting for requeue")
-				upgradeChecker.Upgrading = false
-				time.Sleep(10 * time.Second)
-				cr = findRemediationCRForNHC(unhealthyNodeName, underTest)
-				Expect(cr).ToNot(BeNil())
+					By("stopping upgrade and waiting for requeue")
+					upgradeChecker.Upgrading = false
+					time.Sleep(10 * time.Second)
+					cr = findRemediationCRForNHC(unhealthyNodeName, underTest)
+					Expect(cr).ToNot(BeNil())
+
+					Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(underTest), underTest)).To(Succeed())
+					Expect(*underTest.Status.HealthyNodes).To(Equal(2))
+					Expect(*underTest.Status.ObservedNodes).To(Equal(3))
+					Expect(underTest.Status.UnhealthyNodes).To(HaveLen(1))
+				})
 
-				Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(underTest), underTest)).To(Succeed())
-				Expect(*underTest.Status.HealthyNodes).To(Equal(2))
-				Expect(*underTest.Status.ObservedNodes).To(Equal(3))
-				Expect(underTest.Status.UnhealthyNodes).To(HaveLen(1))
 			})
+			When("HCP Upgrade", func() {
+				var currentMachineConfigAnnotationKey = "machineconfiguration.openshift.io/currentConfig"
+				var desiredMachineConfigAnnotationKey = "machineconfiguration.openshift.io/desiredConfig"
+				BeforeEach(func() {
+					//Use a real upgrade checker instead of mock
+					prevUpgradeChecker := nhcReconciler.ClusterUpgradeStatusChecker
+					nhcReconciler.ClusterUpgradeStatusChecker = ocpUpgradeChecker
+					DeferCleanup(func() {
+						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(unhealthyNodeName, underTest)
+					Expect(cr).To(BeNil())
+
+					Expect(*underTest.Status.HealthyNodes).To(Equal(0))
+					Expect(*underTest.Status.ObservedNodes).To(Equal(0))
+					Expect(underTest.Status.UnhealthyNodes).To(BeEmpty())
+					Expect(underTest.Status.Phase).To(Equal(v1alpha1.PhaseEnabled))
+					Expect(underTest.Status.Reason).ToNot(BeEmpty())
 
+					By("stopping upgrade and waiting for requeue")
+					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(unhealthyNodeName, underTest)
+					Expect(cr).ToNot(BeNil())
+
+					Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(underTest), underTest)).To(Succeed())
+					Expect(*underTest.Status.HealthyNodes).To(Equal(2))
+					Expect(*underTest.Status.ObservedNodes).To(Equal(3))
+					Expect(underTest.Status.UnhealthyNodes).To(HaveLen(1))
+				})
+
+			})
+		})
 		Context("Machine owners", func() {
 			When("Metal3RemediationTemplate is in correct namespace", func() {
 

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
 }