Merge pull request #1287 from theobarberbany/featuregated-machineset-paused-condition

OCPCLOUD-2565: Adds paused condition to MachineSet

Author: openshift-merge-bot[bot]

go.mod

@@ -29,7 +29,7 @@ require (
 	k8s.io/kubectl v0.30.0
 	k8s.io/utils v0.0.0-20240310230437-4693a0247e57
 	sigs.k8s.io/cluster-api v1.6.1
-	sigs.k8s.io/controller-runtime v0.18.3
+	sigs.k8s.io/controller-runtime v0.18.4
 	sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20240522175850-2e9781e9fc60
 	sigs.k8s.io/yaml v1.4.0
 )

go.sum

@@ -778,8 +778,8 @@ mvdan.cc/unparam v0.0.0-20240427195214-063aff900ca1 h1:Nykk7fggxChwLK4rUPYESzeIw
 mvdan.cc/unparam v0.0.0-20240427195214-063aff900ca1/go.mod h1:ZzZjEpJDOmx8TdVU6umamY3Xy0UAQUI2DHbf05USVbI=
 sigs.k8s.io/cluster-api v1.6.1 h1:I34p/fwgRlEhs+o9cUhKXDwNNfPS3no0yJsd2bJyQVc=
 sigs.k8s.io/cluster-api v1.6.1/go.mod h1:DaxwruDvSaEYq5q6FREDaGzX6UsAVUCA99Sp8vfMHyQ=
-sigs.k8s.io/controller-runtime v0.18.3 h1:B5Wmmo8WMWK7izei+2LlXLVDGzMwAHBNLX68lwtlSR4=
-sigs.k8s.io/controller-runtime v0.18.3/go.mod h1:TVoGrfdpbA9VRFaRnKgk9P5/atA0pMwq+f+msb9M8Sg=
+sigs.k8s.io/controller-runtime v0.18.4 h1:87+guW1zhvuPLh1PHybKdYFLU0YJp4FhJRmiHvm5BZw=
+sigs.k8s.io/controller-runtime v0.18.4/go.mod h1:TVoGrfdpbA9VRFaRnKgk9P5/atA0pMwq+f+msb9M8Sg=
 sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20240522175850-2e9781e9fc60 h1:ihaeBTCFuEYPL1T1/FqAavDY7z5UcKSnWpnb+I3DYeM=
 sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20240522175850-2e9781e9fc60/go.mod h1:4+4tM2Es0ycqPedATtzPer5RTrUq3Xab59BYogt0mCE=
 sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo=

pkg/controller/machine/machine_controller_test.go

@@ -31,8 +31,6 @@ import (
 	"github.com/openshift/cluster-control-plane-machine-set-operator/test/e2e/framework"
 	testutils "github.com/openshift/machine-api-operator/pkg/util/testing"
 
-	apierrs "k8s.io/apimachinery/pkg/api/errors"
-	utilnet "k8s.io/apimachinery/pkg/util/net"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest/komega"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -144,11 +142,6 @@ var _ = Describe("Machine Reconciler", func() {
 			HaveField("Status", Equal(corev1.ConditionTrue)),
 		))))
 
-		By("Transitioning the AuthoritativeAPI though 'Migrating' to MachineAPI")
-		Eventually(k.UpdateStatus(instance, func() {
-			instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMigrating
-		})).Should(Succeed())
-
 		// The condition should remain true whilst transitioning through 'Migrating'
 		// Run this in a goroutine so we don't block
 
