From 526229168a65fe2a73d29a2bf188e87146067eab Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Wed, 13 Nov 2024 22:33:28 +0100 Subject: [PATCH] Refine v1beta2 mirror conditions # Conflicts: # internal/controllers/cluster/cluster_controller_status.go # internal/controllers/cluster/cluster_controller_status_test.go --- api/v1beta1/cluster_types.go | 18 ++- api/v1beta1/machine_types.go | 20 ++- .../cluster/cluster_controller_status.go | 113 +++++---------- .../cluster/cluster_controller_status_test.go | 109 +++++--------- .../machine/machine_controller_status.go | 112 ++++++--------- .../machine/machine_controller_status_test.go | 136 +++++++++--------- util/conditions/v1beta2/mirror.go | 19 ++- 7 files changed, 224 insertions(+), 303 deletions(-) diff --git a/api/v1beta1/cluster_types.go b/api/v1beta1/cluster_types.go index 23ceaf2d30fb..11af512fc561 100644 --- a/api/v1beta1/cluster_types.go +++ b/api/v1beta1/cluster_types.go @@ -126,13 +126,16 @@ const ( // ClusterInfrastructureReadyV1Beta2Condition mirrors Cluster's infrastructure Ready condition. ClusterInfrastructureReadyV1Beta2Condition = InfrastructureReadyV1Beta2Condition + // ClusterInfrastructureReadyV1Beta2Reason surfaces when the cluster infrastructure is ready. + ClusterInfrastructureReadyV1Beta2Reason = ReadyV1Beta2Reason + + // ClusterInfrastructureNotReadyV1Beta2Reason surfaces when the cluster infrastructure is not ready. + ClusterInfrastructureNotReadyV1Beta2Reason = NotReadyV1Beta2Reason + // ClusterInfrastructureInvalidConditionReportedV1Beta2Reason surfaces a infrastructure Ready condition (read from an infra cluster object) which is invalid // (e.g. its status is missing). ClusterInfrastructureInvalidConditionReportedV1Beta2Reason = InvalidConditionReportedV1Beta2Reason - // ClusterInfrastructureReadyNoReasonReportedV1Beta2Reason applies to a infrastructure Ready condition (read from an infra cluster object) that reports no reason. - ClusterInfrastructureReadyNoReasonReportedV1Beta2Reason = NoReasonReportedV1Beta2Reason - // ClusterInfrastructureInternalErrorV1Beta2Reason surfaces unexpected failures when reading an infra cluster object. ClusterInfrastructureInternalErrorV1Beta2Reason = InternalErrorV1Beta2Reason @@ -170,13 +173,16 @@ const ( // ClusterControlPlaneAvailableV1Beta2Condition is a mirror of Cluster's control plane Available condition. ClusterControlPlaneAvailableV1Beta2Condition = "ControlPlaneAvailable" + // ClusterControlPlaneAvailableV1Beta2Reason surfaces when the cluster control plane is available. + ClusterControlPlaneAvailableV1Beta2Reason = AvailableV1Beta2Reason + + // ClusterControlPlaneNotAvailableV1Beta2Reason surfaces when the cluster control plane is not available. + ClusterControlPlaneNotAvailableV1Beta2Reason = NotAvailableV1Beta2Reason + // ClusterControlPlaneInvalidConditionReportedV1Beta2Reason surfaces a control plane Available condition (read from a control plane object) which is invalid. // (e.g. its status is missing). ClusterControlPlaneInvalidConditionReportedV1Beta2Reason = InvalidConditionReportedV1Beta2Reason - // ClusterControlPlaneAvailableNoReasonReportedV1Beta2Reason applies to a control plane Available condition (read from a control plane object) that reports no reason. - ClusterControlPlaneAvailableNoReasonReportedV1Beta2Reason = NoReasonReportedV1Beta2Reason - // ClusterControlPlaneInternalErrorV1Beta2Reason surfaces unexpected failures when reading a control plane object. ClusterControlPlaneInternalErrorV1Beta2Reason = InternalErrorV1Beta2Reason diff --git a/api/v1beta1/machine_types.go b/api/v1beta1/machine_types.go index ce225a88cc63..501a7ea8c7b0 100644 --- a/api/v1beta1/machine_types.go +++ b/api/v1beta1/machine_types.go @@ -152,13 +152,16 @@ const ( // from a BoostrapConfig object referenced from the machine). MachineBootstrapDataSecretProvidedV1Beta2Reason = "DataSecretProvided" + // MachineBootstrapConfigReadyV1Beta2Reason surfaces when the machine bootstrap config is ready. + MachineBootstrapConfigReadyV1Beta2Reason = ReadyV1Beta2Reason + + // MachineBootstrapConfigNotReadyV1Beta2Reason surfaces when the machine bootstrap config is not ready. + MachineBootstrapConfigNotReadyV1Beta2Reason = NotReadyV1Beta2Reason + // MachineBootstrapConfigInvalidConditionReportedV1Beta2Reason surfaces a BootstrapConfig Ready condition (read from a bootstrap config object) which is invalid. // (e.g. its status is missing). MachineBootstrapConfigInvalidConditionReportedV1Beta2Reason = InvalidConditionReportedV1Beta2Reason - // MachineBootstrapConfigReadyNoReasonReportedV1Beta2Reason applies to a BootstrapConfig Ready condition (read from a bootstrap config object) that reports no reason. - MachineBootstrapConfigReadyNoReasonReportedV1Beta2Reason = NoReasonReportedV1Beta2Reason - // MachineBootstrapConfigInternalErrorV1Beta2Reason surfaces unexpected failures when reading a BootstrapConfig object. MachineBootstrapConfigInternalErrorV1Beta2Reason = InternalErrorV1Beta2Reason @@ -175,16 +178,19 @@ const ( // Machine's InfrastructureReady condition and corresponding reasons that will be used in v1Beta2 API version. // Note: when possible, InfrastructureReady condition will use reasons surfaced from the underlying infra machine object. const ( - // MachineInfrastructureReadyV1Beta2Condition mirrors the corresponding Ready condition from the Machine's Infrastructure resource. + // MachineInfrastructureReadyV1Beta2Condition mirrors the corresponding Ready condition from the Machine's infrastructure resource. MachineInfrastructureReadyV1Beta2Condition = InfrastructureReadyV1Beta2Condition + // MachineInfrastructureReadyV1Beta2Reason surfaces when the machine infrastructure is ready. + MachineInfrastructureReadyV1Beta2Reason = ReadyV1Beta2Reason + + // MachineInfrastructureNotReadyV1Beta2Reason surfaces when the machine infrastructure is not ready. + MachineInfrastructureNotReadyV1Beta2Reason = NotReadyV1Beta2Reason + // MachineInfrastructureInvalidConditionReportedV1Beta2Reason surfaces a infrastructure Ready condition (read from an infra machine object) which is invalid. // (e.g. its status is missing). MachineInfrastructureInvalidConditionReportedV1Beta2Reason = InvalidConditionReportedV1Beta2Reason - // MachineInfrastructureReadyNoReasonReportedV1Beta2Reason applies to a infrastructure Ready condition (read from an infra machine object) that reports no reason. - MachineInfrastructureReadyNoReasonReportedV1Beta2Reason = NoReasonReportedV1Beta2Reason - // MachineInfrastructureInternalErrorV1Beta2Reason surfaces unexpected failures when reading an infra machine object. MachineInfrastructureInternalErrorV1Beta2Reason = InternalErrorV1Beta2Reason diff --git a/internal/controllers/cluster/cluster_controller_status.go b/internal/controllers/cluster/cluster_controller_status.go index cfb140fb6a56..0cc9a9e13962 100644 --- a/internal/controllers/cluster/cluster_controller_status.go +++ b/internal/controllers/cluster/cluster_controller_status.go @@ -241,22 +241,31 @@ func setInfrastructureReadyCondition(_ context.Context, cluster *clusterv1.Clust } if infraCluster != nil { - if err := v1beta2conditions.SetMirrorConditionFromUnstructured( - infraCluster, cluster, + ready, err := v1beta2conditions.NewMirrorConditionFromUnstructured( + infraCluster, contract.InfrastructureCluster().ReadyConditionType(), v1beta2conditions.TargetConditionType(clusterv1.ClusterInfrastructureReadyV1Beta2Condition), v1beta2conditions.FallbackCondition{ Status: v1beta2conditions.BoolToStatus(cluster.Status.InfrastructureReady), - Reason: clusterv1.ClusterInfrastructureReadyNoReasonReportedV1Beta2Reason, + Reason: fallbackReason(cluster.Status.InfrastructureReady, clusterv1.ClusterInfrastructureReadyV1Beta2Reason, clusterv1.ClusterInfrastructureNotReadyV1Beta2Reason), Message: infrastructureReadyFallBackMessage(cluster.Spec.InfrastructureRef.Kind, cluster.Status.InfrastructureReady), }, - ); err != nil { + ) + if err != nil { v1beta2conditions.Set(cluster, metav1.Condition{ Type: clusterv1.ClusterInfrastructureReadyV1Beta2Condition, Status: metav1.ConditionUnknown, Reason: clusterv1.ClusterInfrastructureInvalidConditionReportedV1Beta2Reason, Message: err.Error(), }) + return + } + + // In case condition has NoReasonReported and status true, we assume it is a v1beta1 condition + // and replace the reason with something less confusing. + if ready.Reason == v1beta2conditions.NoReasonReported && ready.Status == metav1.ConditionTrue { + ready.Reason = clusterv1.ClusterInfrastructureReadyV1Beta2Reason } + v1beta2conditions.Set(cluster, *ready) return } @@ -332,22 +341,31 @@ func setControlPlaneAvailableCondition(_ context.Context, cluster *clusterv1.Clu } if controlPlane != nil { - if err := v1beta2conditions.SetMirrorConditionFromUnstructured( - controlPlane, cluster, + available, err := v1beta2conditions.NewMirrorConditionFromUnstructured( + controlPlane, contract.ControlPlane().AvailableConditionType(), v1beta2conditions.TargetConditionType(clusterv1.ClusterControlPlaneAvailableV1Beta2Condition), v1beta2conditions.FallbackCondition{ Status: v1beta2conditions.BoolToStatus(cluster.Status.ControlPlaneReady), - Reason: clusterv1.ClusterControlPlaneAvailableNoReasonReportedV1Beta2Reason, + Reason: fallbackReason(cluster.Status.ControlPlaneReady, clusterv1.ClusterControlPlaneAvailableV1Beta2Reason, clusterv1.ClusterControlPlaneNotAvailableV1Beta2Reason), Message: controlPlaneAvailableFallBackMessage(cluster.Spec.ControlPlaneRef.Kind, cluster.Status.ControlPlaneReady), }, - ); err != nil { + ) + if err != nil { v1beta2conditions.Set(cluster, metav1.Condition{ Type: clusterv1.ClusterControlPlaneAvailableV1Beta2Condition, Status: metav1.ConditionUnknown, Reason: clusterv1.ClusterControlPlaneInvalidConditionReportedV1Beta2Reason, Message: err.Error(), }) + return } + + // In case condition has NoReasonReported and status true, we assume it is a v1beta1 condition + // and replace the reason with something less confusing. + if available.Reason == v1beta2conditions.NoReasonReported && available.Status == metav1.ConditionTrue { + available.Reason = clusterv1.ClusterControlPlaneAvailableV1Beta2Reason + } + v1beta2conditions.Set(cluster, *available) return } @@ -958,21 +976,6 @@ func setAvailableCondition(ctx context.Context, cluster *clusterv1.Cluster) { summaryOpts = append(summaryOpts, v1beta2conditions.IgnoreTypesIfMissing{clusterv1.ClusterTopologyReconciledV1Beta2Condition}) } - // Add overrides for conditions we want to surface in the Available condition with slightly different messages. - var overrideConditions v1beta2conditions.OverrideConditions - - if infrastructureReadyCondition := calculateInfrastructureReadyForSummary(cluster); infrastructureReadyCondition != nil { - overrideConditions = append(overrideConditions, *infrastructureReadyCondition) - } - - if controlPlaneAvailableCondition := calculateControlPlaneAvailableForSummary(cluster); controlPlaneAvailableCondition != nil { - overrideConditions = append(overrideConditions, *controlPlaneAvailableCondition) - } - - if len(overrideConditions) > 0 { - summaryOpts = append(summaryOpts, overrideConditions) - } - availableCondition, err := v1beta2conditions.NewSummaryCondition(cluster, clusterv1.ClusterAvailableV1Beta2Condition, summaryOpts...) if err != nil { @@ -990,63 +993,25 @@ func setAvailableCondition(ctx context.Context, cluster *clusterv1.Cluster) { v1beta2conditions.Set(cluster, *availableCondition) } -func infrastructureReadyFallBackMessage(kind string, ready bool) string { - return fmt.Sprintf("%s status.ready is %t", kind, ready) -} - -func controlPlaneAvailableFallBackMessage(kind string, ready bool) string { - return fmt.Sprintf("%s status.ready is %t", kind, ready) -} - -func calculateInfrastructureReadyForSummary(cluster *clusterv1.Cluster) *v1beta2conditions.ConditionWithOwnerInfo { - infrastructureReadyCondition := v1beta2conditions.Get(cluster, clusterv1.ClusterInfrastructureReadyV1Beta2Condition) - - if infrastructureReadyCondition == nil { - return nil - } - - message := infrastructureReadyCondition.Message - if infrastructureReadyCondition.Status == metav1.ConditionTrue && cluster.Spec.InfrastructureRef != nil && infrastructureReadyCondition.Message == infrastructureReadyFallBackMessage(cluster.Spec.InfrastructureRef.Kind, cluster.Status.InfrastructureReady) { - message = "" - } - - return &v1beta2conditions.ConditionWithOwnerInfo{ - OwnerResource: v1beta2conditions.ConditionOwnerInfo{ - Kind: "Cluster", - Name: cluster.Name, - }, - Condition: metav1.Condition{ - Type: infrastructureReadyCondition.Type, - Status: infrastructureReadyCondition.Status, - Reason: infrastructureReadyCondition.Reason, - Message: message, - }, +func fallbackReason(status bool, trueReason, falseReason string) string { + if status { + return trueReason } + return falseReason } -func calculateControlPlaneAvailableForSummary(cluster *clusterv1.Cluster) *v1beta2conditions.ConditionWithOwnerInfo { - controlPlaneAvailableCondition := v1beta2conditions.Get(cluster, clusterv1.ClusterControlPlaneAvailableV1Beta2Condition) - if controlPlaneAvailableCondition == nil { - return nil - } - - message := controlPlaneAvailableCondition.Message - if controlPlaneAvailableCondition.Status == metav1.ConditionTrue && cluster.Spec.ControlPlaneRef != nil && controlPlaneAvailableCondition.Message == controlPlaneAvailableFallBackMessage(cluster.Spec.ControlPlaneRef.Kind, cluster.Status.ControlPlaneReady) { - message = "" +func infrastructureReadyFallBackMessage(kind string, ready bool) string { + if ready { + return "" } + return fmt.Sprintf("%s status.ready is %t", kind, ready) +} - return &v1beta2conditions.ConditionWithOwnerInfo{ - OwnerResource: v1beta2conditions.ConditionOwnerInfo{ - Kind: "Cluster", - Name: cluster.Name, - }, - Condition: metav1.Condition{ - Type: controlPlaneAvailableCondition.Type, - Status: controlPlaneAvailableCondition.Status, - Reason: controlPlaneAvailableCondition.Reason, - Message: message, - }, +func controlPlaneAvailableFallBackMessage(kind string, ready bool) string { + if ready { + return "" } + return fmt.Sprintf("%s status.ready is %t", kind, ready) } func aggregateUnhealthyMachines(machines collections.Machines) string { diff --git a/internal/controllers/cluster/cluster_controller_status_test.go b/internal/controllers/cluster/cluster_controller_status_test.go index a7b55e782aa6..09270cd84258 100644 --- a/internal/controllers/cluster/cluster_controller_status_test.go +++ b/internal/controllers/cluster/cluster_controller_status_test.go @@ -242,12 +242,24 @@ func TestSetInfrastructureReadyCondition(t *testing.T) { { name: "mirror Ready condition from infra cluster", cluster: fakeCluster("c", infrastructureRef{Kind: "FakeInfraCluster"}), - infraCluster: fakeInfraCluster("i1", condition{Type: "Ready", Status: "False", Message: "some message"}), + infraCluster: fakeInfraCluster("i1", condition{Type: "Ready", Status: "False", Message: "some message", Reason: "SomeReason"}), infraClusterIsNotFound: false, expectCondition: metav1.Condition{ Type: clusterv1.ClusterInfrastructureReadyV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: clusterv1.ClusterInfrastructureReadyNoReasonReportedV1Beta2Reason, + Reason: "SomeReason", + Message: "some message", + }, + }, + { + name: "mirror Ready condition from infra cluster (true)", + cluster: fakeCluster("c", infrastructureRef{Kind: "FakeInfraCluster"}), + infraCluster: fakeInfraCluster("i1", condition{Type: "Ready", Status: "True", Message: "some message"}), // reason not set for v1beta1 conditions + infraClusterIsNotFound: false, + expectCondition: metav1.Condition{ + Type: clusterv1.ClusterInfrastructureReadyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.ClusterInfrastructureReadyV1Beta2Reason, // reason fixed up Message: "some message", }, }, @@ -259,7 +271,7 @@ func TestSetInfrastructureReadyCondition(t *testing.T) { expectCondition: metav1.Condition{ Type: clusterv1.ClusterInfrastructureReadyV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: clusterv1.ClusterInfrastructureReadyNoReasonReportedV1Beta2Reason, + Reason: clusterv1.ClusterInfrastructureNotReadyV1Beta2Reason, Message: "FakeInfraCluster status.ready is false", }, }, @@ -269,10 +281,9 @@ func TestSetInfrastructureReadyCondition(t *testing.T) { infraCluster: fakeInfraCluster("i1", ready(true)), infraClusterIsNotFound: false, expectCondition: metav1.Condition{ - Type: clusterv1.ClusterInfrastructureReadyV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: clusterv1.ClusterInfrastructureReadyNoReasonReportedV1Beta2Reason, - Message: "FakeInfraCluster status.ready is true", + Type: clusterv1.ClusterInfrastructureReadyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.ClusterInfrastructureReadyV1Beta2Reason, }, }, { @@ -396,12 +407,24 @@ func TestSetControlPlaneAvailableCondition(t *testing.T) { { name: "mirror Available condition from control plane", cluster: fakeCluster("c", controlPlaneRef{Kind: "FakeControlPlane"}), - controlPlane: fakeControlPlane("cp1", condition{Type: "Available", Status: "False", Message: "some message"}), + controlPlane: fakeControlPlane("cp1", condition{Type: "Available", Status: "False", Message: "some message", Reason: "SomeReason"}), controlPlaneIsNotFound: false, expectCondition: metav1.Condition{ Type: clusterv1.ClusterControlPlaneAvailableV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: clusterv1.ClusterControlPlaneAvailableNoReasonReportedV1Beta2Reason, + Reason: "SomeReason", + Message: "some message", + }, + }, + { + name: "mirror Available condition from control plane (true)", + cluster: fakeCluster("c", controlPlaneRef{Kind: "FakeControlPlane"}), + controlPlane: fakeControlPlane("cp1", condition{Type: "Available", Status: "True", Message: "some message"}), // reason not set for v1beta1 conditions + controlPlaneIsNotFound: false, + expectCondition: metav1.Condition{ + Type: clusterv1.ClusterControlPlaneAvailableV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.ClusterControlPlaneAvailableV1Beta2Reason, // reason fixed up Message: "some message", }, }, @@ -413,7 +436,7 @@ func TestSetControlPlaneAvailableCondition(t *testing.T) { expectCondition: metav1.Condition{ Type: clusterv1.ClusterControlPlaneAvailableV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: clusterv1.ClusterControlPlaneAvailableNoReasonReportedV1Beta2Reason, + Reason: clusterv1.ClusterControlPlaneNotAvailableV1Beta2Reason, Message: "FakeControlPlane status.ready is false", }, }, @@ -423,10 +446,9 @@ func TestSetControlPlaneAvailableCondition(t *testing.T) { controlPlane: fakeControlPlane("cp1", ready(true)), controlPlaneIsNotFound: false, expectCondition: metav1.Condition{ - Type: clusterv1.ClusterControlPlaneAvailableV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: clusterv1.ClusterControlPlaneAvailableNoReasonReportedV1Beta2Reason, - Message: "FakeControlPlane status.ready is true", + Type: clusterv1.ClusterControlPlaneAvailableV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.ClusterControlPlaneAvailableV1Beta2Reason, }, }, { @@ -1979,65 +2001,6 @@ func TestSetAvailableCondition(t *testing.T) { Message: "* Deleting: Some message", }, }, - { - name: "Drops messages from InfraCluster and ControlPlane when using fallback to fields", - cluster: &clusterv1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-test", - Namespace: metav1.NamespaceDefault, - }, - Spec: clusterv1.ClusterSpec{ - InfrastructureRef: &corev1.ObjectReference{Kind: "AWSCluster"}, - ControlPlaneRef: &corev1.ObjectReference{Kind: "KubeadmControlPlane"}, - }, - Status: clusterv1.ClusterStatus{ - InfrastructureReady: true, - ControlPlaneReady: true, - V1Beta2: &clusterv1.ClusterV1Beta2Status{ - Conditions: []metav1.Condition{ - { - Type: clusterv1.ClusterInfrastructureReadyV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: "Foo", - Message: infrastructureReadyFallBackMessage("AWSCluster", true), - }, - { - Type: clusterv1.ClusterControlPlaneAvailableV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: "Foo", - Message: controlPlaneAvailableFallBackMessage("KubeadmControlPlane", true), - }, - { - Type: clusterv1.ClusterWorkersAvailableV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: "Foo", - }, - { - Type: clusterv1.ClusterRemoteConnectionProbeV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: clusterv1.ClusterRemoteConnectionProbeSucceededV1Beta2Reason, - }, - { - Type: clusterv1.ClusterDeletingV1Beta2Condition, - Status: metav1.ConditionFalse, - Reason: "Foo", - }, - { - Type: "MyAvailabilityGate", - Status: metav1.ConditionFalse, - Reason: "SomeReason", - Message: "Some message", - }, - }, - }, - }, - }, - expectCondition: metav1.Condition{ - Type: clusterv1.ClusterAvailableV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: clusterv1.ClusterAvailableV1Beta2Reason, - }, - }, { name: "Surfaces message from TopologyReconciled for reason that doesn't affect availability (no other issues)", cluster: &clusterv1.Cluster{ diff --git a/internal/controllers/machine/machine_controller_status.go b/internal/controllers/machine/machine_controller_status.go index b3141fd01f53..870490b4d21b 100644 --- a/internal/controllers/machine/machine_controller_status.go +++ b/internal/controllers/machine/machine_controller_status.go @@ -65,14 +65,10 @@ func (r *Reconciler) updateStatus(ctx context.Context, s *scope) { // in reconcileDelete (e.g. status.Deletion nested fields), and also in the defer patch at the end of the main reconcile loop (status.ObservedGeneration) etc. // Note: also other controllers adds conditions to the machine object (machine's owner controller sets the UpToDate condition, // MHC controller sets HealthCheckSucceeded and OwnerRemediated conditions, KCP sets conditions about etcd and control plane pods). - - // TODO: Set the uptodate condition for standalone pods - setDeletingCondition(ctx, s.machine, s.reconcileDeleteExecuted, s.deletingReason, s.deletingMessage) - setReadyCondition(ctx, s.machine) - setAvailableCondition(ctx, s.machine) + // TODO: Set the uptodate condition for standalone pods setMachinePhaseAndLastUpdated(ctx, s.machine) } @@ -88,22 +84,31 @@ func setBootstrapReadyCondition(_ context.Context, machine *clusterv1.Machine, b } if bootstrapConfig != nil { - if err := v1beta2conditions.SetMirrorConditionFromUnstructured( - bootstrapConfig, machine, + ready, err := v1beta2conditions.NewMirrorConditionFromUnstructured( + bootstrapConfig, contract.Bootstrap().ReadyConditionType(), v1beta2conditions.TargetConditionType(clusterv1.MachineBootstrapConfigReadyV1Beta2Condition), v1beta2conditions.FallbackCondition{ Status: v1beta2conditions.BoolToStatus(machine.Status.BootstrapReady), - Reason: clusterv1.MachineBootstrapConfigReadyNoReasonReportedV1Beta2Reason, + Reason: fallbackReason(machine.Status.BootstrapReady, clusterv1.MachineBootstrapConfigReadyV1Beta2Reason, clusterv1.MachineBootstrapConfigNotReadyV1Beta2Reason), Message: bootstrapConfigReadyFallBackMessage(machine.Spec.Bootstrap.ConfigRef.Kind, machine.Status.BootstrapReady), }, - ); err != nil { + ) + if err != nil { v1beta2conditions.Set(machine, metav1.Condition{ Type: clusterv1.MachineBootstrapConfigReadyV1Beta2Condition, Status: metav1.ConditionUnknown, Reason: clusterv1.MachineBootstrapConfigInvalidConditionReportedV1Beta2Reason, Message: err.Error(), }) + return } + + // In case condition has NoReasonReported and status true, we assume it is a v1beta1 condition + // and replace the reason with something less confusing. + if ready.Reason == v1beta2conditions.NoReasonReported && ready.Status == metav1.ConditionTrue { + ready.Reason = clusterv1.MachineBootstrapConfigReadyV1Beta2Reason + } + v1beta2conditions.Set(machine, *ready) return } @@ -142,28 +147,47 @@ func setBootstrapReadyCondition(_ context.Context, machine *clusterv1.Machine, b }) } +func fallbackReason(status bool, trueReason, falseReason string) string { + if status { + return trueReason + } + return falseReason +} + func bootstrapConfigReadyFallBackMessage(kind string, ready bool) string { + if ready { + return "" + } return fmt.Sprintf("%s status.ready is %t", kind, ready) } func setInfrastructureReadyCondition(_ context.Context, machine *clusterv1.Machine, infraMachine *unstructured.Unstructured, infraMachineIsNotFound bool) { if infraMachine != nil { - if err := v1beta2conditions.SetMirrorConditionFromUnstructured( - infraMachine, machine, + ready, err := v1beta2conditions.NewMirrorConditionFromUnstructured( + infraMachine, contract.InfrastructureMachine().ReadyConditionType(), v1beta2conditions.TargetConditionType(clusterv1.MachineInfrastructureReadyV1Beta2Condition), v1beta2conditions.FallbackCondition{ Status: v1beta2conditions.BoolToStatus(machine.Status.InfrastructureReady), - Reason: clusterv1.MachineInfrastructureReadyNoReasonReportedV1Beta2Reason, + Reason: fallbackReason(machine.Status.InfrastructureReady, clusterv1.MachineInfrastructureReadyV1Beta2Reason, clusterv1.MachineInfrastructureNotReadyV1Beta2Reason), Message: infrastructureReadyFallBackMessage(machine.Spec.InfrastructureRef.Kind, machine.Status.InfrastructureReady), }, - ); err != nil { + ) + if err != nil { v1beta2conditions.Set(machine, metav1.Condition{ Type: clusterv1.MachineInfrastructureReadyV1Beta2Condition, Status: metav1.ConditionUnknown, Reason: clusterv1.MachineInfrastructureInvalidConditionReportedV1Beta2Reason, Message: err.Error(), }) + return + } + + // In case condition has NoReasonReported and status true, we assume it is a v1beta1 condition + // and replace the reason with something less confusing. + if ready.Reason == v1beta2conditions.NoReasonReported && ready.Status == metav1.ConditionTrue { + ready.Reason = clusterv1.MachineInfrastructureReadyV1Beta2Reason } + v1beta2conditions.Set(machine, *ready) return } @@ -225,6 +249,9 @@ func setInfrastructureReadyCondition(_ context.Context, machine *clusterv1.Machi } func infrastructureReadyFallBackMessage(kind string, ready bool) string { + if ready { + return "" + } return fmt.Sprintf("%s status.ready is %t", kind, ready) } @@ -552,14 +579,6 @@ func setReadyCondition(ctx context.Context, machine *clusterv1.Machine) { overrideConditions = append(overrideConditions, calculateDeletingConditionForSummary(machine)) } - if infrastructureReadyCondition := calculateInfrastructureReadyForSummary(machine); infrastructureReadyCondition != nil { - overrideConditions = append(overrideConditions, *infrastructureReadyCondition) - } - - if bootstrapReadyCondition := calculateBootstrapConfigReadyForSummary(machine); bootstrapReadyCondition != nil { - overrideConditions = append(overrideConditions, *bootstrapReadyCondition) - } - if len(overrideConditions) > 0 { summaryOpts = append(summaryOpts, overrideConditions) } @@ -624,57 +643,6 @@ func calculateDeletingConditionForSummary(machine *clusterv1.Machine) v1beta2con } } -func calculateInfrastructureReadyForSummary(machine *clusterv1.Machine) *v1beta2conditions.ConditionWithOwnerInfo { - infrastructureReadyCondition := v1beta2conditions.Get(machine, clusterv1.MachineInfrastructureReadyV1Beta2Condition) - - if infrastructureReadyCondition == nil { - return nil - } - - message := infrastructureReadyCondition.Message - if infrastructureReadyCondition.Status == metav1.ConditionTrue && infrastructureReadyCondition.Message == infrastructureReadyFallBackMessage(machine.Spec.InfrastructureRef.Kind, machine.Status.InfrastructureReady) { - message = "" - } - - return &v1beta2conditions.ConditionWithOwnerInfo{ - OwnerResource: v1beta2conditions.ConditionOwnerInfo{ - Kind: "Machine", - Name: machine.Name, - }, - Condition: metav1.Condition{ - Type: infrastructureReadyCondition.Type, - Status: infrastructureReadyCondition.Status, - Reason: infrastructureReadyCondition.Reason, - Message: message, - }, - } -} - -func calculateBootstrapConfigReadyForSummary(machine *clusterv1.Machine) *v1beta2conditions.ConditionWithOwnerInfo { - bootstrapConfigReadyCondition := v1beta2conditions.Get(machine, clusterv1.MachineBootstrapConfigReadyV1Beta2Condition) - if bootstrapConfigReadyCondition == nil { - return nil - } - - message := bootstrapConfigReadyCondition.Message - if bootstrapConfigReadyCondition.Status == metav1.ConditionTrue && machine.Spec.Bootstrap.ConfigRef != nil && bootstrapConfigReadyCondition.Message == bootstrapConfigReadyFallBackMessage(machine.Spec.Bootstrap.ConfigRef.Kind, machine.Status.BootstrapReady) { - message = "" - } - - return &v1beta2conditions.ConditionWithOwnerInfo{ - OwnerResource: v1beta2conditions.ConditionOwnerInfo{ - Kind: "Machine", - Name: machine.Name, - }, - Condition: metav1.Condition{ - Type: bootstrapConfigReadyCondition.Type, - Status: bootstrapConfigReadyCondition.Status, - Reason: bootstrapConfigReadyCondition.Reason, - Message: message, - }, - } -} - func setAvailableCondition(ctx context.Context, machine *clusterv1.Machine) { log := ctrl.LoggerFrom(ctx) readyCondition := v1beta2conditions.Get(machine, clusterv1.MachineReadyV1Beta2Condition) diff --git a/internal/controllers/machine/machine_controller_status_test.go b/internal/controllers/machine/machine_controller_status_test.go index 0c18884471b2..4ffcf237b502 100644 --- a/internal/controllers/machine/machine_controller_status_test.go +++ b/internal/controllers/machine/machine_controller_status_test.go @@ -85,6 +85,35 @@ func TestSetBootstrapReadyCondition(t *testing.T) { Reason: clusterv1.MachineBootstrapDataSecretProvidedV1Beta2Reason, }, }, + { + name: "mirror Ready condition from bootstrap config (true)", + machine: defaultMachine.DeepCopy(), + bootstrapConfig: &unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "GenericBootstrapConfig", + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "metadata": map[string]interface{}{ + "name": "bootstrap-config1", + "namespace": metav1.NamespaceDefault, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "type": "Ready", + "status": "True", + // reason not set for v1beta1 conditions + "message": "some message", + }, + }, + }, + }}, + bootstrapConfigIsNotFound: false, + expectCondition: metav1.Condition{ + Type: clusterv1.MachineBootstrapConfigReadyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineBootstrapConfigReadyV1Beta2Reason, // reason fixed up + Message: "some message", + }, + }, { name: "mirror Ready condition from bootstrap config", machine: defaultMachine.DeepCopy(), @@ -100,6 +129,7 @@ func TestSetBootstrapReadyCondition(t *testing.T) { map[string]interface{}{ "type": "Ready", "status": "False", + "reason": "SomeReason", "message": "some message", }, }, @@ -109,7 +139,7 @@ func TestSetBootstrapReadyCondition(t *testing.T) { expectCondition: metav1.Condition{ Type: clusterv1.MachineBootstrapConfigReadyV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: clusterv1.MachineBootstrapConfigReadyNoReasonReportedV1Beta2Reason, + Reason: "SomeReason", Message: "some message", }, }, @@ -129,7 +159,7 @@ func TestSetBootstrapReadyCondition(t *testing.T) { expectCondition: metav1.Condition{ Type: clusterv1.MachineBootstrapConfigReadyV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: clusterv1.MachineBootstrapConfigReadyNoReasonReportedV1Beta2Reason, + Reason: clusterv1.MachineBootstrapConfigNotReadyV1Beta2Reason, Message: "GenericBootstrapConfig status.ready is false", }, }, @@ -153,10 +183,9 @@ func TestSetBootstrapReadyCondition(t *testing.T) { }}, bootstrapConfigIsNotFound: false, expectCondition: metav1.Condition{ - Type: clusterv1.MachineBootstrapConfigReadyV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: clusterv1.MachineBootstrapConfigReadyNoReasonReportedV1Beta2Reason, - Message: "GenericBootstrapConfig status.ready is true", + Type: clusterv1.MachineBootstrapConfigReadyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineBootstrapConfigReadyV1Beta2Reason, }, }, { @@ -279,6 +308,35 @@ func TestSetInfrastructureReadyCondition(t *testing.T) { infraMachineIsNotFound bool expectCondition metav1.Condition }{ + { + name: "mirror Ready condition from infra machine (true)", + machine: defaultMachine.DeepCopy(), + infraMachine: &unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "GenericInfrastructureMachine", + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "metadata": map[string]interface{}{ + "name": "infra-machine1", + "namespace": metav1.NamespaceDefault, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "type": "Ready", + "status": "True", + // reason not set for v1beta1 conditions + "message": "some message", + }, + }, + }, + }}, + infraMachineIsNotFound: false, + expectCondition: metav1.Condition{ + Type: clusterv1.MachineInfrastructureReadyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineInfrastructureReadyV1Beta2Reason, // reason fixed up + Message: "some message", + }, + }, { name: "mirror Ready condition from infra machine", machine: defaultMachine.DeepCopy(), @@ -294,6 +352,7 @@ func TestSetInfrastructureReadyCondition(t *testing.T) { map[string]interface{}{ "type": "Ready", "status": "False", + "reason": "SomeReason", "message": "some message", }, }, @@ -303,7 +362,7 @@ func TestSetInfrastructureReadyCondition(t *testing.T) { expectCondition: metav1.Condition{ Type: clusterv1.MachineInfrastructureReadyV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: clusterv1.MachineInfrastructureReadyNoReasonReportedV1Beta2Reason, + Reason: "SomeReason", Message: "some message", }, }, @@ -323,7 +382,7 @@ func TestSetInfrastructureReadyCondition(t *testing.T) { expectCondition: metav1.Condition{ Type: clusterv1.MachineInfrastructureReadyV1Beta2Condition, Status: metav1.ConditionFalse, - Reason: clusterv1.MachineInfrastructureReadyNoReasonReportedV1Beta2Reason, + Reason: clusterv1.MachineInfrastructureNotReadyV1Beta2Reason, Message: "GenericInfrastructureMachine status.ready is false", }, }, @@ -347,10 +406,9 @@ func TestSetInfrastructureReadyCondition(t *testing.T) { }}, infraMachineIsNotFound: false, expectCondition: metav1.Condition{ - Type: clusterv1.MachineInfrastructureReadyV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: clusterv1.MachineInfrastructureReadyNoReasonReportedV1Beta2Reason, - Message: "GenericInfrastructureMachine status.ready is true", + Type: clusterv1.MachineInfrastructureReadyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineInfrastructureReadyV1Beta2Reason, }, }, { @@ -1250,60 +1308,6 @@ func TestSetReadyCondition(t *testing.T) { Message: "* Deleting: Machine deletion in progress, stage: WaitingForPreDrainHook", }, }, - { - name: "Drops messages from BootstrapConfigReady and Infrastructure ready when using fallback to fields", - machine: &clusterv1.Machine{ - ObjectMeta: metav1.ObjectMeta{ - Name: "machine-test", - Namespace: metav1.NamespaceDefault, - }, - Spec: clusterv1.MachineSpec{ - Bootstrap: clusterv1.Bootstrap{ - ConfigRef: &corev1.ObjectReference{ - Kind: "KubeadmConfig", - }, - }, - InfrastructureRef: corev1.ObjectReference{ - Kind: "AWSMachine", - }, - }, - Status: clusterv1.MachineStatus{ - InfrastructureReady: true, - BootstrapReady: true, - V1Beta2: &clusterv1.MachineV1Beta2Status{ - Conditions: []metav1.Condition{ - { - Type: clusterv1.MachineBootstrapConfigReadyV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: "Foo", - Message: bootstrapConfigReadyFallBackMessage("KubeadmConfig", true), - }, - { - Type: clusterv1.InfrastructureReadyV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: "Bar", - Message: infrastructureReadyFallBackMessage("AWSMachine", true), - }, - { - Type: clusterv1.MachineNodeHealthyV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: "AllGood", - }, - { - Type: clusterv1.MachineDeletingV1Beta2Condition, - Status: metav1.ConditionFalse, - Reason: clusterv1.MachineDeletingDeletionTimestampNotSetV1Beta2Reason, - }, - }, - }, - }, - }, - expectCondition: metav1.Condition{ - Type: clusterv1.MachineReadyV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: clusterv1.MachineReadyV1Beta2Reason, - }, - }, { name: "Aggregates Ready condition correctly while the machine is deleting (waiting for Node drain)", machine: &clusterv1.Machine{ diff --git a/util/conditions/v1beta2/mirror.go b/util/conditions/v1beta2/mirror.go index 4db71476f3a5..bbf5986fe079 100644 --- a/util/conditions/v1beta2/mirror.go +++ b/util/conditions/v1beta2/mirror.go @@ -108,16 +108,25 @@ func SetMirrorCondition(sourceObj Getter, targetObj Setter, sourceConditionType Set(targetObj, *mirrorCondition) } +// NewMirrorConditionFromUnstructured is a convenience method create a mirror of the given condition from the unstructured source obj. +// It combines, UnstructuredGet, NewMirrorCondition (most specifically it uses only the logic to +// create a mirror condition). +func NewMirrorConditionFromUnstructured(sourceObj runtime.Unstructured, sourceConditionType string, opts ...MirrorOption) (*metav1.Condition, error) { + condition, err := UnstructuredGet(sourceObj, sourceConditionType) + if err != nil { + return nil, err + } + return newMirrorCondition(condition, sourceConditionType, opts), nil +} + // SetMirrorConditionFromUnstructured is a convenience method that mirror of the given condition from the unstructured source obj -// into the target object. It combines, UnstructuredGet, NewMirrorCondition (most specifically it uses only the logic to -// create a mirror condition), and Set. +// into the target object. It combines, NewMirrorConditionFromUnstructured, and Set. func SetMirrorConditionFromUnstructured(sourceObj runtime.Unstructured, targetObj Setter, sourceConditionType string, opts ...MirrorOption) error { - condition, err := UnstructuredGet(sourceObj, sourceConditionType) + condition, err := NewMirrorConditionFromUnstructured(sourceObj, sourceConditionType, opts...) if err != nil { return err } - - Set(targetObj, *newMirrorCondition(condition, sourceConditionType, opts)) + Set(targetObj, *condition) return nil }