diff --git a/controllers/machine_controller.go b/controllers/machine_controller.go index b733ba048ce4..ab32f1b726e8 100644 --- a/controllers/machine_controller.go +++ b/controllers/machine_controller.go @@ -235,8 +235,6 @@ func patchMachine(ctx context.Context, patchHelper *patch.Helper, machine *clust } func (r *MachineReconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, m *clusterv1.Machine) (ctrl.Result, error) { - logger := r.Log.WithValues("machine", m.Name, "namespace", m.Namespace) - logger = logger.WithValues("cluster", cluster.Name) // If the Machine belongs to a cluster, add an owner reference. if r.shouldAdopt(m) { @@ -248,28 +246,24 @@ func (r *MachineReconciler) reconcile(ctx context.Context, cluster *clusterv1.Cl }) } - // Call the inner reconciliation methods. - reconciliationErrors := []error{ - r.reconcileBootstrap(ctx, cluster, m), - r.reconcileInfrastructure(ctx, cluster, m), - r.reconcileNodeRef(ctx, cluster, m), + phases := []func(context.Context, *clusterv1.Cluster, *clusterv1.Machine) (ctrl.Result, error){ + r.reconcileBootstrap, + r.reconcileInfrastructure, + r.reconcileNodeRef, } - // Parse the errors, making sure we record if there is a RequeueAfterError. res := ctrl.Result{} errs := []error{} - for _, err := range reconciliationErrors { - if requeueErr, ok := errors.Cause(err).(capierrors.HasRequeueAfterError); ok { - // Only record and log the first RequeueAfterError. - if !res.Requeue { - res.Requeue = true - res.RequeueAfter = requeueErr.GetRequeueAfter() - logger.Error(err, "Reconciliation for Machine asked to requeue") - } + for _, phase := range phases { + // Call the inner reconciliation methods. + phaseResult, err := phase(ctx, cluster, m) + if err != nil { + errs = append(errs, err) + } + if len(errs) > 0 { continue } - - errs = append(errs, err) + res = util.LowestNonZeroResult(res, phaseResult) } return res, kerrors.NewAggregate(errs) } diff --git a/controllers/machine_controller_noderef.go b/controllers/machine_controller_noderef.go index ec79dd5dd3f6..9fa4f21058c5 100644 --- a/controllers/machine_controller_noderef.go +++ b/controllers/machine_controller_noderef.go @@ -18,14 +18,15 @@ package controllers import ( "context" + "fmt" "time" "github.com/pkg/errors" apicorev1 "k8s.io/api/core/v1" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" "sigs.k8s.io/cluster-api/controllers/noderefutil" - capierrors "sigs.k8s.io/cluster-api/errors" "sigs.k8s.io/cluster-api/util" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -33,16 +34,16 @@ var ( ErrNodeNotFound = errors.New("cannot find node with matching ProviderID") ) -func (r *MachineReconciler) reconcileNodeRef(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error { +func (r *MachineReconciler) reconcileNodeRef(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) (ctrl.Result, error) { logger := r.Log.WithValues("machine", machine.Name, "namespace", machine.Namespace) // Check that the Machine hasn't been deleted or in the process. if !machine.DeletionTimestamp.IsZero() { - return nil + return ctrl.Result{}, nil } // Check that the Machine doesn't already have a NodeRef. if machine.Status.NodeRef != nil { - return nil + return ctrl.Result{}, nil } logger = logger.WithValues("cluster", cluster.Name) @@ -50,36 +51,36 @@ func (r *MachineReconciler) reconcileNodeRef(ctx context.Context, cluster *clust // Check that the Machine has a valid ProviderID. if machine.Spec.ProviderID == nil || *machine.Spec.ProviderID == "" { logger.Info("Machine doesn't have a valid ProviderID yet") - return nil + return ctrl.Result{}, nil } providerID, err := noderefutil.NewProviderID(*machine.Spec.ProviderID) if err != nil { - return err + return ctrl.Result{}, err } remoteClient, err := r.Tracker.GetClient(ctx, util.ObjectKey(cluster)) if err != nil { - return err + return ctrl.Result{}, err } // Get the Node reference. nodeRef, err := r.getNodeReference(remoteClient, providerID) if err != nil { if err == ErrNodeNotFound { - return errors.Wrapf(&capierrors.RequeueAfterError{RequeueAfter: 20 * time.Second}, - "cannot assign NodeRef to Machine %q in namespace %q, no matching Node", machine.Name, machine.Namespace) + logger.Info(fmt.Sprintf("Cannot assign NodeRef to Machine: %s, requeuing", ErrNodeNotFound.Error())) + return ctrl.Result{RequeueAfter: 20 * time.Second}, nil } logger.Error(err, "Failed to assign NodeRef") r.recorder.Event(machine, apicorev1.EventTypeWarning, "FailedSetNodeRef", err.Error()) - return err + return ctrl.Result{}, err } // Set the Machine NodeRef. machine.Status.NodeRef = nodeRef logger.Info("Set Machine's NodeRef", "noderef", machine.Status.NodeRef.Name) r.recorder.Event(machine, apicorev1.EventTypeNormal, "SuccessfulSetNodeRef", machine.Status.NodeRef.Name) - return nil + return ctrl.Result{}, nil } func (r *MachineReconciler) getNodeReference(c client.Reader, providerID *noderefutil.ProviderID) (*apicorev1.ObjectReference, error) { diff --git a/controllers/machine_controller_phases.go b/controllers/machine_controller_phases.go index 81edf10f7f37..19d5c4027c42 100644 --- a/controllers/machine_controller_phases.go +++ b/controllers/machine_controller_phases.go @@ -29,17 +29,17 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/pointer" - "sigs.k8s.io/cluster-api/util/annotations" - "sigs.k8s.io/cluster-api/util/conditions" - utilconversion "sigs.k8s.io/cluster-api/util/conversion" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/handler" - clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" "sigs.k8s.io/cluster-api/controllers/external" capierrors "sigs.k8s.io/cluster-api/errors" "sigs.k8s.io/cluster-api/util" + "sigs.k8s.io/cluster-api/util/annotations" + "sigs.k8s.io/cluster-api/util/conditions" + utilconversion "sigs.k8s.io/cluster-api/util/conversion" "sigs.k8s.io/cluster-api/util/patch" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" ) var ( @@ -172,38 +172,40 @@ func (r *MachineReconciler) reconcileExternal(ctx context.Context, cluster *clus } // reconcileBootstrap reconciles the Spec.Bootstrap.ConfigRef object on a Machine. -func (r *MachineReconciler) reconcileBootstrap(ctx context.Context, cluster *clusterv1.Cluster, m *clusterv1.Machine) error { +func (r *MachineReconciler) reconcileBootstrap(ctx context.Context, cluster *clusterv1.Cluster, m *clusterv1.Machine) (ctrl.Result, error) { + logger := r.Log.WithValues("machine", m.Name, "namespace", m.Namespace) + // If the bootstrap data is populated, set ready and return. if m.Spec.Bootstrap.DataSecretName != nil { m.Status.BootstrapReady = true conditions.MarkTrue(m, clusterv1.BootstrapReadyCondition) - return nil + return ctrl.Result{}, nil } // If the Boostrap ref is nil (and so the machine should use user generated data secret), return. if m.Spec.Bootstrap.ConfigRef == nil { - return nil + return ctrl.Result{}, nil } // Call generic external reconciler if we have an external reference. externalResult, err := r.reconcileExternal(ctx, cluster, m, m.Spec.Bootstrap.ConfigRef) if err != nil { - return err + return ctrl.Result{}, err } if externalResult.Paused { - return nil + return ctrl.Result{}, nil } bootstrapConfig := externalResult.Result // If the bootstrap config is being deleted, return early. if !bootstrapConfig.GetDeletionTimestamp().IsZero() { - return nil + return ctrl.Result{}, nil } // Determine if the bootstrap provider is ready. ready, err := external.IsReady(bootstrapConfig) if err != nil { - return err + return ctrl.Result{}, err } // Report a summary of current status of the bootstrap object defined for this machine. @@ -214,26 +216,28 @@ func (r *MachineReconciler) reconcileBootstrap(ctx context.Context, cluster *clu // If the bootstrap provider is not ready, requeue. if !ready { - return errors.Wrapf(&capierrors.RequeueAfterError{RequeueAfter: externalReadyWait}, - "Bootstrap provider for Machine %q in namespace %q is not ready, requeuing", m.Name, m.Namespace) + logger.Info("Bootstrap provider is not ready, requeuing") + return ctrl.Result{RequeueAfter: externalReadyWait}, nil } // Get and set the name of the secret containing the bootstrap data. secretName, _, err := unstructured.NestedString(bootstrapConfig.Object, "status", "dataSecretName") if err != nil { - return errors.Wrapf(err, "failed to retrieve dataSecretName from bootstrap provider for Machine %q in namespace %q", m.Name, m.Namespace) + return ctrl.Result{}, errors.Wrapf(err, "failed to retrieve dataSecretName from bootstrap provider for Machine %q in namespace %q", m.Name, m.Namespace) } else if secretName == "" { - return errors.Errorf("retrieved empty dataSecretName from bootstrap provider for Machine %q in namespace %q", m.Name, m.Namespace) + return ctrl.Result{}, errors.Errorf("retrieved empty dataSecretName from bootstrap provider for Machine %q in namespace %q", m.Name, m.Namespace) } m.Spec.Bootstrap.Data = nil m.Spec.Bootstrap.DataSecretName = pointer.StringPtr(secretName) m.Status.BootstrapReady = true - return nil + return ctrl.Result{}, nil } // reconcileInfrastructure reconciles the Spec.InfrastructureRef object on a Machine. -func (r *MachineReconciler) reconcileInfrastructure(ctx context.Context, cluster *clusterv1.Cluster, m *clusterv1.Machine) error { +func (r *MachineReconciler) reconcileInfrastructure(ctx context.Context, cluster *clusterv1.Cluster, m *clusterv1.Machine) (ctrl.Result, error) { + logger := r.Log.WithValues("machine", m.Name, "namespace", m.Namespace) + // Call generic external reconciler. infraReconcileResult, err := r.reconcileExternal(ctx, cluster, m, &m.Spec.InfrastructureRef) if err != nil { @@ -244,22 +248,22 @@ func (r *MachineReconciler) reconcileInfrastructure(ctx context.Context, cluster m.Status.FailureMessage = pointer.StringPtr(fmt.Sprintf("Machine infrastructure resource %v with name %q has been deleted after being ready", m.Spec.InfrastructureRef.GroupVersionKind(), m.Spec.InfrastructureRef.Name)) } - return err + return ctrl.Result{}, err } // if the external object is paused, return without any further processing if infraReconcileResult.Paused { - return nil + return ctrl.Result{}, nil } infraConfig := infraReconcileResult.Result if !infraConfig.GetDeletionTimestamp().IsZero() { - return nil + return ctrl.Result{}, nil } // Determine if the infrastructure provider is ready. ready, err := external.IsReady(infraConfig) if err != nil { - return err + return ctrl.Result{}, err } m.Status.InfrastructureReady = ready @@ -271,23 +275,22 @@ func (r *MachineReconciler) reconcileInfrastructure(ctx context.Context, cluster // If the infrastructure provider is not ready, return early. if !ready { - return errors.Wrapf(&capierrors.RequeueAfterError{RequeueAfter: externalReadyWait}, - "Infrastructure provider for Machine %q in namespace %q is not ready, requeuing", m.Name, m.Namespace, - ) + logger.Info("Infrastructure provider is not ready, requeuing") + return ctrl.Result{RequeueAfter: externalReadyWait}, nil } // Get Spec.ProviderID from the infrastructure provider. var providerID string if err := util.UnstructuredUnmarshalField(infraConfig, &providerID, "spec", "providerID"); err != nil { - return errors.Wrapf(err, "failed to retrieve Spec.ProviderID from infrastructure provider for Machine %q in namespace %q", m.Name, m.Namespace) + return ctrl.Result{}, errors.Wrapf(err, "failed to retrieve Spec.ProviderID from infrastructure provider for Machine %q in namespace %q", m.Name, m.Namespace) } else if providerID == "" { - return errors.Errorf("retrieved empty Spec.ProviderID from infrastructure provider for Machine %q in namespace %q", m.Name, m.Namespace) + return ctrl.Result{}, errors.Errorf("retrieved empty Spec.ProviderID from infrastructure provider for Machine %q in namespace %q", m.Name, m.Namespace) } // Get and set Status.Addresses from the infrastructure provider. err = util.UnstructuredUnmarshalField(infraConfig, &m.Status.Addresses, "status", "addresses") if err != nil && err != util.ErrUnstructuredFieldNotFound { - return errors.Wrapf(err, "failed to retrieve addresses from infrastructure provider for Machine %q in namespace %q", m.Name, m.Namespace) + return ctrl.Result{}, errors.Wrapf(err, "failed to retrieve addresses from infrastructure provider for Machine %q in namespace %q", m.Name, m.Namespace) } // Get and set the failure domain from the infrastructure provider. @@ -296,11 +299,11 @@ func (r *MachineReconciler) reconcileInfrastructure(ctx context.Context, cluster switch { case err == util.ErrUnstructuredFieldNotFound: // no-op case err != nil: - return errors.Wrapf(err, "failed to failure domain from infrastructure provider for Machine %q in namespace %q", m.Name, m.Namespace) + return ctrl.Result{}, errors.Wrapf(err, "failed to failure domain from infrastructure provider for Machine %q in namespace %q", m.Name, m.Namespace) default: m.Spec.FailureDomain = pointer.StringPtr(failureDomain) } m.Spec.ProviderID = pointer.StringPtr(providerID) - return nil + return ctrl.Result{}, nil } diff --git a/controllers/machine_controller_phases_test.go b/controllers/machine_controller_phases_test.go index 480d96f0cbf7..66520bff50f2 100644 --- a/controllers/machine_controller_phases_test.go +++ b/controllers/machine_controller_phases_test.go @@ -29,10 +29,12 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" "k8s.io/utils/pointer" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" "sigs.k8s.io/cluster-api/controllers/external" "sigs.k8s.io/cluster-api/util/kubeconfig" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -104,7 +106,7 @@ var _ = Describe("Reconcile Machine Phases", func() { } BeforeEach(func() { - defaultKubeconfigSecret = kubeconfig.GenerateSecret(defaultCluster, kubeconfig.FromEnvTestConfig(testEnv.Config, defaultCluster)) + defaultKubeconfigSecret = kubeconfig.GenerateSecret(defaultCluster, kubeconfig.FromEnvTestConfig(&rest.Config{}, defaultCluster)) }) It("Should set OwnerReference and cluster name label on external objects", func() { @@ -128,7 +130,7 @@ var _ = Describe("Reconcile Machine Phases", func() { res, err := r.reconcile(context.Background(), defaultCluster, machine) Expect(err).NotTo(HaveOccurred()) - Expect(res.Requeue).To(BeTrue()) + Expect(res.RequeueAfter).To(Equal(externalReadyWait)) r.reconcilePhase(context.Background(), machine) @@ -164,7 +166,7 @@ var _ = Describe("Reconcile Machine Phases", func() { res, err := r.reconcile(context.Background(), defaultCluster, machine) Expect(err).NotTo(HaveOccurred()) - Expect(res.Requeue).To(BeTrue()) + Expect(res.RequeueAfter).To(Equal(externalReadyWait)) r.reconcilePhase(context.Background(), machine) Expect(machine.Status.GetTypedPhase()).To(Equal(clusterv1.MachinePhasePending)) @@ -205,7 +207,7 @@ var _ = Describe("Reconcile Machine Phases", func() { res, err := r.reconcile(context.Background(), defaultCluster, machine) Expect(err).NotTo(HaveOccurred()) - Expect(res.Requeue).To(BeTrue()) + Expect(res.Requeue).To(BeFalse()) r.reconcilePhase(context.Background(), machine) Expect(machine.Status.GetTypedPhase()).To(Equal(clusterv1.MachinePhaseProvisioning)) @@ -436,7 +438,7 @@ var _ = Describe("Reconcile Machine Phases", func() { res, err := r.reconcile(context.Background(), defaultCluster, machine) Expect(err).NotTo(HaveOccurred()) - Expect(res.Requeue).To(BeTrue()) + Expect(res.RequeueAfter).To(Equal(externalReadyWait)) r.reconcilePhase(context.Background(), machine) Expect(machine.Status.GetTypedPhase()).To(Equal(clusterv1.MachinePhaseProvisioned)) @@ -547,6 +549,7 @@ func TestReconcileBootstrap(t *testing.T) { machine *clusterv1.Machine expectError bool expected func(g *WithT, m *clusterv1.Machine) + result *ctrl.Result }{ { name: "new machine, bootstrap config ready with data", @@ -602,7 +605,8 @@ func TestReconcileBootstrap(t *testing.T) { "spec": map[string]interface{}{}, "status": map[string]interface{}{}, }, - expectError: true, + expectError: false, + result: &ctrl.Result{RequeueAfter: externalReadyWait}, expected: func(g *WithT, m *clusterv1.Machine) { g.Expect(m.Status.BootstrapReady).To(BeFalse()) }, @@ -720,7 +724,8 @@ func TestReconcileBootstrap(t *testing.T) { BootstrapReady: true, }, }, - expectError: true, + expectError: false, + result: &ctrl.Result{RequeueAfter: externalReadyWait}, expected: func(g *WithT, m *clusterv1.Machine) { g.Expect(m.GetOwnerReferences()).NotTo(ContainRefOfGroupKind("cluster.x-k8s.io", "MachineSet")) }, @@ -745,7 +750,7 @@ func TestReconcileBootstrap(t *testing.T) { }, "spec": map[string]interface{}{}, "status": map[string]interface{}{ - "ready": false, + "ready": true, }, }, machine: &clusterv1.Machine{ @@ -795,7 +800,7 @@ func TestReconcileBootstrap(t *testing.T) { scheme: scheme.Scheme, } - err := r.reconcileBootstrap(context.Background(), defaultCluster, tc.machine) + res, err := r.reconcileBootstrap(context.Background(), defaultCluster, tc.machine) if tc.expectError { g.Expect(err).ToNot(BeNil()) } else { @@ -805,6 +810,10 @@ func TestReconcileBootstrap(t *testing.T) { if tc.expected != nil { tc.expected(g, tc.machine) } + + if tc.result != nil { + g.Expect(res).To(Equal(*tc.result)) + } }) } } @@ -1006,7 +1015,7 @@ func TestReconcileInfrastructure(t *testing.T) { scheme: scheme.Scheme, } - err := r.reconcileInfrastructure(context.Background(), defaultCluster, tc.machine) + _, err := r.reconcileInfrastructure(context.Background(), defaultCluster, tc.machine) r.reconcilePhase(context.Background(), tc.machine) if tc.expectError { g.Expect(err).ToNot(BeNil()) diff --git a/controlplane/kubeadm/controllers/controller.go b/controlplane/kubeadm/controllers/controller.go index d6a10904cd75..32f7840c4bb0 100644 --- a/controlplane/kubeadm/controllers/controller.go +++ b/controlplane/kubeadm/controllers/controller.go @@ -276,7 +276,7 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, cluster * return ctrl.Result{}, err } - controlPlaneMachines, err := r.managementCluster.GetMachinesForCluster(ctx, util.ObjectKey(cluster), machinefilters.ControlPlaneMachines(cluster.Name)) + controlPlaneMachines, err := r.managementClusterUncached.GetMachinesForCluster(ctx, util.ObjectKey(cluster), machinefilters.ControlPlaneMachines(cluster.Name)) if err != nil { logger.Error(err, "failed to retrieve control plane machines for cluster") return ctrl.Result{}, err diff --git a/util/conditions/getter.go b/util/conditions/getter.go index 64dc8d21aa67..50e3ad5b6eb6 100644 --- a/util/conditions/getter.go +++ b/util/conditions/getter.go @@ -153,15 +153,20 @@ func summary(from Getter, options ...MergeOption) *clusterv1.Condition { } // If it is required to add a step counter only if a subset of condition exists, check if the conditions - // in scope are included in this subset. + // in scope are included in this subset or not. if mergeOpt.addStepCounterIfOnlyConditionTypes != nil { for _, c := range conditionsInScope { + found := false for _, t := range mergeOpt.addStepCounterIfOnlyConditionTypes { - if c.Type != t { - mergeOpt.addStepCounter = false + if c.Type == t { + found = true break } } + if !found { + mergeOpt.addStepCounter = false + break + } } } diff --git a/util/conditions/getter_test.go b/util/conditions/getter_test.go index 0119c0599bba..ca0183b3ee5a 100644 --- a/util/conditions/getter_test.go +++ b/util/conditions/getter_test.go @@ -200,6 +200,12 @@ func TestSummary(t *testing.T) { options: []MergeOption{WithConditions("bar", "baz"), WithStepCounter(), WithStepCounterIfOnly("bar")}, // there is only bar, the step counter should be set and counts only a subset of conditions want: FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "0 of 1 completed"), }, + { + name: "Returns ready condition with the summary of selected conditions (using WithConditions and WithStepCounterIfOnly options - with inconsistent order between the two)", + from: getterWithConditions(bar), + options: []MergeOption{WithConditions("baz", "bar"), WithStepCounter(), WithStepCounterIfOnly("bar", "baz")}, // conditions in WithStepCounterIfOnly could be in different order than in WithConditions + want: FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "0 of 2 completed"), + }, { name: "Returns ready condition with the summary of selected conditions (using WithConditions and WithStepCounterIfOnly options)", from: getterWithConditions(bar, baz),