@@ -164,7 +157,7 @@ var _ = Describe("Machine Reconciler", func() {
 
 					localInstance := instanceCopy.DeepCopy()
 					if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(localInstance), localInstance); err != nil {
-						return g.Expect(err).Should(WithTransform(isRetryableAPIError, BeTrue()), "expected temporary error while getting machine: %v", err)
+						return g.Expect(err).Should(WithTransform(testutils.IsRetryableAPIError, BeTrue()), "expected temporary error while getting machine: %v", err)
 					}
 
 					return g.Expect(localInstance.Status.Conditions).Should(ContainElement(SatisfyAll(
@@ -179,13 +172,18 @@ var _ = Describe("Machine Reconciler", func() {
 
 					localInstance := instanceCopy.DeepCopy()
 					if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(localInstance), localInstance); err != nil {
-						return g.Expect(err).Should(WithTransform(isRetryableAPIError, BeTrue()), "expected temporary error while getting machine: %v", err)
+						return g.Expect(err).Should(WithTransform(testutils.IsRetryableAPIError, BeTrue()), "expected temporary error while getting machine: %v", err)
 					}
 
-					return g.Expect(localInstance.Status.AuthoritativeAPI).ToNot(Equal(machinev1.MachineAuthorityMachineAPI))
+					return g.Expect(localInstance.Status.AuthoritativeAPI).To(Equal(machinev1.MachineAuthorityMachineAPI))
 				})
 		}()
 
+		By("Transitioning the AuthoritativeAPI though 'Migrating' to MachineAPI")
+		Eventually(k.UpdateStatus(instance, func() {
+			instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMigrating
+		})).Should(Succeed())
+
 		By("Updating the AuthoritativeAPI from Migrating to MachineAPI")
 		Eventually(k.UpdateStatus(instance, func() {
 			instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
@@ -220,21 +218,3 @@ func cleanResources(namespace string) error {
 
 	return nil
 }
-
-// isRetryableAPIError returns whether an API error is retryable or not.
-// inspired by: k8s.io/kubernetes/test/utils.
-func isRetryableAPIError(err error) bool {
-	// These errors may indicate a transient error that we can retry in tests.
-	if apierrs.IsInternalError(err) || apierrs.IsTimeout(err) || apierrs.IsServerTimeout(err) ||
-		apierrs.IsTooManyRequests(err) || utilnet.IsProbableEOF(err) || utilnet.IsConnectionReset(err) ||
-		utilnet.IsHTTP2ConnectionLost(err) {
-		return true
-	}
-
-	// If the error sends the Retry-After header, we respect it as an explicit confirmation we should retry.
-	if _, shouldRetry := apierrs.SuggestsClientDelay(err); shouldRetry {
-		return true
-	}
-
-	return false
-}

pkg/controller/machineset/controller.go

@@ -25,8 +25,10 @@ import (
 	"sync"
 	"time"
 
+	openshiftfeatures "github.com/openshift/api/features"
 	machinev1 "github.com/openshift/api/machine/v1beta1"
 	"github.com/openshift/machine-api-operator/pkg/util"
+	"github.com/openshift/machine-api-operator/pkg/util/conditions"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -45,6 +47,14 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/source"
 )
 
+const (
+	PausedCondition machinev1.ConditionType = "Paused"
+
+	PausedConditionReason = "AuthoritativeAPI is not set to MachineAPI"
+
+	NotPausedConditionReason = "AuthoritativeAPI is set to MachineAPI"
+)
+
 var (
 	controllerKind = machinev1.SchemeGroupVersion.WithKind("MachineSet")
 
@@ -170,6 +180,42 @@ func (r *ReconcileMachineSet) Reconcile(ctx context.Context, request reconcile.R
 		return reconcile.Result{}, nil
 	}
 
+	if r.gate.Enabled(featuregate.Feature(openshiftfeatures.FeatureGateMachineAPIMigration)) {
+		machineSetCopy := machineSet.DeepCopy()
+		// Check Status.AuthoritativeAPI. If it's not set to MachineAPI. Set the
+		// paused condition true and return early.
+		//
+		// Once we have a webhook, we want to remove the check that the AuthoritativeAPI
+		// field is populated.
+		if machineSet.Status.AuthoritativeAPI != "" &&
+			machineSet.Status.AuthoritativeAPI != machinev1.MachineAuthorityMachineAPI {
+			conditions.Set(machineSetCopy, conditions.TrueConditionWithReason(
+				PausedCondition,
+				PausedConditionReason,
+				"The AuthoritativeAPI is set to %s", machineSet.Status.AuthoritativeAPI,
+			))
+
+			_, err := updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
+			if err != nil {
+				klog.Errorf("%v: error updating status: %v", machineSet.Name, err)
+			}
+
+			klog.Infof("%v: machine is paused, taking no further action", machineSet.Name)
+			return reconcile.Result{}, nil
+		}
+
+		conditions.Set(machineSetCopy, conditions.FalseCondition(
+			PausedCondition,
+			NotPausedConditionReason,
+			"The AuthoritativeAPI is set to %s", string(machineSet.Status.AuthoritativeAPI),
+		))
+		_, err := updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
+		if err != nil {
+			klog.Errorf("%v: error updating status: %v", machineSet.Name, err)
+		}
+		klog.Infof("%v: setting paused to false and continuing reconcile", machineSet.Name)
+	}
+
 	result, err := r.reconcile(ctx, machineSet)
 	if err != nil {
 		klog.Errorf("Failed to reconcile MachineSet %q: %v", request.NamespacedName, err)

pkg/controller/machineset/controller_test.go

@@ -57,6 +57,9 @@ func TestMachineSetToMachines(t *testing.T) {
 						},
 					},
 				},
+				Status: machinev1.MachineSetStatus{
+					AuthoritativeAPI: machinev1.MachineAuthorityMachineAPI,
+				},
 			},
 		},
 	}
@@ -153,7 +156,11 @@ func TestShouldExcludeMachine(t *testing.T) {
 		expected   bool
 	}{
 		{
-			machineSet: machinev1.MachineSet{},
+			machineSet: machinev1.MachineSet{
+				Status: machinev1.MachineSetStatus{
+					AuthoritativeAPI: machinev1.MachineAuthorityMachineAPI,
+				},
+			},
 			machine: machinev1.Machine{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "withNoMatchingOwnerRef",
@@ -178,6 +185,9 @@ func TestShouldExcludeMachine(t *testing.T) {
 						},
 					},
 				},
+				Status: machinev1.MachineSetStatus{
+					AuthoritativeAPI: machinev1.MachineAuthorityMachineAPI,
+				},
 			},
 			machine: machinev1.Machine{
 				ObjectMeta: metav1.ObjectMeta{
@@ -191,7 +201,11 @@ func TestShouldExcludeMachine(t *testing.T) {
 			expected: false,
 		},
 		{
-			machineSet: machinev1.MachineSet{},
+			machineSet: machinev1.MachineSet{
+				Status: machinev1.MachineSetStatus{
+					AuthoritativeAPI: machinev1.MachineAuthorityMachineAPI,
+				},
+			},
 			machine: machinev1.Machine{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:              "withDeletionTimestamp",
@@ -225,6 +239,9 @@ func TestAdoptOrphan(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "adoptOrphanMachine",
 		},
+		Status: machinev1.MachineSetStatus{
+			AuthoritativeAPI: machinev1.MachineAuthorityMachineAPI,
+		},
 	}
 	controller := true
 	blockOwnerDeletion := true
@@ -306,7 +323,11 @@ var _ = Describe("MachineSet Reconcile", func() {
 				},
 				Spec: machinev1.MachineSetSpec{
 					Template: machinev1.MachineTemplateSpec{},
-				}}
+				},
+				Status: machinev1.MachineSetStatus{
+					AuthoritativeAPI: machinev1.MachineAuthorityMachineAPI,
+				},
+			}
 
 			r.Client = fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(ms).WithStatusSubresource(&machinev1.MachineSet{}).Build()
 		})
@@ -335,6 +356,9 @@ var _ = Describe("MachineSet Reconcile", func() {
 				Spec: machinev1.MachineSetSpec{
 					Replicas: &replicas,
 				},
+				Status: machinev1.MachineSetStatus{
+					AuthoritativeAPI: machinev1.MachineAuthorityMachineAPI,
+				},
 			}
 
 			ms.Spec.Selector.MatchLabels = map[string]string{

pkg/controller/machineset/machineset_controller_suite_test.go

@@ -19,13 +19,17 @@ package machineset
 import (
 	"context"
 	"flag"
+	"log"
+	"os"
 	"path/filepath"
 	"testing"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	configv1 "github.com/openshift/api/config/v1"
 	machinev1 "github.com/openshift/api/machine/v1beta1"
+
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"k8s.io/klog/v2/textlogger"
@@ -38,9 +42,6 @@ func init() {
 	textLoggerConfig := textlogger.NewConfig()
 	textLoggerConfig.AddFlags(flag.CommandLine)
 	logf.SetLogger(textlogger.NewLogger(textLoggerConfig))
-
-	// Register required object kinds with global scheme.
-	_ = machinev1.Install(scheme.Scheme)
 }
 
 const (
@@ -62,18 +63,38 @@ func TestMachinesetController(t *testing.T) {
 
 var _ = BeforeSuite(func(ctx SpecContext) {
 	By("bootstrapping test environment")
+
 	testEnv = &envtest.Environment{
-		CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "install")},
+
+		CRDInstallOptions: envtest.CRDInstallOptions{
+			Paths: []string{
+				filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "machine", "v1beta1", "zz_generated.crd-manifests", "0000_10_machine-api_01_machines-CustomNoUpgrade.crd.yaml"),
+				filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "machine", "v1beta1", "zz_generated.crd-manifests", "0000_10_machine-api_01_machinesets-CustomNoUpgrade.crd.yaml"),
+				filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests", "0000_00_cluster-version-operator_01_clusterversions-CustomNoUpgrade.crd.yaml"),
+				filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1", "zz_generated.crd-manifests", "0000_10_config-operator_01_featuregates.crd.yaml"),
+			},
+		},
 	}
 
 	var err error
 	cfg, err = testEnv.Start()
 	Expect(err).ToNot(HaveOccurred())
 	Expect(cfg).ToNot(BeNil())
-
 })
 
 var _ = AfterSuite(func() {
 	By("tearing down the test environment")
 	Expect(testEnv.Stop()).To(Succeed())
 })
+
+func TestMain(m *testing.M) {
+	// Register required object kinds with global scheme.
+	if err := machinev1.Install(scheme.Scheme); err != nil {
+		log.Fatalf("cannot add scheme: %v", err)
+	}
+	if err := configv1.Install(scheme.Scheme); err != nil {
+		log.Fatalf("cannot add scheme: %v", err)
+	}
+	exitVal := m.Run()
+	os.Exit(exitVal)
+}