From 88d43aa5c3755b27927cce21a38cb7a1b0d1da33 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Mon, 8 Jun 2020 20:43:40 +0200 Subject: [PATCH 1/3] Fix to allow multiple test environments --- controllers/cluster_controller_phases_test.go | 4 +- controllers/machine_controller_phases_test.go | 40 +++++++++---------- exp/controllers/suite_test.go | 8 ++-- test/helpers/envtest.go | 8 ++-- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/controllers/cluster_controller_phases_test.go b/controllers/cluster_controller_phases_test.go index a676a111696b..ef8be2f38fdd 100644 --- a/controllers/cluster_controller_phases_test.go +++ b/controllers/cluster_controller_phases_test.go @@ -130,9 +130,9 @@ func TestClusterReconcilePhases(t *testing.T) { var c client.Client if tt.infraRef != nil { infraConfig := &unstructured.Unstructured{Object: tt.infraRef} - c = fake.NewFakeClientWithScheme(scheme.Scheme, external.TestGenericInfrastructureCRD, tt.cluster, infraConfig) + c = fake.NewFakeClientWithScheme(scheme.Scheme, external.TestGenericInfrastructureCRD.DeepCopy(), tt.cluster, infraConfig) } else { - c = fake.NewFakeClientWithScheme(scheme.Scheme, external.TestGenericInfrastructureCRD, tt.cluster) + c = fake.NewFakeClientWithScheme(scheme.Scheme, external.TestGenericInfrastructureCRD.DeepCopy(), tt.cluster) } r := &ClusterReconciler{ Client: c, diff --git a/controllers/machine_controller_phases_test.go b/controllers/machine_controller_phases_test.go index 5ad70311875e..7ef6eaad4776 100644 --- a/controllers/machine_controller_phases_test.go +++ b/controllers/machine_controller_phases_test.go @@ -118,8 +118,8 @@ var _ = Describe("Reconcile Machine Phases", func() { defaultCluster, defaultKubeconfigSecret, machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), bootstrapConfig, infraConfig, ), @@ -154,8 +154,8 @@ var _ = Describe("Reconcile Machine Phases", func() { defaultCluster, defaultKubeconfigSecret, machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), bootstrapConfig, infraConfig, ), @@ -195,8 +195,8 @@ var _ = Describe("Reconcile Machine Phases", func() { defaultCluster, defaultKubeconfigSecret, machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), bootstrapConfig, infraConfig, ), @@ -262,8 +262,8 @@ var _ = Describe("Reconcile Machine Phases", func() { defaultCluster, defaultKubeconfigSecret, machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), bootstrapConfig, infraConfig, ), @@ -316,8 +316,8 @@ var _ = Describe("Reconcile Machine Phases", func() { defaultCluster, defaultKubeconfigSecret, machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), bootstrapConfig, infraConfig, ), @@ -381,8 +381,8 @@ var _ = Describe("Reconcile Machine Phases", func() { defaultCluster, defaultKubeconfigSecret, machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), bootstrapConfig, infraConfig, ), @@ -426,8 +426,8 @@ var _ = Describe("Reconcile Machine Phases", func() { defaultCluster, defaultKubeconfigSecret, machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), bootstrapConfig, infraConfig, ), @@ -493,8 +493,8 @@ var _ = Describe("Reconcile Machine Phases", func() { defaultCluster, defaultKubeconfigSecret, machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), bootstrapConfig, infraConfig, ), @@ -730,8 +730,8 @@ func TestReconcileBootstrap(t *testing.T) { r := &MachineReconciler{ Client: fake.NewFakeClientWithScheme(scheme.Scheme, tc.machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), bootstrapConfig, ), Log: log.Log, @@ -931,8 +931,8 @@ func TestReconcileInfrastructure(t *testing.T) { r := &MachineReconciler{ Client: fake.NewFakeClientWithScheme(scheme.Scheme, tc.machine, - external.TestGenericBootstrapCRD, - external.TestGenericInfrastructureCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), infraConfig, ), Log: log.Log, diff --git a/exp/controllers/suite_test.go b/exp/controllers/suite_test.go index eeac2f5bf199..3f40b8db21e6 100644 --- a/exp/controllers/suite_test.go +++ b/exp/controllers/suite_test.go @@ -79,10 +79,10 @@ var _ = BeforeSuite(func(done Done) { By("bootstrapping test environment") testEnv = &envtest.Environment{ CRDs: []runtime.Object{ - external.TestGenericBootstrapCRD, - external.TestGenericBootstrapTemplateCRD, - external.TestGenericInfrastructureCRD, - external.TestGenericInfrastructureTemplateCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericBootstrapTemplateCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), + external.TestGenericInfrastructureTemplateCRD.DeepCopy(), }, CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, } diff --git a/test/helpers/envtest.go b/test/helpers/envtest.go index 1304432b1fab..8317b976c221 100644 --- a/test/helpers/envtest.go +++ b/test/helpers/envtest.go @@ -65,10 +65,10 @@ func newTestEnvironment() *TestEnvironment { filepath.Join(root, "bootstrap", "kubeadm", "config", "crd", "bases"), }, CRDs: []runtime.Object{ - external.TestGenericBootstrapCRD, - external.TestGenericBootstrapTemplateCRD, - external.TestGenericInfrastructureCRD, - external.TestGenericInfrastructureTemplateCRD, + external.TestGenericBootstrapCRD.DeepCopy(), + external.TestGenericBootstrapTemplateCRD.DeepCopy(), + external.TestGenericInfrastructureCRD.DeepCopy(), + external.TestGenericInfrastructureTemplateCRD.DeepCopy(), }, }, scheme: scheme, From 6494b2d0479131cbc2013f0a0f6193ad27bebb1c Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Mon, 8 Jun 2020 20:43:58 +0200 Subject: [PATCH 2/3] Refactor condition helpers for Mirror, Summary and Aggregate --- util/conditions/getter.go | 46 ++++++++++++++++++++++++++++++---- util/conditions/getter_test.go | 6 ++--- util/conditions/merge_test.go | 4 +-- util/conditions/setter.go | 14 +++++------ util/conditions/setter_test.go | 30 ++++++++++++++++++++++ 5 files changed, 83 insertions(+), 17 deletions(-) diff --git a/util/conditions/getter.go b/util/conditions/getter.go index f6841680c6ad..24ef2a626d23 100644 --- a/util/conditions/getter.go +++ b/util/conditions/getter.go @@ -114,9 +114,9 @@ func GetLastTransitionTime(from Getter, t clusterv1.ConditionType) *metav1.Time return nil } -// Summary returns a Ready condition with the summary of all the conditions existing +// summary returns a Ready condition with the summary of all the conditions existing // on an object. If the object does not have other conditions, no summary condition is generated. -func Summary(from Getter, options ...MergeOption) *clusterv1.Condition { +func summary(from Getter, options ...MergeOption) *clusterv1.Condition { conditions := from.GetConditions() conditionsInScope := make([]localizedCondition, 0, len(conditions)) @@ -137,11 +137,47 @@ func Summary(from Getter, options ...MergeOption) *clusterv1.Condition { return merge(conditionsInScope, clusterv1.ReadyCondition, mergeOpt) } -// Mirror mirrors the Ready condition from a dependent object into the target condition; +// mirrorOptions allows to set options for the mirror operation. +type mirrorOptions struct { + fallbackTo *bool + fallbackReason string + fallbackSeverity clusterv1.ConditionSeverity + fallbackMessage string +} + +// MirrorOptions defines an option for mirroring conditions. +type MirrorOptions func(*mirrorOptions) + +// WithFallbackValue specify a fallback value to use in case the mirrored condition does not exists; +// in case the fallbackValue is false, given values for reason, severity and message will be used. +func WithFallbackValue(fallbackValue bool, reason string, severity clusterv1.ConditionSeverity, message string) MirrorOptions { + return func(c *mirrorOptions) { + c.fallbackTo = &fallbackValue + c.fallbackReason = reason + c.fallbackSeverity = severity + c.fallbackMessage = message + } +} + +// mirror mirrors the Ready condition from a dependent object into the target condition; // if the Ready condition does not exists in the source object, no target conditions is generated. -func Mirror(from Getter, targetCondition clusterv1.ConditionType) *clusterv1.Condition { +func mirror(from Getter, targetCondition clusterv1.ConditionType, options ...MirrorOptions) *clusterv1.Condition { + mirrorOpt := &mirrorOptions{} + for _, o := range options { + o(mirrorOpt) + } + condition := Get(from, clusterv1.ReadyCondition) + if mirrorOpt.fallbackTo != nil && condition == nil { + switch *mirrorOpt.fallbackTo { + case true: + condition = TrueCondition(targetCondition) + case false: + condition = FalseCondition(targetCondition, mirrorOpt.fallbackReason, mirrorOpt.fallbackSeverity, mirrorOpt.fallbackMessage) + } + } + if condition != nil { condition.Type = targetCondition } @@ -152,7 +188,7 @@ func Mirror(from Getter, targetCondition clusterv1.ConditionType) *clusterv1.Con // Aggregates all the the Ready condition from a list of dependent objects into the target object; // if the Ready condition does not exists in one of the source object, the object is excluded from // the aggregation; if none of the source object have ready condition, no target conditions is generated. -func Aggregate(from []Getter, targetCondition clusterv1.ConditionType, options ...MergeOption) *clusterv1.Condition { +func aggregate(from []Getter, targetCondition clusterv1.ConditionType, options ...MergeOption) *clusterv1.Condition { conditionsInScope := make([]localizedCondition, 0, len(from)) for i := range from { condition := Get(from[i], clusterv1.ReadyCondition) diff --git a/util/conditions/getter_test.go b/util/conditions/getter_test.go index 9c9f2536af40..ea5ae3e04233 100644 --- a/util/conditions/getter_test.go +++ b/util/conditions/getter_test.go @@ -120,7 +120,7 @@ func TestMirror(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got := Mirror(tt.from, tt.t) + got := mirror(tt.from, tt.t) if tt.want == nil { g.Expect(got).To(BeNil()) return @@ -161,7 +161,7 @@ func TestSummary(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got := Summary(tt.from) + got := summary(tt.from) if tt.want == nil { g.Expect(got).To(BeNil()) return @@ -205,7 +205,7 @@ func TestAggregate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got := Aggregate(tt.from, tt.t) + got := aggregate(tt.from, tt.t) if tt.want == nil { g.Expect(got).To(BeNil()) return diff --git a/util/conditions/merge_test.go b/util/conditions/merge_test.go index 33cec5cbdb76..c27ab171c6af 100644 --- a/util/conditions/merge_test.go +++ b/util/conditions/merge_test.go @@ -89,12 +89,12 @@ func TestMergeRespectPriority(t *testing.T) { want *clusterv1.Condition }{ { - name: "Aggregate nil list return nil", + name: "aggregate nil list return nil", conditions: nil, want: nil, }, { - name: "Aggregate empty list return nil", + name: "aggregate empty list return nil", conditions: []*clusterv1.Condition{}, want: nil, }, diff --git a/util/conditions/setter.go b/util/conditions/setter.go index 2ed67abe4d8b..5aea137faa07 100644 --- a/util/conditions/setter.go +++ b/util/conditions/setter.go @@ -116,21 +116,21 @@ func MarkFalse(to Setter, t clusterv1.ConditionType, reason string, severity clu // SetSummary sets a Ready condition with the summary of all the conditions existing // on an object. If the object does not have other conditions, no summary condition is generated. func SetSummary(to Setter, options ...MergeOption) { - Set(to, Summary(to, options...)) + Set(to, summary(to, options...)) } -// SetMirrorCondition creates a new condition by mirroring the the Ready condition from a dependent object; +// SetMirror creates a new condition by mirroring the the Ready condition from a dependent object; // if the Ready condition does not exists in the source object, no target conditions is generated. -func SetMirrorCondition(to Setter, targetCondition clusterv1.ConditionType, from Getter) { - Set(to, Mirror(from, targetCondition)) +func SetMirror(to Setter, targetCondition clusterv1.ConditionType, from Getter, options ...MirrorOptions) { + Set(to, mirror(from, targetCondition, options...)) } -// SetAggregateCondition creates a new condition with the aggregation of all the the Ready condition +// SetAggregate creates a new condition with the aggregation of all the the Ready condition // from a list of dependent objects; if the Ready condition does not exists in one of the source object, // the object is excluded from the aggregation; if none of the source object have ready condition, // no target conditions is generated. -func SetAggregateCondition(to Setter, targetCondition clusterv1.ConditionType, from []Getter, options ...MergeOption) { - Set(to, Aggregate(from, targetCondition, options...)) +func SetAggregate(to Setter, targetCondition clusterv1.ConditionType, from []Getter, options ...MergeOption) { + Set(to, aggregate(from, targetCondition, options...)) } // Delete deletes the condition with the given type. diff --git a/util/conditions/setter_test.go b/util/conditions/setter_test.go index b568b7f324b3..d12c55c01dee 100644 --- a/util/conditions/setter_test.go +++ b/util/conditions/setter_test.go @@ -216,6 +216,36 @@ func TestMarkMethods(t *testing.T) { })) } +func TestSetSummary(t *testing.T) { + g := NewWithT(t) + target := setterWithConditions(TrueCondition("foo")) + + SetSummary(target) + + g.Expect(Has(target, clusterv1.ReadyCondition)).To(BeTrue()) +} + +func TestSetMirror(t *testing.T) { + g := NewWithT(t) + source := getterWithConditions(TrueCondition(clusterv1.ReadyCondition)) + target := setterWithConditions() + + SetMirror(target, "foo", source) + + g.Expect(Has(target, "foo")).To(BeTrue()) +} + +func TestSetAggregate(t *testing.T) { + g := NewWithT(t) + source1 := getterWithConditions(TrueCondition(clusterv1.ReadyCondition)) + source2 := getterWithConditions(TrueCondition(clusterv1.ReadyCondition)) + target := setterWithConditions() + + SetAggregate(target, "foo", []Getter{source1, source2}) + + g.Expect(Has(target, "foo")).To(BeTrue()) +} + func setterWithConditions(conditions ...*clusterv1.Condition) Setter { obj := &clusterv1.Cluster{} obj.SetConditions(conditionList(conditions...)) From 5113f801f130f2d8f0a1b90403b7d868c696c1b8 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Mon, 8 Jun 2020 20:44:20 +0200 Subject: [PATCH 3/3] Add conditions to the Machine object --- api/v1alpha2/conversion.go | 1 + api/v1alpha2/zz_generated.conversion.go | 1 + api/v1alpha3/condition_consts.go | 31 +++++++++++++ api/v1alpha3/machine_types.go | 12 +++++ api/v1alpha3/zz_generated.deepcopy.go | 7 +++ .../crd/bases/cluster.x-k8s.io_machines.yaml | 44 +++++++++++++++++++ controllers/machine_controller.go | 8 ++++ controllers/machine_controller_phases.go | 34 +++++++++++--- controllers/machine_controller_test.go | 20 ++++----- 9 files changed, 141 insertions(+), 17 deletions(-) diff --git a/api/v1alpha2/conversion.go b/api/v1alpha2/conversion.go index 98677ce2be9d..00974460d07e 100644 --- a/api/v1alpha2/conversion.go +++ b/api/v1alpha2/conversion.go @@ -118,6 +118,7 @@ func (src *Machine) ConvertTo(dstRaw conversion.Hub) error { } restoreMachineSpec(&restored.Spec, &dst.Spec) dst.Status.ObservedGeneration = restored.Status.ObservedGeneration + dst.Status.Conditions = restored.Status.Conditions return nil } diff --git a/api/v1alpha2/zz_generated.conversion.go b/api/v1alpha2/zz_generated.conversion.go index 188ed97bc4fd..c096b057d71d 100644 --- a/api/v1alpha2/zz_generated.conversion.go +++ b/api/v1alpha2/zz_generated.conversion.go @@ -918,6 +918,7 @@ func autoConvert_v1alpha3_MachineStatus_To_v1alpha2_MachineStatus(in *v1alpha3.M out.BootstrapReady = in.BootstrapReady out.InfrastructureReady = in.InfrastructureReady // WARNING: in.ObservedGeneration requires manual conversion: does not exist in peer-type + // WARNING: in.Conditions requires manual conversion: does not exist in peer-type return nil } diff --git a/api/v1alpha3/condition_consts.go b/api/v1alpha3/condition_consts.go index 4c216d2be996..e8fdf7da7930 100644 --- a/api/v1alpha3/condition_consts.go +++ b/api/v1alpha3/condition_consts.go @@ -25,3 +25,34 @@ const ( ) // ANCHOR_END: CommonConditions + +// Conditions and condition Reasons for the Machine object + +// MachineSummaryConditionsCount defines the total number of conditions on the Machine object. +const MachineSummaryConditionsCount = 2 + +const ( + // BootstrapReadyCondition reports a summary of current status of the bootstrap object defined for this machine. + // This condition is mirrored from the Ready condition in the bootstrap ref object, and + // the absence of this condition might signal problems in the reconcile external loops or the fact that + // the bootstrap provider does not not implements the Ready condition yet. + BootstrapReadyCondition ConditionType = "BootstrapReady" + + // WaitingForDataSecretFallbackReason (Severity=Info) documents a machine waiting for the bootstrap data secret + // to be available. + // NOTE: This reason is used only as a fallback when the bootstrap object is not reporting its own ready condition. + WaitingForDataSecretFallbackReason = "WaitingForDataSecret" +) + +const ( + // InfrastructureReadyCondition reports a summary of current status of the infrastructure object defined for this machine. + // This condition is mirrored from the Ready condition in the infrastructure ref object, and + // the absence of this condition might signal problems in the reconcile external loops or the fact that + // the infrastructure provider does not not implements the Ready condition yet. + InfrastructureReadyCondition ConditionType = "InfrastructureReady" + + // WaitingForInfrastructureFallbackReason (Severity=Info) documents a machine waiting for the machine infrastructure + // to be available. + // NOTE: This reason is used only as a fallback when the infrastructure object is not reporting its own ready condition. + WaitingForInfrastructureFallbackReason = "WaitingForInfrastructure" +) diff --git a/api/v1alpha3/machine_types.go b/api/v1alpha3/machine_types.go index a164f5a2368f..32fe0dc431df 100644 --- a/api/v1alpha3/machine_types.go +++ b/api/v1alpha3/machine_types.go @@ -159,6 +159,10 @@ type MachineStatus struct { // ObservedGeneration is the latest generation observed by the controller. // +optional ObservedGeneration int64 `json:"observedGeneration,omitempty"` + + // Conditions defines current service state of the Machine. + // +optional + Conditions Conditions `json:"conditions,omitempty"` } // ANCHOR_END: MachineStatus @@ -231,6 +235,14 @@ type Machine struct { Status MachineStatus `json:"status,omitempty"` } +func (m *Machine) GetConditions() Conditions { + return m.Status.Conditions +} + +func (m *Machine) SetConditions(conditions Conditions) { + m.Status.Conditions = conditions +} + // +kubebuilder:object:root=true // MachineList contains a list of Machine diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index 71b8377fa46f..a6efd2d9c6d7 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -841,6 +841,13 @@ func (in *MachineStatus) DeepCopyInto(out *MachineStatus) { *out = make(MachineAddresses, len(*in)) copy(*out, *in) } + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make(Conditions, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new MachineStatus. diff --git a/config/crd/bases/cluster.x-k8s.io_machines.yaml b/config/crd/bases/cluster.x-k8s.io_machines.yaml index 501eb79eb84a..9480df7aa308 100644 --- a/config/crd/bases/cluster.x-k8s.io_machines.yaml +++ b/config/crd/bases/cluster.x-k8s.io_machines.yaml @@ -560,6 +560,50 @@ spec: bootstrapReady: description: BootstrapReady is the state of the bootstrap provider. type: boolean + conditions: + description: Conditions defines current service state of the Machine. + items: + description: Condition defines an observation of a Cluster API resource + operational state. + properties: + lastTransitionTime: + description: Last time the condition transitioned from one status + to another. This should be when the underlying condition changed. + If that is not known, then using the time when the API field + changed is acceptable. + format: date-time + type: string + message: + description: A human readable message indicating details about + the transition. This field may be empty. + type: string + reason: + description: The reason for the condition's last transition + in CamelCase. The specific API may choose whether or not this + field is considered a guaranteed API. This field may not be + empty. + type: string + severity: + description: Severity provides an explicit classification of + Reason code, so the users or machines can immediately understand + the current situation and act accordingly. The Severity field + MUST be set only when Status=False. + type: string + status: + description: Status of the condition, one of True, False, Unknown. + type: string + type: + description: Type of condition in CamelCase or in foo.example.com/CamelCase. + Many .condition.type values are consistent across resources + like Available, but because arbitrary conditions can be useful + (see .node.status.conditions), the ability to deconflict is + important. + type: string + required: + - status + - type + type: object + type: array failureMessage: description: "FailureMessage will be set in the event that there is a terminal problem reconciling the Machine and will contain a more diff --git a/controllers/machine_controller.go b/controllers/machine_controller.go index ab7feca5d3c1..c686570c2174 100644 --- a/controllers/machine_controller.go +++ b/controllers/machine_controller.go @@ -43,6 +43,7 @@ import ( kubedrain "sigs.k8s.io/cluster-api/third_party/kubernetes-drain" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" + "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" ctrl "sigs.k8s.io/controller-runtime" @@ -166,6 +167,13 @@ func (r *MachineReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, reterr e } defer func() { + // always update the readyCondition with the summary of the machine conditions. + conditions.SetSummary(m, + // we want to surface infrastructure problems first, then the others. + conditions.WithConditionOrder(clusterv1.InfrastructureReadyCondition), + conditions.WithStepCounter(clusterv1.MachineSummaryConditionsCount), + ) + r.reconcilePhase(ctx, m) r.reconcileMetrics(ctx, m) diff --git a/controllers/machine_controller_phases.go b/controllers/machine_controller_phases.go index 73cf303a9804..1fc2732bd8a6 100644 --- a/controllers/machine_controller_phases.go +++ b/controllers/machine_controller_phases.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "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" @@ -158,6 +159,14 @@ 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 { + // 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 + } + + // 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 } @@ -172,12 +181,6 @@ func (r *MachineReconciler) reconcileBootstrap(ctx context.Context, cluster *clu } bootstrapConfig := externalResult.Result - // If the bootstrap data is populated, set ready and return. - if m.Spec.Bootstrap.DataSecretName != nil { - m.Status.BootstrapReady = true - return nil - } - // If the bootstrap config is being deleted, return early. if !bootstrapConfig.GetDeletionTimestamp().IsZero() { return nil @@ -187,7 +190,16 @@ func (r *MachineReconciler) reconcileBootstrap(ctx context.Context, cluster *clu ready, err := external.IsReady(bootstrapConfig) if err != nil { return err - } else if !ready { + } + + // Report a summary of current status of the bootstrap object defined for this machine. + conditions.SetMirror(m, clusterv1.BootstrapReadyCondition, + conditions.UnstructuredGetter(bootstrapConfig), + conditions.WithFallbackValue(ready, clusterv1.WaitingForDataSecretFallbackReason, clusterv1.ConditionSeverityInfo, ""), + ) + + // 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) } @@ -236,6 +248,14 @@ func (r *MachineReconciler) reconcileInfrastructure(ctx context.Context, cluster return err } m.Status.InfrastructureReady = ready + + // Report a summary of current status of the infrastructure object defined for this machine. + conditions.SetMirror(m, clusterv1.InfrastructureReadyCondition, + conditions.UnstructuredGetter(infraConfig), + conditions.WithFallbackValue(ready, clusterv1.WaitingForInfrastructureFallbackReason, clusterv1.ConditionSeverityInfo, ""), + ) + + // 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, diff --git a/controllers/machine_controller_test.go b/controllers/machine_controller_test.go index ee4d1574ba68..e33efb44b2ba 100644 --- a/controllers/machine_controller_test.go +++ b/controllers/machine_controller_test.go @@ -31,9 +31,9 @@ import ( "k8s.io/utils/pointer" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" "sigs.k8s.io/cluster-api/controllers/external" + "sigs.k8s.io/cluster-api/test/helpers" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/metrics" @@ -105,7 +105,7 @@ func TestMachineFinalizer(t *testing.T) { g := NewWithT(t) mr := &MachineReconciler{ - Client: fake.NewFakeClientWithScheme( + Client: helpers.NewFakeClientWithScheme( scheme.Scheme, clusterCorrectMeta, machineValidCluster, @@ -266,7 +266,7 @@ func TestMachineOwnerReference(t *testing.T) { g := NewWithT(t) mr := &MachineReconciler{ - Client: fake.NewFakeClientWithScheme( + Client: helpers.NewFakeClientWithScheme( scheme.Scheme, testCluster, machineInvalidCluster, @@ -419,11 +419,11 @@ func TestReconcileRequest(t *testing.T) { t.Run("machine should be "+tc.machine.Name, func(t *testing.T) { g := NewWithT(t) - clientFake := fake.NewFakeClientWithScheme( + clientFake := helpers.NewFakeClientWithScheme( scheme.Scheme, &testCluster, &tc.machine, - external.TestGenericInfrastructureCRD, + external.TestGenericInfrastructureCRD.DeepCopy(), &infraConfig, ) @@ -546,7 +546,7 @@ func TestReconcileDeleteExternal(t *testing.T) { } r := &MachineReconciler{ - Client: fake.NewFakeClientWithScheme(scheme.Scheme, objs...), + Client: helpers.NewFakeClientWithScheme(scheme.Scheme, objs...), Log: log.Log, scheme: scheme.Scheme, } @@ -590,7 +590,7 @@ func TestRemoveMachineFinalizerAfterDeleteReconcile(t *testing.T) { } key := client.ObjectKey{Namespace: m.Namespace, Name: m.Name} mr := &MachineReconciler{ - Client: fake.NewFakeClientWithScheme(scheme.Scheme, testCluster, m), + Client: helpers.NewFakeClientWithScheme(scheme.Scheme, testCluster, m), Log: log.Log, scheme: scheme.Scheme, } @@ -667,7 +667,7 @@ func TestReconcileMetrics(t *testing.T) { objs = append(objs, machine) r := &MachineReconciler{ - Client: fake.NewFakeClientWithScheme(scheme.Scheme, objs...), + Client: helpers.NewFakeClientWithScheme(scheme.Scheme, objs...), Log: log.Log, scheme: scheme.Scheme, } @@ -778,7 +778,7 @@ func Test_clusterToActiveMachines(t *testing.T) { objs = append(objs, m2) r := &MachineReconciler{ - Client: fake.NewFakeClientWithScheme(scheme.Scheme, objs...), + Client: helpers.NewFakeClientWithScheme(scheme.Scheme, objs...), Log: log.Log, scheme: scheme.Scheme, } @@ -952,7 +952,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) { } mr := &MachineReconciler{ - Client: fake.NewFakeClientWithScheme( + Client: helpers.NewFakeClientWithScheme( scheme.Scheme, tc.cluster, tc.machine,