diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go index 8ade9cb8fc8..979f15b74fd 100644 --- a/cmd/aro/operator.go +++ b/cmd/aro/operator.go @@ -46,6 +46,7 @@ import ( "github.com/Azure/ARO-RP/pkg/operator/controllers/storageaccounts" "github.com/Azure/ARO-RP/pkg/operator/controllers/subnets" "github.com/Azure/ARO-RP/pkg/operator/controllers/workaround" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" "github.com/Azure/ARO-RP/pkg/util/dynamichelper" utillog "github.com/Azure/ARO-RP/pkg/util/log" // +kubebuilder:scaffold:imports @@ -94,6 +95,8 @@ func operator(ctx context.Context, log *logrus.Entry) error { return err } + ch := clienthelper.NewWithClient(log, client) + if role == pkgoperator.RoleMaster { if err = (genevalogging.NewReconciler( log.WithField("controller", genevalogging.ControllerName), @@ -137,17 +140,17 @@ func operator(ctx context.Context, log *logrus.Entry) error { } if err = (dnsmasq.NewClusterReconciler( log.WithField("controller", dnsmasq.ClusterControllerName), - client, dh)).SetupWithManager(mgr); err != nil { + client, ch)).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create controller %s: %v", dnsmasq.ClusterControllerName, err) } if err = (dnsmasq.NewMachineConfigReconciler( log.WithField("controller", dnsmasq.MachineConfigControllerName), - client, dh)).SetupWithManager(mgr); err != nil { + client, ch)).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create controller %s: %v", dnsmasq.MachineConfigControllerName, err) } if err = (dnsmasq.NewMachineConfigPoolReconciler( log.WithField("controller", dnsmasq.MachineConfigPoolControllerName), - client, dh)).SetupWithManager(mgr); err != nil { + client, ch)).SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create controller %s: %v", dnsmasq.MachineConfigPoolControllerName, err) } if err = (node.NewReconciler( diff --git a/pkg/cluster/arooperator.go b/pkg/cluster/arooperator.go index 810c461c838..35289901aa8 100644 --- a/pkg/cluster/arooperator.go +++ b/pkg/cluster/arooperator.go @@ -56,6 +56,22 @@ func (m *manager) syncClusterObject(ctx context.Context) error { return err } +func (m *manager) enableOperatorReconciliation(ctx context.Context) error { + err := m.aroOperatorDeployer.SetForceReconcile(ctx, true) + if err != nil { + m.log.Error(fmt.Errorf("cannot ensureAROOperator.SetForceReconcile: %w", err)) + } + return err +} + +func (m *manager) disableOperatorReconciliation(ctx context.Context) error { + err := m.aroOperatorDeployer.SetForceReconcile(ctx, false) + if err != nil { + m.log.Error(fmt.Errorf("cannot ensureAROOperator.SetForceReconcile: %w", err)) + } + return err +} + func (m *manager) aroDeploymentReady(ctx context.Context) (bool, error) { if !m.isIngressProfileAvailable() { // If the ingress profile is not available, ARO operator update/deploy will fail. diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index 288932e405a..d52a8aff683 100644 --- a/pkg/cluster/install.go +++ b/pkg/cluster/install.go @@ -360,6 +360,7 @@ func (m *manager) bootstrap() []steps.Step { steps.Action(m.initializeOperatorDeployer), // depends on kube clients steps.Condition(m.apiServersReady, 30*time.Minute, true), steps.Action(m.installAROOperator), + steps.Action(m.enableOperatorReconciliation), steps.Action(m.incrInstallPhase), ) @@ -393,6 +394,7 @@ func (m *manager) Install(ctx context.Context) error { steps.Action(m.configureIngressCertificate), steps.Condition(m.ingressControllerReady, 30*time.Minute, true), steps.Action(m.configureDefaultStorageClass), + steps.Action(m.disableOperatorReconciliation), steps.Action(m.finishInstallation), }, } diff --git a/pkg/operator/controllers/base/aro_controller.go b/pkg/operator/controllers/base/aro_controller.go index 04852881084..47a8b03f80c 100644 --- a/pkg/operator/controllers/base/aro_controller.go +++ b/pkg/operator/controllers/base/aro_controller.go @@ -5,7 +5,9 @@ package base import ( "context" + "fmt" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" "github.com/openshift/library-go/pkg/operator/v1helpers" "github.com/sirupsen/logrus" @@ -13,7 +15,9 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/Azure/ARO-RP/pkg/operator" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" + "github.com/Azure/ARO-RP/pkg/util/version" ) type AROController struct { @@ -103,3 +107,31 @@ func (c *AROController) defaultDegraded() *operatorv1.OperatorCondition { func (c *AROController) conditionName(conditionType string) string { return c.Name + "Controller" + conditionType } + +func (c *AROController) IsClusterUpgrading(ctx context.Context) (bool, error) { + cv := &configv1.ClusterVersion{} + err := c.Client.Get(ctx, types.NamespacedName{Name: "version"}, cv) + if err != nil { + err = fmt.Errorf("error getting the ClusterVersion: %w", err) + c.Log.Error(err) + return false, err + } + + return version.IsClusterUpgrading(cv), nil +} + +func (c *AROController) AllowRebootCausingReconciliation(ctx context.Context, instance *arov1alpha1.Cluster) (bool, error) { + // If reconciliation is forced, perform it + if instance.Spec.OperatorFlags.GetSimpleBoolean(operator.ForceReconciliation) { + c.Log.Debugf("allowing reconciliation of %s because reconciliation forced", c.Name) + return true, nil + } + + // Allow the reconciliation if the cluster is upgrading + isClusterUpgrading, err := c.IsClusterUpgrading(ctx) + if err != nil { + return false, err + } + + return isClusterUpgrading, nil +} diff --git a/pkg/operator/controllers/dnsmasq/cluster_controller.go b/pkg/operator/controllers/dnsmasq/cluster_controller.go index 1fd4b201de3..9d6167fc447 100644 --- a/pkg/operator/controllers/dnsmasq/cluster_controller.go +++ b/pkg/operator/controllers/dnsmasq/cluster_controller.go @@ -5,20 +5,26 @@ package dnsmasq import ( "context" + "fmt" + configv1 "github.com/openshift/api/config/v1" mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" "github.com/sirupsen/logrus" + kerrors "k8s.io/apimachinery/pkg/api/errors" kruntime "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/source" "github.com/Azure/ARO-RP/pkg/operator" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" "github.com/Azure/ARO-RP/pkg/operator/controllers/base" "github.com/Azure/ARO-RP/pkg/operator/predicates" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" "github.com/Azure/ARO-RP/pkg/util/dynamichelper" ) @@ -28,22 +34,22 @@ const ( type ClusterReconciler struct { base.AROController - dh dynamichelper.Interface + ch clienthelper.Interface } -func NewClusterReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *ClusterReconciler { +func NewClusterReconciler(log *logrus.Entry, client client.Client, ch clienthelper.Interface) *ClusterReconciler { return &ClusterReconciler{ AROController: base.AROController{ Log: log, Client: client, Name: ClusterControllerName, }, - dh: dh, + ch: ch, } } -// Reconcile watches the ARO object, and if it changes, reconciles all the -// 99-%s-aro-dns machineconfigs +// Reconcile watches the ARO object and ClusterVersion, and if they change, +// reconciles all the 99-%s-aro-dns machineconfigs func (r *ClusterReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { instance, err := r.GetCluster(ctx) if err != nil { @@ -60,6 +66,13 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, request ctrl.Request) r.Log.Debug("restartDnsmasq is enabled") } + allowReconcile, err := r.AllowRebootCausingReconciliation(ctx, instance) + if err != nil { + r.Log.Error(err) + r.SetDegraded(ctx, err) + return reconcile.Result{}, err + } + r.Log.Debug("running") mcps := &mcv1.MachineConfigPoolList{} err = r.Client.List(ctx, mcps) @@ -69,7 +82,7 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, request ctrl.Request) return reconcile.Result{}, err } - err = reconcileMachineConfigs(ctx, instance, r.dh, restartDnsmasq, mcps.Items...) + err = reconcileMachineConfigs(ctx, instance, r.ch, r.Client, allowReconcile, restartDnsmasq, mcps.Items...) if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) @@ -82,13 +95,22 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, request ctrl.Request) // SetupWithManager setup our mananger func (r *ClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { + clusterVersionPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool { + return o.GetName() == "version" + }) + return ctrl.NewControllerManagedBy(mgr). For(&arov1alpha1.Cluster{}, builder.WithPredicates(predicate.And(predicates.AROCluster, predicate.GenerationChangedPredicate{}))). Named(ClusterControllerName). + Watches( + &source.Kind{Type: &configv1.ClusterVersion{}}, + &handler.EnqueueRequestForObject{}, + builder.WithPredicates(clusterVersionPredicate), + ). Complete(r) } -func reconcileMachineConfigs(ctx context.Context, instance *arov1alpha1.Cluster, dh dynamichelper.Interface, restartDnsmasq bool, mcps ...mcv1.MachineConfigPool) error { +func reconcileMachineConfigs(ctx context.Context, instance *arov1alpha1.Cluster, ch clienthelper.Interface, c client.Client, allowReconcile bool, restartDnsmasq bool, mcps ...mcv1.MachineConfigPool) error { var resources []kruntime.Object for _, mcp := range mcps { resource, err := dnsmasqMachineConfig(instance.Spec.Domain, instance.Spec.APIIntIP, instance.Spec.IngressIP, mcp.Name, instance.Spec.GatewayDomains, instance.Spec.GatewayPrivateEndpointIP, restartDnsmasq) @@ -109,5 +131,19 @@ func reconcileMachineConfigs(ctx context.Context, instance *arov1alpha1.Cluster, return err } - return dh.Ensure(ctx, resources...) + // If we are allowed to reconcile the resources, then we run Ensure to + // create or update. If we are not allowed to reconcile, we do not want to + // perform any updates, but we do want to perform initial configuration. + if allowReconcile { + return ch.Ensure(ctx, resources...) + } else { + for _, i := range resources { + err := c.Create(ctx, i.(client.Object)) + // Since we are only creating, ignore AlreadyExists + if err != nil && !kerrors.IsAlreadyExists(err) { + return fmt.Errorf("error creating client object: %w", err) + } + } + } + return nil } diff --git a/pkg/operator/controllers/dnsmasq/cluster_controller_test.go b/pkg/operator/controllers/dnsmasq/cluster_controller_test.go index 3239641acf8..f7cb499dc13 100644 --- a/pkg/operator/controllers/dnsmasq/cluster_controller_test.go +++ b/pkg/operator/controllers/dnsmasq/cluster_controller_test.go @@ -8,7 +8,7 @@ import ( "testing" "time" - "github.com/golang/mock/gomock" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" "github.com/sirupsen/logrus" @@ -19,7 +19,8 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" - mock_dynamichelper "github.com/Azure/ARO-RP/pkg/util/mocks/dynamichelper" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" + testclienthelper "github.com/Azure/ARO-RP/test/util/clienthelper" utilconditions "github.com/Azure/ARO-RP/test/util/conditions" utilerror "github.com/Azure/ARO-RP/test/util/error" ) @@ -31,24 +32,22 @@ func TestClusterReconciler(t *testing.T) { defaultDegraded := utilconditions.ControllerDefaultDegraded(ClusterControllerName) defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} - fakeDh := func(controller *gomock.Controller) *mock_dynamichelper.MockInterface { - return mock_dynamichelper.NewMockInterface(controller) - } - tests := []struct { name string objects []client.Object - mocks func(mdh *mock_dynamichelper.MockInterface) + wantCreated map[string]int + wantUpdated map[string]int request ctrl.Request wantErrMsg string wantConditions []operatorv1.OperatorCondition }{ { - name: "no cluster", - objects: []client.Object{}, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, - request: ctrl.Request{}, - wantErrMsg: "clusters.aro.openshift.io \"cluster\" not found", + name: "no cluster", + objects: []client.Object{}, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: "clusters.aro.openshift.io \"cluster\" not found", }, { name: "controller disabled", @@ -65,7 +64,8 @@ func TestClusterReconciler(t *testing.T) { }, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, request: ctrl.Request{}, wantErrMsg: "", wantConditions: defaultConditions, @@ -86,22 +86,214 @@ func TestClusterReconciler(t *testing.T) { }, }, }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + operator.ForceReconciliation: operator.FlagTrue, + }, + }, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "valid MachineConfigPool creates ARO DNS MachineConfig, forced reconciliation", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + operator.ForceReconciliation: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "master"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + }, + wantCreated: map[string]int{ + "MachineConfig//99-master-aro-dns": 1, + }, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "missing a clusterversion fails", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "master"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: `error getting the ClusterVersion: clusterversions.config.openshift.io "version" not found`, + wantConditions: []operatorv1.OperatorCondition{ + defaultAvailable, defaultProgressing, { + Type: "DnsmasqClusterControllerDegraded", + Status: "True", + Message: `error getting the ClusterVersion: clusterversions.config.openshift.io "version" not found`, + LastTransitionTime: transitionTime, + }, + }, + }, + { + name: "valid MachineConfigPool, cluster not updating, not forced, does create", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "master"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + { + State: configv1.CompletedUpdate, + Version: "4.10.11", + }, + }, + }, + }, + }, + wantCreated: map[string]int{ + "MachineConfig//99-master-aro-dns": 1, + }, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "valid MachineConfigPool, cluster not updating, not forced, does not update", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "master"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-master-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, + &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + { + State: configv1.CompletedUpdate, + Version: "4.10.11", + }, + }, + }, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{}, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "valid MachineConfigPool, while cluster updating, updates existing ARO DNS MachineConfig", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ operator.DnsmasqEnabled: operator.FlagTrue, }, }, }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "master"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-master-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, + &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Spec: configv1.ClusterVersionSpec{}, + Status: configv1.ClusterVersionStatus{ + Conditions: []configv1.ClusterOperatorStatusCondition{ + { + Type: configv1.OperatorProgressing, + Status: configv1.ConditionTrue, + }, + }, + }, + }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) { - mdh.EXPECT().Ensure(gomock.Any()).Times(1) + wantCreated: map[string]int{}, + wantUpdated: map[string]int{ + "MachineConfig//99-master-aro-dns": 1, }, request: ctrl.Request{}, wantErrMsg: "", wantConditions: defaultConditions, }, { - name: "valid MachineConfigPool creates ARO DNS MachineConfig", + name: "valid MachineConfigPool, while cluster updating, creates ARO DNS MachineConfig", objects: []client.Object{ &arov1alpha1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, @@ -119,10 +311,25 @@ func TestClusterReconciler(t *testing.T) { Status: mcv1.MachineConfigPoolStatus{}, Spec: mcv1.MachineConfigPoolSpec{}, }, + &configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Spec: configv1.ClusterVersionSpec{}, + Status: configv1.ClusterVersionStatus{ + Conditions: []configv1.ClusterOperatorStatusCondition{ + { + Type: configv1.OperatorProgressing, + Status: configv1.ConditionTrue, + }, + }, + }, + }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) { - mdh.EXPECT().Ensure(gomock.Any(), gomock.AssignableToTypeOf(&mcv1.MachineConfig{})).Times(1) + wantCreated: map[string]int{ + "MachineConfig//99-master-aro-dns": 1, }, + wantUpdated: map[string]int{}, request: ctrl.Request{}, wantErrMsg: "", wantConditions: defaultConditions, @@ -131,26 +338,42 @@ func TestClusterReconciler(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - controller := gomock.NewController(t) - defer controller.Finish() + createTally := make(map[string]int) + updateTally := make(map[string]int) - client := ctrlfake.NewClientBuilder(). + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). WithObjects(tt.objects...). - Build() + Build()).WithPostCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithPostUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) - dh := fakeDh(controller) - tt.mocks(dh) + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) r := NewClusterReconciler( - logrus.NewEntry(logrus.StandardLogger()), + log, client, - dh, + ch, ) ctx := context.Background() _, err := r.Reconcile(ctx, tt.request) utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) utilconditions.AssertControllerConditions(t, ctx, client, tt.wantConditions) + + e, err := testclienthelper.CompareTally(tt.wantCreated, createTally) + if err != nil { + t.Errorf("create comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + + e, err = testclienthelper.CompareTally(tt.wantUpdated, updateTally) + if err != nil { + t.Errorf("update comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } }) } } diff --git a/pkg/operator/controllers/dnsmasq/machineconfig_controller.go b/pkg/operator/controllers/dnsmasq/machineconfig_controller.go index 3f0fe88f71d..b045bbeede9 100644 --- a/pkg/operator/controllers/dnsmasq/machineconfig_controller.go +++ b/pkg/operator/controllers/dnsmasq/machineconfig_controller.go @@ -17,7 +17,7 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" "github.com/Azure/ARO-RP/pkg/operator/controllers/base" - "github.com/Azure/ARO-RP/pkg/util/dynamichelper" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" ) const ( @@ -27,19 +27,19 @@ const ( type MachineConfigReconciler struct { base.AROController - dh dynamichelper.Interface + ch clienthelper.Interface } var rxARODNS = regexp.MustCompile("^99-(.*)-aro-dns$") -func NewMachineConfigReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *MachineConfigReconciler { +func NewMachineConfigReconciler(log *logrus.Entry, client client.Client, ch clienthelper.Interface) *MachineConfigReconciler { return &MachineConfigReconciler{ AROController: base.AROController{ Log: log, Client: client, Name: MachineConfigControllerName, }, - dh: dh, + ch: ch, } } @@ -60,6 +60,13 @@ func (r *MachineConfigReconciler) Reconcile(ctx context.Context, request ctrl.Re r.Log.Debug("restart dnsmasq machineconfig enabled") } + allowReconcile, err := r.AllowRebootCausingReconciliation(ctx, instance) + if err != nil { + r.Log.Error(err) + r.SetDegraded(ctx, err) + return reconcile.Result{}, err + } + r.Log.Debug("running") m := rxARODNS.FindStringSubmatch(request.Name) if m == nil { @@ -82,7 +89,7 @@ func (r *MachineConfigReconciler) Reconcile(ctx context.Context, request ctrl.Re return reconcile.Result{}, nil } - err = reconcileMachineConfigs(ctx, instance, r.dh, instance.Spec.OperatorFlags.GetSimpleBoolean(operator.RestartDnsmasqEnabled), *mcp) + err = reconcileMachineConfigs(ctx, instance, r.ch, r.Client, allowReconcile, instance.Spec.OperatorFlags.GetSimpleBoolean(operator.RestartDnsmasqEnabled), *mcp) if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) diff --git a/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go b/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go index 318da59a958..266957c2b0a 100644 --- a/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go +++ b/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go @@ -8,7 +8,7 @@ import ( "testing" "time" - "github.com/golang/mock/gomock" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" "github.com/sirupsen/logrus" @@ -20,7 +20,8 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" - mock_dynamichelper "github.com/Azure/ARO-RP/pkg/util/mocks/dynamichelper" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" + testclienthelper "github.com/Azure/ARO-RP/test/util/clienthelper" utilconditions "github.com/Azure/ARO-RP/test/util/conditions" utilerror "github.com/Azure/ARO-RP/test/util/error" ) @@ -31,14 +32,10 @@ func TestMachineConfigReconciler(t *testing.T) { defaultProgressing := utilconditions.ControllerDefaultProgressing(MachineConfigControllerName) defaultDegraded := utilconditions.ControllerDefaultDegraded(MachineConfigControllerName) defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} - fakeDh := func(controller *gomock.Controller) *mock_dynamichelper.MockInterface { - return mock_dynamichelper.NewMockInterface(controller) - } tests := []struct { name string objects []client.Object - mocks func(mdh *mock_dynamichelper.MockInterface) request ctrl.Request wantErrMsg string wantConditions []operatorv1.OperatorCondition @@ -46,7 +43,6 @@ func TestMachineConfigReconciler(t *testing.T) { { name: "no cluster", objects: []client.Object{}, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, request: ctrl.Request{}, wantErrMsg: "clusters.aro.openshift.io \"cluster\" not found", }, @@ -65,13 +61,285 @@ func TestMachineConfigReconciler(t *testing.T) { }, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, request: ctrl.Request{}, wantErrMsg: "", wantConditions: defaultConditions, }, { - name: "no MachineConfigPool for MachineConfig does nothing", + name: "missing a clusterversion fails", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "master"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + }, + request: ctrl.Request{}, + wantErrMsg: `error getting the ClusterVersion: clusterversions.config.openshift.io "version" not found`, + wantConditions: []operatorv1.OperatorCondition{ + defaultAvailable, defaultProgressing, { + Type: MachineConfigControllerName + "ControllerDegraded", + Status: "True", + Message: `error getting the ClusterVersion: clusterversions.config.openshift.io "version" not found`, + LastTransitionTime: transitionTime, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + createTally := make(map[string]int) + updateTally := make(map[string]int) + + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). + WithObjects(tt.objects...). + Build()) + + client.WithPostCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithPostUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) + + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) + + r := NewMachineConfigReconciler( + logrus.NewEntry(logrus.StandardLogger()), + client, + ch, + ) + ctx := context.Background() + _, err := r.Reconcile(ctx, tt.request) + + utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) + utilconditions.AssertControllerConditions(t, ctx, client, tt.wantConditions) + + e, err := testclienthelper.CompareTally(map[string]int{}, createTally) + if err != nil { + t.Errorf("create comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + + e, err = testclienthelper.CompareTally(map[string]int{}, updateTally) + if err != nil { + t.Errorf("update comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + }) + } +} + +func TestMachineConfigReconcilerNotUpgrading(t *testing.T) { + defaultAvailable := utilconditions.ControllerDefaultAvailable(MachineConfigControllerName) + defaultProgressing := utilconditions.ControllerDefaultProgressing(MachineConfigControllerName) + defaultDegraded := utilconditions.ControllerDefaultDegraded(MachineConfigControllerName) + defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} + + tests := []struct { + name string + objects []client.Object + wantCreated map[string]int + wantUpdated map[string]int + request ctrl.Request + wantErrMsg string + wantConditions []operatorv1.OperatorCondition + }{ + { + name: "valid MachineConfigPool for MachineConfig creates MachineConfig, even if cluster not updating", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "custom"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + }, + wantCreated: map[string]int{ + "MachineConfig//99-custom-aro-dns": 1, + }, + wantUpdated: map[string]int{}, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "99-custom-aro-dns", + }, + }, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "valid MachineConfigPool for MachineConfig does not update existing MachineConfig while cluster not upgrading", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "custom"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-custom-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "99-custom-aro-dns", + }, + }, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "valid MachineConfigPool for MachineConfig updates existing MachineConfig when reconciliation forced", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + operator.ForceReconciliation: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "custom"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-custom-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{ + "MachineConfig//99-custom-aro-dns": 1, + }, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "99-custom-aro-dns", + }, + }, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + createTally := make(map[string]int) + updateTally := make(map[string]int) + + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). + WithObjects(tt.objects...). + WithObjects(&configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + { + State: configv1.CompletedUpdate, + Version: "4.10.11", + }, + }, + }, + }). + Build()) + + client.WithPostCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithPostUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) + + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) + + r := NewMachineConfigReconciler( + logrus.NewEntry(logrus.StandardLogger()), + client, + ch, + ) + ctx := context.Background() + _, err := r.Reconcile(ctx, tt.request) + + utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) + utilconditions.AssertControllerConditions(t, ctx, client, tt.wantConditions) + + e, err := testclienthelper.CompareTally(tt.wantCreated, createTally) + if err != nil { + t.Errorf("create comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + + e, err = testclienthelper.CompareTally(tt.wantUpdated, updateTally) + if err != nil { + t.Errorf("update comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + }) + } +} + +func TestMachineConfigReconcilerClusterUpgrading(t *testing.T) { + transitionTime := metav1.Time{Time: time.Now()} + defaultAvailable := utilconditions.ControllerDefaultAvailable(MachineConfigControllerName) + defaultProgressing := utilconditions.ControllerDefaultProgressing(MachineConfigControllerName) + defaultDegraded := utilconditions.ControllerDefaultDegraded(MachineConfigControllerName) + defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} + + tests := []struct { + name string + objects []client.Object + wantCreated map[string]int + wantUpdated map[string]int + request ctrl.Request + wantErrMsg string + wantConditions []operatorv1.OperatorCondition + }{ + { + name: "no MachineConfigPool for MachineConfig does nothing (cluster upgrading)", objects: []client.Object{ &arov1alpha1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, @@ -88,12 +356,46 @@ func TestMachineConfigReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - operator.DnsmasqEnabled: "true", + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "99-custom-aro-dns", + }, + }, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "valid MachineConfigPool for MachineConfig creates MachineConfig while cluster upgrading", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, }, }, }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{Name: "custom"}, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, + wantCreated: map[string]int{ + "MachineConfig//99-custom-aro-dns": 1, + }, + wantUpdated: map[string]int{}, request: ctrl.Request{ NamespacedName: types.NamespacedName{ Namespace: "", @@ -104,7 +406,7 @@ func TestMachineConfigReconciler(t *testing.T) { wantConditions: defaultConditions, }, { - name: "valid MachineConfigPool for MachineConfig reconciles MachineConfig", + name: "valid MachineConfigPool for MachineConfig updates existing MachineConfig while cluster upgrading", objects: []client.Object{ &arov1alpha1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, @@ -113,7 +415,7 @@ func TestMachineConfigReconciler(t *testing.T) { }, Spec: arov1alpha1.ClusterSpec{ OperatorFlags: arov1alpha1.OperatorFlags{ - operator.DnsmasqEnabled: "true", + operator.DnsmasqEnabled: operator.FlagTrue, }, }, }, @@ -122,9 +424,46 @@ func TestMachineConfigReconciler(t *testing.T) { Status: mcv1.MachineConfigPoolStatus{}, Spec: mcv1.MachineConfigPoolSpec{}, }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-custom-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{ + "MachineConfig//99-custom-aro-dns": 1, + }, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "99-custom-aro-dns", + }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) { - mdh.EXPECT().Ensure(gomock.Any(), gomock.AssignableToTypeOf(&mcv1.MachineConfig{})).Times(1) + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "changes for a deleted MachineConfigPool do nothing", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + DeletionTimestamp: &transitionTime, + }, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, }, request: ctrl.Request{ NamespacedName: types.NamespacedName{ @@ -132,6 +471,8 @@ func TestMachineConfigReconciler(t *testing.T) { Name: "99-custom-aro-dns", }, }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, wantErrMsg: "", wantConditions: defaultConditions, }, @@ -139,25 +480,58 @@ func TestMachineConfigReconciler(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - controller := gomock.NewController(t) - defer controller.Finish() + createTally := make(map[string]int) + updateTally := make(map[string]int) - client := ctrlfake.NewClientBuilder(). + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). WithObjects(tt.objects...). - Build() - dh := fakeDh(controller) - tt.mocks(dh) + WithObjects(&configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Spec: configv1.ClusterVersionSpec{}, + Status: configv1.ClusterVersionStatus{ + Conditions: []configv1.ClusterOperatorStatusCondition{ + { + Type: configv1.OperatorProgressing, + Status: configv1.ConditionTrue, + }, + }, + }, + }). + Build()) + + client.WithPostCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithPostUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) + + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) r := NewMachineConfigReconciler( logrus.NewEntry(logrus.StandardLogger()), client, - dh, + ch, ) ctx := context.Background() _, err := r.Reconcile(ctx, tt.request) utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) utilconditions.AssertControllerConditions(t, ctx, client, tt.wantConditions) + + e, err := testclienthelper.CompareTally(tt.wantCreated, createTally) + if err != nil { + t.Errorf("create comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + + e, err = testclienthelper.CompareTally(tt.wantUpdated, updateTally) + if err != nil { + t.Errorf("update comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } }) } } diff --git a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go index c1e9d511d0c..1f9932f1d5e 100644 --- a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go +++ b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller.go @@ -16,7 +16,7 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" "github.com/Azure/ARO-RP/pkg/operator/controllers/base" - "github.com/Azure/ARO-RP/pkg/util/dynamichelper" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" ) const ( @@ -26,17 +26,17 @@ const ( type MachineConfigPoolReconciler struct { base.AROController - dh dynamichelper.Interface + ch clienthelper.Interface } -func NewMachineConfigPoolReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *MachineConfigPoolReconciler { +func NewMachineConfigPoolReconciler(log *logrus.Entry, client client.Client, ch clienthelper.Interface) *MachineConfigPoolReconciler { return &MachineConfigPoolReconciler{ AROController: base.AROController{ Log: log, Client: client, Name: MachineConfigPoolControllerName, }, - dh: dh, + ch: ch, } } @@ -58,6 +58,13 @@ func (r *MachineConfigPoolReconciler) Reconcile(ctx context.Context, request ctr r.Log.Debug("restart dnsmasq machineconfig enabled") } + allowReconcile, err := r.AllowRebootCausingReconciliation(ctx, instance) + if err != nil { + r.Log.Error(err) + r.SetDegraded(ctx, err) + return reconcile.Result{}, err + } + r.Log.Debug("running") mcp := &mcv1.MachineConfigPool{} err = r.Client.Get(ctx, types.NamespacedName{Name: request.Name}, mcp) @@ -70,8 +77,11 @@ func (r *MachineConfigPoolReconciler) Reconcile(ctx context.Context, request ctr r.SetDegraded(ctx, err) return reconcile.Result{}, err } + if mcp.GetDeletionTimestamp() != nil { + return reconcile.Result{}, nil + } - err = reconcileMachineConfigs(ctx, instance, r.dh, restartDnsmasq, *mcp) + err = reconcileMachineConfigs(ctx, instance, r.ch, r.Client, allowReconcile, restartDnsmasq, *mcp) if err != nil { r.Log.Error(err) r.SetDegraded(ctx, err) diff --git a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller_test.go b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller_test.go index 4eb50a02c1b..8c84dc1214d 100644 --- a/pkg/operator/controllers/dnsmasq/machineconfigpool_controller_test.go +++ b/pkg/operator/controllers/dnsmasq/machineconfigpool_controller_test.go @@ -8,7 +8,7 @@ import ( "testing" "time" - "github.com/golang/mock/gomock" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" "github.com/sirupsen/logrus" @@ -20,7 +20,8 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1" - mock_dynamichelper "github.com/Azure/ARO-RP/pkg/util/mocks/dynamichelper" + "github.com/Azure/ARO-RP/pkg/util/clienthelper" + testclienthelper "github.com/Azure/ARO-RP/test/util/clienthelper" utilconditions "github.com/Azure/ARO-RP/test/util/conditions" utilerror "github.com/Azure/ARO-RP/test/util/error" ) @@ -32,14 +33,9 @@ func TestMachineConfigPoolReconciler(t *testing.T) { defaultDegraded := utilconditions.ControllerDefaultDegraded(MachineConfigPoolControllerName) defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} - fakeDh := func(controller *gomock.Controller) *mock_dynamichelper.MockInterface { - return mock_dynamichelper.NewMockInterface(controller) - } - tests := []struct { name string objects []client.Object - mocks func(mdh *mock_dynamichelper.MockInterface) request ctrl.Request wantErrMsg string wantConditions []operatorv1.OperatorCondition @@ -47,7 +43,6 @@ func TestMachineConfigPoolReconciler(t *testing.T) { { name: "no cluster", objects: []client.Object{}, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, request: ctrl.Request{}, wantErrMsg: "clusters.aro.openshift.io \"cluster\" not found", }, @@ -66,10 +61,109 @@ func TestMachineConfigPoolReconciler(t *testing.T) { }, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, request: ctrl.Request{}, wantErrMsg: "", }, + { + name: "missing a clusterversion fails", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + Finalizers: []string{MachineConfigPoolControllerName}, + }, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + }, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "custom", + }, + }, + wantErrMsg: `error getting the ClusterVersion: clusterversions.config.openshift.io "version" not found`, + wantConditions: []operatorv1.OperatorCondition{ + defaultAvailable, defaultProgressing, { + Type: MachineConfigPoolControllerName + "ControllerDegraded", + Status: "True", + Message: `error getting the ClusterVersion: clusterversions.config.openshift.io "version" not found`, + LastTransitionTime: transitionTime, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + createTally := make(map[string]int) + updateTally := make(map[string]int) + + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). + WithObjects(tt.objects...). + Build()) + + client.WithPostCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithPostUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) + + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) + + r := NewMachineConfigPoolReconciler( + log, + client, + ch, + ) + ctx := context.Background() + _, err := r.Reconcile(ctx, tt.request) + + utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) + utilconditions.AssertControllerConditions(t, ctx, client, tt.wantConditions) + + e, err := testclienthelper.CompareTally(map[string]int{}, createTally) + if err != nil { + t.Errorf("create comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + + e, err = testclienthelper.CompareTally(map[string]int{}, updateTally) + if err != nil { + t.Errorf("update comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + }) + } +} + +func TestMachineConfigPoolReconcilerNotUpgrading(t *testing.T) { + transitionTime := metav1.Time{Time: time.Now()} + defaultAvailable := utilconditions.ControllerDefaultAvailable(MachineConfigPoolControllerName) + defaultProgressing := utilconditions.ControllerDefaultProgressing(MachineConfigPoolControllerName) + defaultDegraded := utilconditions.ControllerDefaultDegraded(MachineConfigPoolControllerName) + defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} + + tests := []struct { + name string + objects []client.Object + request ctrl.Request + wantCreated map[string]int + wantUpdated map[string]int + wantErrMsg string + wantConditions []operatorv1.OperatorCondition + }{ { name: "no MachineConfigPool does nothing", objects: []client.Object{ @@ -93,7 +187,45 @@ func TestMachineConfigPoolReconciler(t *testing.T) { }, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) {}, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "custom", + }, + }, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "Deleted MachineConfigPool does nothing", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + }, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + Finalizers: []string{MachineConfigPoolControllerName}, + DeletionTimestamp: &transitionTime, + }, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, request: ctrl.Request{ NamespacedName: types.NamespacedName{ Namespace: "", @@ -104,7 +236,7 @@ func TestMachineConfigPoolReconciler(t *testing.T) { wantConditions: defaultConditions, }, { - name: "MachineConfigPool reconciles ARO DNS MachineConfig", + name: "MachineConfigPool reconciliation create missing DNS MachineConfigs, even when cluster not upgrading", objects: []client.Object{ &arov1alpha1.Cluster{ ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, @@ -126,8 +258,88 @@ func TestMachineConfigPoolReconciler(t *testing.T) { Spec: mcv1.MachineConfigPoolSpec{}, }, }, - mocks: func(mdh *mock_dynamichelper.MockInterface) { - mdh.EXPECT().Ensure(gomock.Any(), gomock.AssignableToTypeOf(&mcv1.MachineConfig{})).Times(1) + wantCreated: map[string]int{ + "MachineConfig//99-custom-aro-dns": 1, + }, + wantUpdated: map[string]int{}, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "custom", + }, + }, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "MachineConfigPool reconciliation does not existing DNS MachineConfigs when not updating", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + Finalizers: []string{MachineConfigPoolControllerName}, + }, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-custom-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{}, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "custom", + }, + }, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "MachineConfigPool reconciliation updates existing DNS MachineConfigs when force enabled", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + operator.ForceReconciliation: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + Finalizers: []string{MachineConfigPoolControllerName}, + }, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-custom-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{ + "MachineConfig//99-custom-aro-dns": 1, }, request: ctrl.Request{ NamespacedName: types.NamespacedName{ @@ -139,28 +351,209 @@ func TestMachineConfigPoolReconciler(t *testing.T) { wantConditions: defaultConditions, }, } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + createTally := make(map[string]int) + updateTally := make(map[string]int) + + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). + WithObjects(tt.objects...). + WithObjects(&configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Status: configv1.ClusterVersionStatus{ + History: []configv1.UpdateHistory{ + { + State: configv1.CompletedUpdate, + Version: "4.10.11", + }, + }, + }, + }). + Build()) + + client.WithPostCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithPostUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) + + r := NewMachineConfigPoolReconciler( + log, + client, + ch, + ) + ctx := context.Background() + _, err := r.Reconcile(ctx, tt.request) + + utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) + utilconditions.AssertControllerConditions(t, ctx, client, tt.wantConditions) + + e, err := testclienthelper.CompareTally(tt.wantCreated, createTally) + if err != nil { + t.Errorf("create comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + + e, err = testclienthelper.CompareTally(tt.wantUpdated, updateTally) + if err != nil { + t.Errorf("update comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + }) + } +} + +func TestMachineConfigPoolReconcilerClusterUpgrading(t *testing.T) { + defaultAvailable := utilconditions.ControllerDefaultAvailable(MachineConfigPoolControllerName) + defaultProgressing := utilconditions.ControllerDefaultProgressing(MachineConfigPoolControllerName) + defaultDegraded := utilconditions.ControllerDefaultDegraded(MachineConfigPoolControllerName) + defaultConditions := []operatorv1.OperatorCondition{defaultAvailable, defaultProgressing, defaultDegraded} + + tests := []struct { + name string + objects []client.Object + request ctrl.Request + wantCreated map[string]int + wantUpdated map[string]int + wantErrMsg string + wantConditions []operatorv1.OperatorCondition + }{ + { + name: "MachineConfigPool reconciliation create missing DNS MachineConfigs", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + Finalizers: []string{MachineConfigPoolControllerName}, + }, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + }, + wantCreated: map[string]int{ + "MachineConfig//99-custom-aro-dns": 1, + }, + wantUpdated: map[string]int{}, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "custom", + }, + }, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + { + name: "MachineConfigPool reconciliation updates existing DNS MachineConfigs", + objects: []client.Object{ + &arov1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: arov1alpha1.ClusterStatus{ + Conditions: defaultConditions, + }, + Spec: arov1alpha1.ClusterSpec{ + OperatorFlags: arov1alpha1.OperatorFlags{ + operator.DnsmasqEnabled: operator.FlagTrue, + }, + }, + }, + &mcv1.MachineConfigPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom", + Finalizers: []string{MachineConfigPoolControllerName}, + }, + Status: mcv1.MachineConfigPoolStatus{}, + Spec: mcv1.MachineConfigPoolSpec{}, + }, + &mcv1.MachineConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "99-custom-aro-dns"}, + Spec: mcv1.MachineConfigSpec{}, + }, + }, + wantCreated: map[string]int{}, + wantUpdated: map[string]int{ + "MachineConfig//99-custom-aro-dns": 1, + }, + request: ctrl.Request{ + NamespacedName: types.NamespacedName{ + Namespace: "", + Name: "custom", + }, + }, + wantErrMsg: "", + wantConditions: defaultConditions, + }, + } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - controller := gomock.NewController(t) - defer controller.Finish() + createTally := make(map[string]int) + updateTally := make(map[string]int) - client := ctrlfake.NewClientBuilder(). + client := testclienthelper.NewHookingClient(ctrlfake.NewClientBuilder(). WithObjects(tt.objects...). - Build() - dh := fakeDh(controller) - tt.mocks(dh) + WithObjects(&configv1.ClusterVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: "version", + }, + Spec: configv1.ClusterVersionSpec{}, + Status: configv1.ClusterVersionStatus{ + Conditions: []configv1.ClusterOperatorStatusCondition{ + { + Type: configv1.OperatorProgressing, + Status: configv1.ConditionTrue, + }, + }, + }, + }). + Build()) + + client.WithPostCreateHook(testclienthelper.TallyCountsAndKey(createTally)).WithPostUpdateHook(testclienthelper.TallyCountsAndKey(updateTally)) + + log := logrus.NewEntry(logrus.StandardLogger()) + ch := clienthelper.NewWithClient(log, client) r := NewMachineConfigPoolReconciler( - logrus.NewEntry(logrus.StandardLogger()), + log, client, - dh, + ch, ) ctx := context.Background() _, err := r.Reconcile(ctx, tt.request) utilerror.AssertErrorMessage(t, err, tt.wantErrMsg) utilconditions.AssertControllerConditions(t, ctx, client, tt.wantConditions) + + e, err := testclienthelper.CompareTally(tt.wantCreated, createTally) + if err != nil { + t.Errorf("create comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } + + e, err = testclienthelper.CompareTally(tt.wantUpdated, updateTally) + if err != nil { + t.Errorf("update comparison: %v", err) + for _, i := range e { + t.Error(i) + } + } }) } } diff --git a/pkg/operator/deploy/deploy.go b/pkg/operator/deploy/deploy.go index dfe7efa316b..92d3e1cf695 100644 --- a/pkg/operator/deploy/deploy.go +++ b/pkg/operator/deploy/deploy.go @@ -60,6 +60,7 @@ type Operator interface { RenewMDSDCertificate(context.Context) error EnsureUpgradeAnnotation(context.Context) error SyncClusterObject(context.Context) error + SetForceReconcile(context.Context, bool) error } type operator struct { @@ -108,6 +109,22 @@ type deploymentData struct { SupportsPodSecurityAdmission bool } +func (o *operator) SetForceReconcile(ctx context.Context, enable bool) error { + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + c, err := o.arocli.AroV1alpha1().Clusters().Get(ctx, arov1alpha1.SingletonClusterName, metav1.GetOptions{}) + if err != nil { + return err + } + if enable { + c.Spec.OperatorFlags[pkgoperator.ForceReconciliation] = "true" + } else { + c.Spec.OperatorFlags[pkgoperator.ForceReconciliation] = "false" + } + _, err = o.arocli.AroV1alpha1().Clusters().Update(ctx, c, metav1.UpdateOptions{}) + return err + }) +} + func templateManifests(data deploymentData) ([][]byte, error) { templatesRoot, err := template.ParseFS(embeddedFiles, "staticresources/*.yaml") if err != nil { diff --git a/pkg/operator/flags.go b/pkg/operator/flags.go index b4506f78c40..930b696175f 100644 --- a/pkg/operator/flags.go +++ b/pkg/operator/flags.go @@ -35,6 +35,7 @@ const ( GuardrailsDeployManaged = "aro.guardrails.deploy.managed" CloudProviderConfigEnabled = "aro.cloudproviderconfig.enabled" EtcHostsEnabled = "aro.etchosts.enabled" + ForceReconciliation = "aro.forcereconciliation" FlagTrue = "true" FlagFalse = "false" ) @@ -74,5 +75,6 @@ func DefaultOperatorFlags() map[string]string { GuardrailsDeployManaged: FlagFalse, CloudProviderConfigEnabled: FlagTrue, EtcHostsEnabled: FlagFalse, + ForceReconciliation: FlagFalse, } } diff --git a/pkg/util/mocks/operator/deploy/deploy.go b/pkg/util/mocks/operator/deploy/deploy.go index 41280196582..4070df56df2 100644 --- a/pkg/util/mocks/operator/deploy/deploy.go +++ b/pkg/util/mocks/operator/deploy/deploy.go @@ -134,6 +134,20 @@ func (mr *MockOperatorMockRecorder) Restart(arg0, arg1 interface{}) *gomock.Call return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Restart", reflect.TypeOf((*MockOperator)(nil).Restart), arg0, arg1) } +// SetForceReconcile mocks base method. +func (m *MockOperator) SetForceReconcile(arg0 context.Context, arg1 bool) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetForceReconcile", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// SetForceReconcile indicates an expected call of SetForceReconcile. +func (mr *MockOperatorMockRecorder) SetForceReconcile(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetForceReconcile", reflect.TypeOf((*MockOperator)(nil).SetForceReconcile), arg0, arg1) +} + // SyncClusterObject mocks base method. func (m *MockOperator) SyncClusterObject(arg0 context.Context) error { m.ctrl.T.Helper() diff --git a/pkg/util/version/clusterversion.go b/pkg/util/version/clusterversion.go index 2316b1b333a..00cbb020fa6 100644 --- a/pkg/util/version/clusterversion.go +++ b/pkg/util/version/clusterversion.go @@ -51,3 +51,22 @@ func ClusterVersionIsLessThan4_4(ctx context.Context, configcli configclient.Int // 4.3 uses SRV records for etcd return v.Lt(NewVersion(4, 4)), nil } + +func IsClusterUpgrading(cv *configv1.ClusterVersion) bool { + var isUpgrading bool + if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorProgressing); c != nil && c.Status == configv1.ConditionTrue { + isUpgrading = true + } else { + isUpgrading = false + } + return isUpgrading +} + +func findClusterOperatorStatusCondition(conditions []configv1.ClusterOperatorStatusCondition, name configv1.ClusterStatusConditionType) *configv1.ClusterOperatorStatusCondition { + for i := range conditions { + if conditions[i].Type == name { + return &conditions[i] + } + } + return nil +} diff --git a/test/e2e/operator.go b/test/e2e/operator.go index 66da8cb1065..90c94298e8d 100644 --- a/test/e2e/operator.go +++ b/test/e2e/operator.go @@ -630,31 +630,107 @@ var _ = Describe("ARO Operator - dnsmasq", func() { return names } - BeforeEach(func(ctx context.Context) { - By("Create custom MachineConfigPool") - _, err := clients.MachineConfig.MachineconfigurationV1().MachineConfigPools().Create(ctx, &customMcp, metav1.CreateOptions{}) + AfterEach(func(ctx context.Context) { + // delete the MCP after, just in case + err := clients.MachineConfig.MachineconfigurationV1().MachineConfigPools().Delete(ctx, mcpName, metav1.DeleteOptions{}) + if err != nil && !kerrors.IsNotFound(err) { + Expect(err).ToNot(HaveOccurred()) + } + + // reset the force reconciliation flag + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + co, err := clients.AROClusters.AroV1alpha1().Clusters().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + return err + } + co.Spec.OperatorFlags[operator.ForceReconciliation] = operator.FlagFalse + _, err = clients.AROClusters.AroV1alpha1().Clusters().Update(ctx, co, metav1.UpdateOptions{}) + return err + }) Expect(err).NotTo(HaveOccurred()) }) It("must handle the lifetime of the `99-${MCP}-custom-dns MachineConfig for every MachineConfigPool ${MCP}", func(ctx context.Context) { - By("creating an ARO DNS MachineConfig when creating a custom MachineConfigPool") - Eventually(func(g Gomega, ctx context.Context) { - machineConfigs := getMachineConfigNames(g, ctx) - g.Expect(machineConfigs).To(ContainElement(mcName)) + By("Create custom MachineConfigPool") + _, err := clients.MachineConfig.MachineconfigurationV1().MachineConfigPools().Create(ctx, &customMcp, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) - customMachineConfig, err := clients.MachineConfig.MachineconfigurationV1().MachineConfigs().Get(ctx, mcName, metav1.GetOptions{}) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(customMachineConfig.ObjectMeta.OwnerReferences[0].Name).To(Equal(mcpName)) - }).WithContext(ctx).WithTimeout(timeout).WithPolling(polling).Should(Succeed()) + By("should create an ARO DNS MachineConfig when creating a custom MachineConfigPool") + Eventually(func(g Gomega, _ctx context.Context) []string { + return getMachineConfigNames(g, _ctx) + }).WithContext(ctx).WithTimeout(timeout).WithPolling(polling). + Should(ContainElement(mcName)) - By("deleting the ARO DNS MachineConfig when deleting the custom MachineConfigPool") - err := clients.MachineConfig.MachineconfigurationV1().MachineConfigPools().Delete(ctx, mcpName, metav1.DeleteOptions{}) + By("should have the MachineConfig be owned by the Operator") + customMachineConfig, err := clients.MachineConfig.MachineconfigurationV1().MachineConfigs().Get(ctx, mcName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(customMachineConfig.ObjectMeta.OwnerReferences[0].Name).To(Equal(mcpName)) + + By("should delete the MachineConfig when deleting the custom MachineConfigPool") + err = clients.MachineConfig.MachineconfigurationV1().MachineConfigPools().Delete(ctx, mcpName, metav1.DeleteOptions{}) Expect(err).NotTo(HaveOccurred()) - Eventually(func(g Gomega) { - machineConfigs := getMachineConfigNames(g, ctx) + Eventually(func(g Gomega, _ctx context.Context) { + machineConfigs := getMachineConfigNames(g, _ctx) g.Expect(machineConfigs).NotTo(ContainElement(mcName)) }).WithContext(ctx).WithTimeout(timeout).WithPolling(polling).Should(Succeed()) }) + + It("must respect the forceReconciliation flag and not update MachineConfigs by default", func(ctx context.Context) { + By("Create custom MachineConfigPool") + _, err := clients.MachineConfig.MachineconfigurationV1().MachineConfigPools().Create(ctx, &customMcp, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + + By("should create an ARO DNS MachineConfig when creating a custom MachineConfigPool") + Eventually(func(g Gomega, _ctx context.Context) []string { + return getMachineConfigNames(g, _ctx) + }).WithContext(ctx).WithTimeout(timeout).WithPolling(polling). + Should(ContainElement(mcName)) + + By("updating the MachineConfig, it should not trigger a reconcile") + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + customMachineConfig, err := clients.MachineConfig.MachineconfigurationV1().MachineConfigs().Get(ctx, mcName, metav1.GetOptions{}) + if err != nil { + return err + } + customMachineConfig.ObjectMeta.Labels["testlabel"] = "testvalue" + _, err = clients.MachineConfig.MachineconfigurationV1().MachineConfigs().Update(ctx, customMachineConfig, metav1.UpdateOptions{}) + return err + }) + Expect(err).NotTo(HaveOccurred()) + + By("checking the machineconfig labels, we can see if it has reconciled") + Eventually(func(g Gomega, _ctx context.Context) map[string]string { + config, err := clients.MachineConfig.MachineconfigurationV1().MachineConfigs().Get(ctx, mcName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + + return config.Labels + }).WithContext(ctx).WithPolling(polling).WithTimeout(timeout).MustPassRepeatedly(3).Should(Equal(map[string]string{ + "machineconfiguration.openshift.io/role": mcpName, + "testlabel": "testvalue", + })) + + By("updating the forceReconciliation flag, we can force a reconcile") + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + co, err := clients.AROClusters.AroV1alpha1().Clusters().Get(ctx, "cluster", metav1.GetOptions{}) + if err != nil { + return err + } + co.Spec.OperatorFlags[operator.ForceReconciliation] = operator.FlagTrue + _, err = clients.AROClusters.AroV1alpha1().Clusters().Update(ctx, co, metav1.UpdateOptions{}) + return err + }) + Expect(err).NotTo(HaveOccurred()) + + By("checking the machineconfig labels, we can see if our flag has been removed") + Eventually(func(g Gomega, _ctx context.Context) map[string]string { + config, err := clients.MachineConfig.MachineconfigurationV1().MachineConfigs().Get(ctx, mcName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + + return config.Labels + }).WithContext(ctx).WithPolling(polling).WithTimeout(timeout).Should(Equal(map[string]string{ + "machineconfiguration.openshift.io/role": mcpName, + })) + }) }) var _ = Describe("ARO Operator - Guardrails", func() { diff --git a/test/util/clienthelper/hookclient.go b/test/util/clienthelper/hookclient.go index 7ce15dc287e..2cfceacfe7a 100644 --- a/test/util/clienthelper/hookclient.go +++ b/test/util/clienthelper/hookclient.go @@ -69,10 +69,12 @@ func (c *HookingClient) WithPostCreateHook(f hookFunc) *HookingClient { c.postCreateHook = append(c.postCreateHook, f) return c } + func (c *HookingClient) WithPostUpdateHook(f hookFunc) *HookingClient { c.postUpdateHook = append(c.postUpdateHook, f) return c } + func (c *HookingClient) WithPostPatchHook(f hookFunc) *HookingClient { c.postPatchHook = append(c.postPatchHook, f) return c @@ -96,6 +98,7 @@ func (c *HookingClient) WithPreUpdateHook(f hookFunc) *HookingClient { c.preUpdateHook = append(c.preUpdateHook, f) return c } + func (c *HookingClient) WithPrePatchHook(f hookFunc) *HookingClient { c.prePatchHook = append(c.prePatchHook, f) return c