From 9535bff2c1a2a7b8fa65b63e3522f229458f7c76 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Tue, 22 Oct 2024 18:22:36 +0200 Subject: [PATCH 1/4] Implement grace period for KCP remote conditions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- api/v1beta1/machine_types.go | 14 +- api/v1beta1/v1beta2_condition_consts.go | 15 +- .../api/v1beta1/v1beta2_condition_consts.go | 20 +- controlplane/kubeadm/controllers/alias.go | 3 + .../kubeadm/internal/control_plane.go | 6 + .../internal/controllers/controller.go | 157 +++++- .../internal/controllers/controller_test.go | 464 ++++++++++++++++++ .../internal/controllers/fakes_test.go | 3 +- controlplane/kubeadm/main.go | 13 + .../controllers/machine/machine_controller.go | 4 +- .../machine/machine_controller_status.go | 196 +++----- .../machine/machine_controller_status_test.go | 161 +++++- internal/controllers/machine/suite_test.go | 43 -- .../topology/cluster/cluster_controller.go | 4 +- .../topology/cluster/suite_test.go | 7 +- main.go | 5 + util/collections/machine_filters.go | 2 +- 17 files changed, 887 insertions(+), 230 deletions(-) diff --git a/api/v1beta1/machine_types.go b/api/v1beta1/machine_types.go index 9b7c88fe1ba1..3f74d096f700 100644 --- a/api/v1beta1/machine_types.go +++ b/api/v1beta1/machine_types.go @@ -206,15 +206,11 @@ const ( // during the deletion workflow, or by a users. MachineNodeDeletedV1Beta2Reason = ObjectDeletedV1Beta2Reason - // MachineNodeRemoteConnectionFailedV1Beta2Reason surfaces that the remote connection failed. - // If the remote connection probe failed for longer than remote conditions grace period, - // this reason is used when setting NodeHealthy and NodeReady conditions to `Unknown`. - MachineNodeRemoteConnectionFailedV1Beta2Reason = RemoteConnectionFailedV1Beta2Reason - - // MachineNodeRemoteConnectionDownV1Beta2Reason surfaces that the remote connection is down. - // This is used when setting NodeHealthy and NodeReady conditions to `Unknown` - // when the connection is down and they haven't been set yet. - MachineNodeRemoteConnectionDownV1Beta2Reason = RemoteConnectionDownV1Beta2Reason + // MachineNodeInspectionFailedV1Beta2Reason documents a failure when inspecting the status of a Node. + MachineNodeInspectionFailedV1Beta2Reason = InspectionFailedV1Beta2Reason + + // MachineNodeConnectionDownV1Beta2Reason surfaces that the connection to the workload cluster is down. + MachineNodeConnectionDownV1Beta2Reason = ConnectionDownV1Beta2Reason ) // Machine's HealthCheckSucceeded condition and corresponding reasons that will be used in v1Beta2 API version. diff --git a/api/v1beta1/v1beta2_condition_consts.go b/api/v1beta1/v1beta2_condition_consts.go index bc8561c70f7a..e8785b72f668 100644 --- a/api/v1beta1/v1beta2_condition_consts.go +++ b/api/v1beta1/v1beta2_condition_consts.go @@ -106,8 +106,8 @@ const ( // set to false and with the OwnerRemediated condition set to false by the MachineHealthCheck controller. RemediatingV1Beta2Reason = "Remediating" - // NotRemediatingV1Beta2Reason surfaces when an object does not own any machines marked as not healthy - // by the MachineHealthCheck controller. + // NotRemediatingV1Beta2Reason surfaces when an object does not own any machines with HealthCheckSucceeded + // set to false and with the OwnerRemediated condition set to false by the MachineHealthCheck controller. NotRemediatingV1Beta2Reason = "NotRemediating" // NoReplicasV1Beta2Reason surfaces when an object that manage replicas does not have any. @@ -142,15 +142,8 @@ const ( // PausedV1Beta2Reason surfaces when an object is paused. PausedV1Beta2Reason = "Paused" - // RemoteConnectionFailedV1Beta2Reason surfaces that the remote connection failed. - // This is typically used when setting remote conditions (e.g. `NodeHealthy`) to `Unknown` - // after the remote connection probe didn't succeed for remote conditions grace period. - RemoteConnectionFailedV1Beta2Reason = "RemoteConnectionFailed" - - // RemoteConnectionDownV1Beta2Reason surfaces that the remote connection is down. - // This is typically used when setting remote conditions (e.g. `NodeHealthy`) to `Unknown` - // when the connection is down and they haven't been set yet. - RemoteConnectionDownV1Beta2Reason = "RemoteConnectionDown" + // ConnectionDownV1Beta2Reason surfaces that the connection to the workload cluster is down. + ConnectionDownV1Beta2Reason = "ConnectionDown" // DeletionTimestampNotSetV1Beta2Reason surfaces when an object is not deleting because the // DeletionTimestamp is not set. diff --git a/controlplane/kubeadm/api/v1beta1/v1beta2_condition_consts.go b/controlplane/kubeadm/api/v1beta1/v1beta2_condition_consts.go index 3b0db9bbc186..981f6f787eca 100644 --- a/controlplane/kubeadm/api/v1beta1/v1beta2_condition_consts.go +++ b/controlplane/kubeadm/api/v1beta1/v1beta2_condition_consts.go @@ -52,6 +52,10 @@ const ( // etcd cluster hosted on KubeadmControlPlane controlled machines. KubeadmControlPlaneEtcdClusterInspectionFailedV1Beta2Reason = clusterv1.InspectionFailedV1Beta2Reason + // KubeadmControlPlaneEtcdClusterConnectionDownV1Beta2Reason surfaces that the connection to the workload + // cluster is down. + KubeadmControlPlaneEtcdClusterConnectionDownV1Beta2Reason = clusterv1.ConnectionDownV1Beta2Reason + // KubeadmControlPlaneEtcdClusterHealthyV1Beta2Reason surfaces when the etcd cluster hosted on KubeadmControlPlane // machines is healthy. KubeadmControlPlaneEtcdClusterHealthyV1Beta2Reason = "Healthy" @@ -77,6 +81,10 @@ const ( // control plane components hosted on KubeadmControlPlane controlled machines. KubeadmControlPlaneControlPlaneComponentsInspectionFailedV1Beta2Reason = clusterv1.InspectionFailedV1Beta2Reason + // KubeadmControlPlaneControlPlaneComponentsConnectionDownV1Beta2Reason surfaces that the connection to the workload + // cluster is down. + KubeadmControlPlaneControlPlaneComponentsConnectionDownV1Beta2Reason = clusterv1.ConnectionDownV1Beta2Reason + // KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Reason surfaces when the Kubernetes control plane components // hosted on KubeadmControlPlane machines are healthy. KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Reason = "Healthy" @@ -233,13 +241,13 @@ const ( // pod hosted on a KubeadmControlPlane controlled machine. KubeadmControlPlaneMachinePodInspectionFailedV1Beta2Reason = clusterv1.InspectionFailedV1Beta2Reason + // KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason surfaces that the connection to the workload + // cluster is down. + KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason = clusterv1.ConnectionDownV1Beta2Reason + // KubeadmControlPlaneMachinePodDeletingV1Beta2Reason surfaces when the machine hosting control plane components // is being deleted. KubeadmControlPlaneMachinePodDeletingV1Beta2Reason = "Deleting" - - // KubeadmControlPlaneMachinePodInternalErrorV1Beta2Reason surfaces unexpected failures when reading pod hosted - // on a KubeadmControlPlane controlled machine. - KubeadmControlPlaneMachinePodInternalErrorV1Beta2Reason = clusterv1.InternalErrorV1Beta2Reason ) // EtcdMemberHealthy condition and corresponding reasons that will be used for KubeadmControlPlane controlled machines in v1Beta2 API version. @@ -257,6 +265,10 @@ const ( // etcd member hosted on a KubeadmControlPlane controlled machine. KubeadmControlPlaneMachineEtcdMemberInspectionFailedV1Beta2Reason = clusterv1.InspectionFailedV1Beta2Reason + // KubeadmControlPlaneMachineEtcdMemberConnectionDownV1Beta2Reason surfaces that the connection to the workload + // cluster is down. + KubeadmControlPlaneMachineEtcdMemberConnectionDownV1Beta2Reason = clusterv1.ConnectionDownV1Beta2Reason + // KubeadmControlPlaneMachineEtcdMemberDeletingV1Beta2Reason surfaces when the machine hosting an etcd member // is being deleted. KubeadmControlPlaneMachineEtcdMemberDeletingV1Beta2Reason = "Deleting" diff --git a/controlplane/kubeadm/controllers/alias.go b/controlplane/kubeadm/controllers/alias.go index b7a54214a399..5d2332b61dc0 100644 --- a/controlplane/kubeadm/controllers/alias.go +++ b/controlplane/kubeadm/controllers/alias.go @@ -40,6 +40,8 @@ type KubeadmControlPlaneReconciler struct { // WatchFilterValue is the label value used to filter events prior to reconciliation. WatchFilterValue string + RemoteConditionsGracePeriod time.Duration + // Deprecated: DeprecatedInfraMachineNaming. Name the InfraStructureMachines after the InfraMachineTemplate. DeprecatedInfraMachineNaming bool } @@ -53,6 +55,7 @@ func (r *KubeadmControlPlaneReconciler) SetupWithManager(ctx context.Context, mg EtcdDialTimeout: r.EtcdDialTimeout, EtcdCallTimeout: r.EtcdCallTimeout, WatchFilterValue: r.WatchFilterValue, + RemoteConditionsGracePeriod: r.RemoteConditionsGracePeriod, DeprecatedInfraMachineNaming: r.DeprecatedInfraMachineNaming, }).SetupWithManager(ctx, mgr, options) } diff --git a/controlplane/kubeadm/internal/control_plane.go b/controlplane/kubeadm/internal/control_plane.go index b87cd33ce67a..41b1ed8ffed0 100644 --- a/controlplane/kubeadm/internal/control_plane.go +++ b/controlplane/kubeadm/internal/control_plane.go @@ -309,6 +309,12 @@ func (c *ControlPlane) PatchMachines(ctx context.Context) error { controlplanev1.MachineSchedulerPodHealthyCondition, controlplanev1.MachineEtcdPodHealthyCondition, controlplanev1.MachineEtcdMemberHealthyCondition, + }}, patch.WithOwnedV1Beta2Conditions{Conditions: []string{ + controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + controlplanev1.KubeadmControlPlaneMachineControllerManagerPodHealthyV1Beta2Condition, + controlplanev1.KubeadmControlPlaneMachineSchedulerPodHealthyV1Beta2Condition, + controlplanev1.KubeadmControlPlaneMachineEtcdPodHealthyV1Beta2Condition, + controlplanev1.KubeadmControlPlaneMachineEtcdMemberHealthyV1Beta2Condition, }}); err != nil { errList = append(errList, err) } diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/controlplane/kubeadm/internal/controllers/controller.go index 37dfca21f086..03fad8444c22 100644 --- a/controlplane/kubeadm/internal/controllers/controller.go +++ b/controlplane/kubeadm/internal/controllers/controller.go @@ -86,6 +86,8 @@ type KubeadmControlPlaneReconciler struct { // WatchFilterValue is the label value used to filter events prior to reconciliation. WatchFilterValue string + RemoteConditionsGracePeriod time.Duration + // Deprecated: DeprecatedInfraMachineNaming. Name the InfraStructureMachines after the InfraMachineTemplate. DeprecatedInfraMachineNaming bool @@ -95,6 +97,14 @@ type KubeadmControlPlaneReconciler struct { } func (r *KubeadmControlPlaneReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { + if r.Client == nil || r.SecretCachingClient == nil || r.ClusterCache == nil || + r.EtcdDialTimeout == time.Duration(0) || r.EtcdCallTimeout == time.Duration(0) || + r.RemoteConditionsGracePeriod < 2*time.Minute { + return errors.New("Client, SecretCachingClient and ClusterCache must not be nil and " + + "EtcdDialTimeout and EtcdCallTimeout must not be 0 and " + + "RemoteConditionsGracePeriod must not be < 2m") + } + predicateLog := ctrl.LoggerFrom(ctx).WithValues("controller", "kubeadmcontrolplane") c, err := ctrl.NewControllerManagedBy(mgr). For(&controlplanev1.KubeadmControlPlane{}). @@ -111,7 +121,8 @@ func (r *KubeadmControlPlaneReconciler) SetupWithManager(ctx context.Context, mg ), ), ). - WatchesRawSource(r.ClusterCache.GetClusterSource("kubeadmcontrolplane", r.ClusterToKubeadmControlPlane)). + WatchesRawSource(r.ClusterCache.GetClusterSource("kubeadmcontrolplane", r.ClusterToKubeadmControlPlane, + clustercache.WatchForProbeFailure(r.RemoteConditionsGracePeriod))). Build(r) if err != nil { return errors.Wrap(err, "failed setting up with a controller manager") @@ -802,45 +813,169 @@ func (r *KubeadmControlPlaneReconciler) syncMachines(ctx context.Context, contro // reconcileControlPlaneConditions is responsible of reconciling conditions reporting the status of static pods and // the status of the etcd cluster. -func (r *KubeadmControlPlaneReconciler) reconcileControlPlaneConditions(ctx context.Context, controlPlane *internal.ControlPlane) error { +func (r *KubeadmControlPlaneReconciler) reconcileControlPlaneConditions(ctx context.Context, controlPlane *internal.ControlPlane) (reterr error) { // If the cluster is not yet initialized, there is no way to connect to the workload cluster and fetch information // for updating conditions. Return early. - if !controlPlane.KCP.Status.Initialized { + // We additionally check for the Available condition. The Available condition is set at the same time + // as .status.initialized and is never changed to false again. Below we'll need the transition time of the + // Available condition to check if the remote conditions grace period is already reached. + // Note: The Machine controller uses the ControlPlaneInitialized condition on the Cluster instead for + // the same check. We don't use the ControlPlaneInitialized condition from the Cluster here because KCP + // Reconcile does (currently) not get triggered from condition changes to the Cluster object. + controlPlaneInitialized := conditions.Get(controlPlane.KCP, controlplanev1.AvailableCondition) + if !controlPlane.KCP.Status.Initialized || + controlPlaneInitialized == nil || controlPlaneInitialized.Status != corev1.ConditionTrue { v1beta2conditions.Set(controlPlane.KCP, metav1.Condition{ Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, Status: metav1.ConditionUnknown, Reason: controlplanev1.KubeadmControlPlaneEtcdClusterInspectionFailedV1Beta2Reason, - Message: "Waiting for remote connection", + Message: "Waiting for Cluster control plane to be initialized", }) v1beta2conditions.Set(controlPlane.KCP, metav1.Condition{ Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, Status: metav1.ConditionUnknown, Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsInspectionFailedV1Beta2Reason, - Message: "Waiting for remote connection", + Message: "Waiting for Cluster control plane to be initialized", }) return nil } + defer func() { + // Patch machines with the updated conditions. + reterr = kerrors.NewAggregate([]error{reterr, controlPlane.PatchMachines(ctx)}) + }() + + // Remote conditions grace period is counted from the later of last probe success and control plane initialized. + lastProbeSuccessTime := r.ClusterCache.GetLastProbeSuccessTimestamp(ctx, client.ObjectKeyFromObject(controlPlane.Cluster)) + if time.Since(maxTime(lastProbeSuccessTime, controlPlaneInitialized.LastTransitionTime.Time)) > r.RemoteConditionsGracePeriod { + // Overwrite conditions to ConnectionDown. + setConditionsToUnknown(setConditionsToUnknownInput{ + ControlPlane: controlPlane, + Overwrite: true, + EtcdClusterHealthyReason: controlplanev1.KubeadmControlPlaneEtcdClusterConnectionDownV1Beta2Reason, + ControlPlaneComponentsHealthyReason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsConnectionDownV1Beta2Reason, + StaticPodReason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + EtcdMemberHealthyReason: controlplanev1.KubeadmControlPlaneMachineEtcdMemberConnectionDownV1Beta2Reason, + Message: lastProbeSuccessMessage(lastProbeSuccessTime), + }) + return errors.Errorf("connection to the workload cluster is down") + } + workloadCluster, err := controlPlane.GetWorkloadCluster(ctx) if err != nil { - return errors.Wrap(err, "cannot get remote client to workload cluster") + if errors.Is(err, clustercache.ErrClusterNotConnected) { + // If conditions are not set, set them to ConnectionDown. + setConditionsToUnknown(setConditionsToUnknownInput{ + ControlPlane: controlPlane, + Overwrite: false, // Don't overwrite. + EtcdClusterHealthyReason: controlplanev1.KubeadmControlPlaneEtcdClusterConnectionDownV1Beta2Reason, + ControlPlaneComponentsHealthyReason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsConnectionDownV1Beta2Reason, + StaticPodReason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + EtcdMemberHealthyReason: controlplanev1.KubeadmControlPlaneMachineEtcdMemberConnectionDownV1Beta2Reason, + Message: lastProbeSuccessMessage(lastProbeSuccessTime), + }) + return errors.Wrap(err, "cannot get client for the workload cluster") + } + + // Overwrite conditions to InspectionFailed. + setConditionsToUnknown(setConditionsToUnknownInput{ + ControlPlane: controlPlane, + Overwrite: true, + EtcdClusterHealthyReason: controlplanev1.KubeadmControlPlaneEtcdClusterInspectionFailedV1Beta2Reason, + ControlPlaneComponentsHealthyReason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsInspectionFailedV1Beta2Reason, + StaticPodReason: controlplanev1.KubeadmControlPlaneMachinePodInspectionFailedV1Beta2Reason, + EtcdMemberHealthyReason: controlplanev1.KubeadmControlPlaneMachineEtcdMemberInspectionFailedV1Beta2Reason, + Message: "Please check controller logs for errors", + }) + return errors.Wrap(err, "cannot get client for the workload cluster") } // Update conditions status workloadCluster.UpdateStaticPodConditions(ctx, controlPlane) workloadCluster.UpdateEtcdConditions(ctx, controlPlane) - // Patch machines with the updated conditions. - if err := controlPlane.PatchMachines(ctx); err != nil { - return err - } - // KCP will be patched at the end of Reconcile to reflect updated conditions, so we can return now. return nil } +type setConditionsToUnknownInput struct { + ControlPlane *internal.ControlPlane + Overwrite bool + EtcdClusterHealthyReason string + ControlPlaneComponentsHealthyReason string + StaticPodReason string + EtcdMemberHealthyReason string + Message string +} + +func setConditionsToUnknown(input setConditionsToUnknownInput) { + etcdClusterHealthySet := v1beta2conditions.Has(input.ControlPlane.KCP, controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition) + controlPlaneComponentsHealthySet := v1beta2conditions.Has(input.ControlPlane.KCP, controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition) + + if input.Overwrite || !etcdClusterHealthySet { + v1beta2conditions.Set(input.ControlPlane.KCP, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: input.EtcdClusterHealthyReason, + Message: input.Message, + }) + for _, machine := range input.ControlPlane.Machines { + if input.ControlPlane.IsEtcdManaged() { + v1beta2conditions.Set(machine, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneMachineEtcdMemberHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: input.EtcdMemberHealthyReason, + Message: input.Message, + }) + } + } + } + + if input.Overwrite || !controlPlaneComponentsHealthySet { + v1beta2conditions.Set(input.ControlPlane.KCP, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: input.ControlPlaneComponentsHealthyReason, + Message: input.Message, + }) + + allMachinePodV1beta2Conditions := []string{ + controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + controlplanev1.KubeadmControlPlaneMachineControllerManagerPodHealthyV1Beta2Condition, + controlplanev1.KubeadmControlPlaneMachineSchedulerPodHealthyV1Beta2Condition, + } + if input.ControlPlane.IsEtcdManaged() { + allMachinePodV1beta2Conditions = append(allMachinePodV1beta2Conditions, controlplanev1.KubeadmControlPlaneMachineEtcdPodHealthyV1Beta2Condition) + } + for _, machine := range input.ControlPlane.Machines { + for _, condition := range allMachinePodV1beta2Conditions { + v1beta2conditions.Set(machine, metav1.Condition{ + Type: condition, + Status: metav1.ConditionUnknown, + Reason: input.StaticPodReason, + Message: input.Message, + }) + } + } + } +} + +func lastProbeSuccessMessage(lastProbeSuccessTime time.Time) string { + if lastProbeSuccessTime.IsZero() { + return "" + } + return fmt.Sprintf("Last successful probe at %s", lastProbeSuccessTime.Format(time.RFC3339)) +} + +func maxTime(t1, t2 time.Time) time.Time { + if t1.After(t2) { + return t1 + } + return t2 +} + // reconcileEtcdMembers ensures the number of etcd members is in sync with the number of machines/nodes. // This is usually required after a machine deletion. // diff --git a/controlplane/kubeadm/internal/controllers/controller_test.go b/controlplane/kubeadm/internal/controllers/controller_test.go index ff79e501ac45..82fa2e66f5e7 100644 --- a/controlplane/kubeadm/internal/controllers/controller_test.go +++ b/controlplane/kubeadm/internal/controllers/controller_test.go @@ -30,6 +30,7 @@ import ( "github.com/blang/semver/v4" . "github.com/onsi/gomega" + "github.com/pkg/errors" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -45,6 +46,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1" + "sigs.k8s.io/cluster-api/controllers/clustercache" "sigs.k8s.io/cluster-api/controllers/external" controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" "sigs.k8s.io/cluster-api/controlplane/kubeadm/internal" @@ -59,6 +61,7 @@ import ( "sigs.k8s.io/cluster-api/util/certs" "sigs.k8s.io/cluster-api/util/collections" "sigs.k8s.io/cluster-api/util/conditions" + v1beta2conditions "sigs.k8s.io/cluster-api/util/conditions/v1beta2" "sigs.k8s.io/cluster-api/util/kubeconfig" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/secret" @@ -1836,6 +1839,467 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) { g.Expect(updatedDeletingMachine.Spec).Should(BeComparableTo(deletingMachine.Spec)) } +func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testing.T) { + now := time.Now() + + defaultMachine1 := clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine1-test", + Namespace: metav1.NamespaceDefault, + }, + } + defaultMachine2 := clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine2-test", + Namespace: metav1.NamespaceDefault, + }, + } + + defaultCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-test", + Namespace: metav1.NamespaceDefault, + }, + } + + defaultKCP := &controlplanev1.KubeadmControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "kcp-test", + Namespace: metav1.NamespaceDefault, + }, + Status: controlplanev1.KubeadmControlPlaneStatus{ + Initialized: true, + Conditions: clusterv1.Conditions{ + {Type: controlplanev1.AvailableCondition, Status: corev1.ConditionTrue, + LastTransitionTime: metav1.Time{Time: now.Add(-5 * time.Second)}}, + }, + }, + } + + testCases := []struct { + name string + controlPlane *internal.ControlPlane + managementCluster internal.ManagementCluster + lastProbeSuccessTime time.Time + expectErr string + expectKCPConditions []metav1.Condition + expectMachineConditions []metav1.Condition + }{ + { + name: "Cluster control plane is not initialized", + controlPlane: &internal.ControlPlane{ + KCP: func() *controlplanev1.KubeadmControlPlane { + kcp := defaultKCP.DeepCopy() + kcp.Status.Initialized = false + conditions.MarkFalse(kcp, controlplanev1.AvailableCondition, "", clusterv1.ConditionSeverityError, "") + return kcp + }(), + }, + expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneEtcdClusterInspectionFailedV1Beta2Reason, + Message: "Waiting for Cluster control plane to be initialized", + }, + { + Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsInspectionFailedV1Beta2Reason, + Message: "Waiting for Cluster control plane to be initialized", + }, + }, + }, + { + name: "connection down, preserve conditions as they have been set before (remote conditions grace period not passed yet)", + controlPlane: &internal.ControlPlane{ + Cluster: defaultCluster, + KCP: func() *controlplanev1.KubeadmControlPlane { + kcp := defaultKCP.DeepCopy() + for i, condition := range kcp.Status.Conditions { + if condition.Type == controlplanev1.AvailableCondition { + kcp.Status.Conditions[i].LastTransitionTime.Time = now.Add(-4 * time.Minute) + } + } + v1beta2conditions.Set(kcp, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Reason, + }) + v1beta2conditions.Set(kcp, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Reason, + }) + return kcp + }(), + Machines: map[string]*clusterv1.Machine{ + defaultMachine1.Name: func() *clusterv1.Machine { + m := defaultMachine1.DeepCopy() + v1beta2conditions.Set(m, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneMachinePodRunningV1Beta2Reason, + }) + return m + }(), + defaultMachine2.Name: func() *clusterv1.Machine { + m := defaultMachine2.DeepCopy() + v1beta2conditions.Set(m, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneMachinePodRunningV1Beta2Reason, + }) + return m + }(), + }, + }, + managementCluster: &fakeManagementCluster{ + WorkloadErr: errors.Wrapf(clustercache.ErrClusterNotConnected, "error getting REST config"), + }, + lastProbeSuccessTime: now.Add(-3 * time.Minute), + // Conditions have not been updated. + // remoteConditionsGracePeriod is 5m + // control plane is initialized since 4m ago, last probe success was 3m ago. + expectErr: "cannot get client for the workload cluster: error getting REST config: " + + "connection to the workload cluster is down", + expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Reason, + }, + { + Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Reason, + }, + }, + expectMachineConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneMachinePodRunningV1Beta2Reason, + }, + }, + }, + { + name: "connection down, set conditions as they haven't been set before (remote conditions grace period not passed yet)", + controlPlane: &internal.ControlPlane{ + Cluster: defaultCluster, + KCP: func() *controlplanev1.KubeadmControlPlane { + kcp := defaultKCP.DeepCopy() + for i, condition := range kcp.Status.Conditions { + if condition.Type == controlplanev1.AvailableCondition { + kcp.Status.Conditions[i].LastTransitionTime.Time = now.Add(-4 * time.Minute) + } + } + return kcp + }(), + Machines: map[string]*clusterv1.Machine{ + defaultMachine1.Name: defaultMachine1.DeepCopy(), + defaultMachine2.Name: defaultMachine2.DeepCopy(), + }, + }, + managementCluster: &fakeManagementCluster{ + WorkloadErr: errors.Wrapf(clustercache.ErrClusterNotConnected, "error getting REST config"), + }, + lastProbeSuccessTime: now.Add(-3 * time.Minute), + // Conditions have been set. + // remoteConditionsGracePeriod is 5m + // control plane is initialized since 4m ago, last probe success was 3m ago. + expectErr: "cannot get client for the workload cluster: error getting REST config: " + + "connection to the workload cluster is down", + expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneEtcdClusterConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-3*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-3*time.Minute).Format(time.RFC3339)), + }, + }, + expectMachineConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-3*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineControllerManagerPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-3*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineSchedulerPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-3*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineEtcdPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-3*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineEtcdMemberHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachineEtcdMemberConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-3*time.Minute).Format(time.RFC3339)), + }, + }, + }, + { + name: "connection down, set conditions to unknown (remote conditions grace period passed)", + controlPlane: &internal.ControlPlane{ + Cluster: defaultCluster, + KCP: func() *controlplanev1.KubeadmControlPlane { + kcp := defaultKCP.DeepCopy() + for i, condition := range kcp.Status.Conditions { + if condition.Type == controlplanev1.AvailableCondition { + kcp.Status.Conditions[i].LastTransitionTime.Time = now.Add(-7 * time.Minute) + } + } + v1beta2conditions.Set(kcp, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Reason, + }) + v1beta2conditions.Set(kcp, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Reason, + }) + return kcp + }(), + Machines: map[string]*clusterv1.Machine{ + defaultMachine1.Name: func() *clusterv1.Machine { + m := defaultMachine1.DeepCopy() + v1beta2conditions.Set(m, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneMachinePodRunningV1Beta2Reason, + }) + return m + }(), + defaultMachine2.Name: func() *clusterv1.Machine { + m := defaultMachine2.DeepCopy() + v1beta2conditions.Set(m, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneMachinePodRunningV1Beta2Reason, + }) + return m + }(), + }, + }, + lastProbeSuccessTime: now.Add(-6 * time.Minute), + // Conditions have been updated. + // remoteConditionsGracePeriod is 5m + // control plane is initialized since 7m ago, last probe success was 6m ago. + expectErr: "connection to the workload cluster is down", + expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneEtcdClusterConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-6*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-6*time.Minute).Format(time.RFC3339)), + }, + }, + expectMachineConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-6*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineControllerManagerPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-6*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineSchedulerPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-6*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineEtcdPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-6*time.Minute).Format(time.RFC3339)), + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineEtcdMemberHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachineEtcdMemberConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-6*time.Minute).Format(time.RFC3339)), + }, + }, + }, + { + name: "internal error occurred when trying to get workload cluster (InspectionFailed)", + controlPlane: &internal.ControlPlane{ + Cluster: defaultCluster, + KCP: defaultKCP, + Machines: map[string]*clusterv1.Machine{ + defaultMachine1.Name: defaultMachine1.DeepCopy(), + defaultMachine2.Name: defaultMachine2.DeepCopy(), + }, + }, + managementCluster: &fakeManagementCluster{ + WorkloadErr: errors.Errorf("failed to get secret; etcd CA bundle"), + }, + lastProbeSuccessTime: now.Add(-3 * time.Minute), + expectErr: "cannot get client for the workload cluster: failed to get secret; etcd CA bundle", + expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneEtcdClusterInspectionFailedV1Beta2Reason, + Message: "Please check controller logs for errors", + }, + { + Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsInspectionFailedV1Beta2Reason, + Message: "Please check controller logs for errors", + }, + }, + expectMachineConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodInspectionFailedV1Beta2Reason, + Message: "Please check controller logs for errors", + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineControllerManagerPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodInspectionFailedV1Beta2Reason, + Message: "Please check controller logs for errors", + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineSchedulerPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodInspectionFailedV1Beta2Reason, + Message: "Please check controller logs for errors", + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineEtcdPodHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachinePodInspectionFailedV1Beta2Reason, + Message: "Please check controller logs for errors", + }, + { + Type: controlplanev1.KubeadmControlPlaneMachineEtcdMemberHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneMachineEtcdMemberInspectionFailedV1Beta2Reason, + Message: "Please check controller logs for errors", + }, + }, + }, + { + name: "successfully got workload cluster (without Machines)", + controlPlane: &internal.ControlPlane{ + Cluster: defaultCluster, + KCP: defaultKCP, + }, + managementCluster: &fakeManagementCluster{ + Workload: &fakeWorkloadCluster{ + Workload: &internal.Workload{ + Client: fake.NewClientBuilder().Build(), + }, + }, + }, + lastProbeSuccessTime: now.Add(-3 * time.Minute), + expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneEtcdClusterHealthUnknownV1Beta2Reason, + Message: "No Machines reporting etcd member status", + }, + { + Type: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthUnknownV1Beta2Reason, + Message: "No Machines reporting control plane status", + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + r := &KubeadmControlPlaneReconciler{ + ClusterCache: &fakeClusterCache{ + lastProbeSuccessTime: tc.lastProbeSuccessTime, + }, + RemoteConditionsGracePeriod: 5 * time.Minute, + } + + if tc.managementCluster != nil { + tc.controlPlane.InjectTestManagementCluster(tc.managementCluster) + } + + var objs []client.Object + for _, machine := range tc.controlPlane.Machines { + objs = append(objs, machine) + } + fakeClient := fake.NewClientBuilder().WithObjects(objs...).WithStatusSubresource(&clusterv1.Machine{}).Build() + + patchHelpers := map[string]*patch.Helper{} + for _, machine := range tc.controlPlane.Machines { + helper, err := patch.NewHelper(machine, fakeClient) + g.Expect(err).ToNot(HaveOccurred()) + patchHelpers[machine.Name] = helper + } + tc.controlPlane.SetPatchHelpers(patchHelpers) + + err := r.reconcileControlPlaneConditions(ctx, tc.controlPlane) + if tc.expectErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal(tc.expectErr)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + + g.Expect(tc.controlPlane.KCP.GetV1Beta2Conditions()).To(v1beta2conditions.MatchConditions(tc.expectKCPConditions, v1beta2conditions.IgnoreLastTransitionTime(true))) + for _, machine := range tc.controlPlane.Machines { + g.Expect(machine.GetV1Beta2Conditions()).To(v1beta2conditions.MatchConditions(tc.expectMachineConditions, v1beta2conditions.IgnoreLastTransitionTime(true))) + } + }) + } +} + +type fakeClusterCache struct { + clustercache.ClusterCache + lastProbeSuccessTime time.Time +} + +func (cc *fakeClusterCache) GetLastProbeSuccessTimestamp(_ context.Context, _ client.ObjectKey) time.Time { + return cc.lastProbeSuccessTime +} + func TestKubeadmControlPlaneReconciler_reconcilePreTerminateHook(t *testing.T) { cluster := &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{ diff --git a/controlplane/kubeadm/internal/controllers/fakes_test.go b/controlplane/kubeadm/internal/controllers/fakes_test.go index ee27e7d688de..a88b30c7dfae 100644 --- a/controlplane/kubeadm/internal/controllers/fakes_test.go +++ b/controlplane/kubeadm/internal/controllers/fakes_test.go @@ -37,6 +37,7 @@ type fakeManagementCluster struct { Machines collections.Machines MachinePools *expv1.MachinePoolList Workload *fakeWorkloadCluster + WorkloadErr error Reader client.Reader } @@ -49,7 +50,7 @@ func (f *fakeManagementCluster) List(ctx context.Context, list client.ObjectList } func (f *fakeManagementCluster) GetWorkloadCluster(_ context.Context, _ client.ObjectKey) (internal.WorkloadCluster, error) { - return f.Workload, nil + return f.Workload, f.WorkloadErr } func (f *fakeManagementCluster) GetMachinesForCluster(c context.Context, cluster *clusterv1.Cluster, filters ...collections.Func) (collections.Machines, error) { diff --git a/controlplane/kubeadm/main.go b/controlplane/kubeadm/main.go index ccc2f18a98b6..5fc6bc911687 100644 --- a/controlplane/kubeadm/main.go +++ b/controlplane/kubeadm/main.go @@ -25,6 +25,7 @@ import ( goruntime "runtime" "time" + "github.com/pkg/errors" "github.com/spf13/pflag" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -89,6 +90,7 @@ var ( managerOptions = flags.ManagerOptions{} logOptions = logs.NewOptions() // KCP specific flags. + remoteConditionsGracePeriod time.Duration kubeadmControlPlaneConcurrency int clusterCacheConcurrency int etcdDialTimeout time.Duration @@ -135,6 +137,10 @@ func InitFlags(fs *pflag.FlagSet) { fs.BoolVar(&enableContentionProfiling, "contention-profiling", false, "Enable block profiling") + fs.DurationVar(&remoteConditionsGracePeriod, "remote-conditions-grace-period", 5*time.Minute, + "Grace period after which remote conditions (e.g. `APIServerPodHealthy`) are set to `Unknown`, "+ + "the grace period starts from the last successful health probe to the workload cluster") + fs.IntVar(&kubeadmControlPlaneConcurrency, "kubeadmcontrolplane-concurrency", 10, "Number of kubeadm control planes to process simultaneously") @@ -215,6 +221,12 @@ func main() { restConfig.UserAgent = remote.DefaultClusterAPIUserAgent(controllerName) restConfig.WarningHandler = apiwarnings.DefaultHandler(klog.Background().WithName("API Server Warning")) + if remoteConditionsGracePeriod < 2*time.Minute { + // A minimum of 2m is enforced to ensure the ClusterCache always drops the connection before the grace period is reached. + setupLog.Error(errors.Errorf("--remote-conditions-grace-period must be at least 2m"), "Unable to start manager") + os.Exit(1) + } + tlsOptions, metricsOptions, err := flags.GetManagerOptions(managerOptions) if err != nil { setupLog.Error(err, "Unable to start manager: invalid flags") @@ -357,6 +369,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) { WatchFilterValue: watchFilterValue, EtcdDialTimeout: etcdDialTimeout, EtcdCallTimeout: etcdCallTimeout, + RemoteConditionsGracePeriod: remoteConditionsGracePeriod, DeprecatedInfraMachineNaming: useDeprecatedInfraMachineNaming, }).SetupWithManager(ctx, mgr, concurrency(kubeadmControlPlaneConcurrency)); err != nil { setupLog.Error(err, "unable to create controller", "controller", "KubeadmControlPlane") diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index 0d03ad53b4bc..3353eeb2f9d9 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -112,8 +112,8 @@ type Reconciler struct { } func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { - if r.Client == nil || r.APIReader == nil || r.ClusterCache == nil || r.RemoteConditionsGracePeriod == time.Duration(0) { - return errors.New("Client, APIReader and ClusterCache must not be nil and RemoteConditionsGracePeriod must not be 0") + if r.Client == nil || r.APIReader == nil || r.ClusterCache == nil || r.RemoteConditionsGracePeriod < 2*time.Minute { + return errors.New("Client, APIReader and ClusterCache must not be nil and RemoteConditionsGracePeriod must not be < 2m") } predicateLog := ctrl.LoggerFrom(ctx).WithValues("controller", "machine") diff --git a/internal/controllers/machine/machine_controller_status.go b/internal/controllers/machine/machine_controller_status.go index d063402584bd..c6f844b00738 100644 --- a/internal/controllers/machine/machine_controller_status.go +++ b/internal/controllers/machine/machine_controller_status.go @@ -34,6 +34,7 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/controllers/clustercache" "sigs.k8s.io/cluster-api/internal/contract" + "sigs.k8s.io/cluster-api/util/conditions" v1beta2conditions "sigs.k8s.io/cluster-api/util/conditions/v1beta2" ) @@ -58,7 +59,7 @@ func (r *Reconciler) reconcileStatus(ctx context.Context, s *scope) { // Note: some of the status fields are managed in reconcileNode, e.g. status.NodeRef, etc. // here we are taking care only of the delta (condition). lastProbeSuccessTime := r.ClusterCache.GetLastProbeSuccessTimestamp(ctx, client.ObjectKeyFromObject(s.cluster)) - setNodeHealthyAndReadyConditions(ctx, s.machine, s.node, s.nodeGetError, lastProbeSuccessTime, r.RemoteConditionsGracePeriod) + setNodeHealthyAndReadyConditions(ctx, s.cluster, s.machine, s.node, s.nodeGetError, lastProbeSuccessTime, r.RemoteConditionsGracePeriod) // Updates Machine status not observed from Bootstrap Config, InfraMachine or Node (update Machine's own status). // Note: some of the status are set in reconcileCertificateExpiry (e.g.status.CertificatesExpiryDate), @@ -228,74 +229,48 @@ func infrastructureReadyFallBackMessage(kind string, ready bool) string { return fmt.Sprintf("%s status.ready is %t", kind, ready) } -func setNodeHealthyAndReadyConditions(ctx context.Context, machine *clusterv1.Machine, node *corev1.Node, nodeGetErr error, lastProbeSuccessTime time.Time, remoteConditionsGracePeriod time.Duration) { - if time.Since(lastProbeSuccessTime) > remoteConditionsGracePeriod { - var msg string - if lastProbeSuccessTime.IsZero() { - msg = "Remote connection down" - } else { - msg = fmt.Sprintf("Remote connection down since %s", lastProbeSuccessTime.Format(time.RFC3339)) - } - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeReadyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeRemoteConnectionFailedV1Beta2Reason, - Message: msg, - }) +func setNodeHealthyAndReadyConditions(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine, node *corev1.Node, nodeGetErr error, lastProbeSuccessTime time.Time, remoteConditionsGracePeriod time.Duration) { + if !cluster.Status.InfrastructureReady { + setNodeConditions(machine, metav1.ConditionUnknown, + clusterv1.MachineNodeInspectionFailedV1Beta2Reason, + "Waiting for Cluster status.infrastructureReady to be true") + return + } - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeHealthyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeRemoteConnectionFailedV1Beta2Reason, - Message: msg, - }) + controlPlaneInitialized := conditions.Get(cluster, clusterv1.ControlPlaneInitializedCondition) + if controlPlaneInitialized == nil || controlPlaneInitialized.Status != corev1.ConditionTrue { + setNodeConditions(machine, metav1.ConditionUnknown, + clusterv1.MachineNodeInspectionFailedV1Beta2Reason, + "Waiting for Cluster control plane to be initialized") + return + } + + // Remote conditions grace period is counted from the later of last probe success and control plane initialized. + if time.Since(maxTime(lastProbeSuccessTime, controlPlaneInitialized.LastTransitionTime.Time)) > remoteConditionsGracePeriod { + // Overwrite conditions to ConnectionDown. + setNodeConditions(machine, metav1.ConditionUnknown, + clusterv1.MachineNodeConnectionDownV1Beta2Reason, + lastProbeSuccessMessage(lastProbeSuccessTime)) return } if nodeGetErr != nil { if errors.Is(nodeGetErr, clustercache.ErrClusterNotConnected) { - // If the connection to the workload cluster is down, only set the conditions if they - // are not set, but don't update them. - // This can happen either when the cluster comes up initially or when the cluster - // becomes unreachable later. - // If the cluster becomes unreachable later, the conditions will be set to `Unknown` - // only after remote conditions grace period. - if !v1beta2conditions.Has(machine, clusterv1.MachineNodeReadyV1Beta2Condition) { - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeReadyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeRemoteConnectionDownV1Beta2Reason, - Message: "Cannot determine Node state, connection to the cluster is down", - }) + // If conditions are not set, set them to ConnectionDown. + if !v1beta2conditions.Has(machine, clusterv1.MachineNodeReadyV1Beta2Condition) || + !v1beta2conditions.Has(machine, clusterv1.MachineNodeHealthyV1Beta2Condition) { + setNodeConditions(machine, metav1.ConditionUnknown, + clusterv1.MachineNodeConnectionDownV1Beta2Reason, + lastProbeSuccessMessage(lastProbeSuccessTime)) } - - if !v1beta2conditions.Has(machine, clusterv1.MachineNodeHealthyV1Beta2Condition) { - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeHealthyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeRemoteConnectionDownV1Beta2Reason, - Message: "Cannot determine Node state, connection to the cluster is down", - }) - } - return } - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeReadyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeInternalErrorV1Beta2Reason, - Message: "Please check controller logs for errors", - // NOTE: the error is logged by reconcileNode. - }) - - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeHealthyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeInternalErrorV1Beta2Reason, - Message: "Please check controller logs for errors", - // NOTE: the error is logged by reconcileNode. - }) + // Overwrite conditions to InternalError. + setNodeConditions(machine, metav1.ConditionUnknown, + clusterv1.MachineNodeInternalErrorV1Beta2Reason, + "Please check controller logs for errors") + // NOTE: the error is logged by reconcileNode. return } @@ -348,89 +323,66 @@ func setNodeHealthyAndReadyConditions(ctx context.Context, machine *clusterv1.Ma // will be considered unreachable Machine deletion will complete. if !machine.DeletionTimestamp.IsZero() { if machine.Status.NodeRef != nil { - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeReadyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeDeletedV1Beta2Reason, - Message: fmt.Sprintf("Node %s has been deleted", machine.Status.NodeRef.Name), - }) - - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeHealthyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeDeletedV1Beta2Reason, - Message: fmt.Sprintf("Node %s has been deleted", machine.Status.NodeRef.Name), - }) + setNodeConditions(machine, metav1.ConditionUnknown, + clusterv1.MachineNodeDeletedV1Beta2Reason, + fmt.Sprintf("Node %s has been deleted", machine.Status.NodeRef.Name)) return } - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeReadyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeDoesNotExistV1Beta2Reason, - Message: "Node does not exist", - }) - - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeHealthyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeDoesNotExistV1Beta2Reason, - Message: "Node does not exist", - }) + setNodeConditions(machine, metav1.ConditionUnknown, + clusterv1.MachineNodeDoesNotExistV1Beta2Reason, + "Node does not exist") return } // Report an issue if node missing after being initialized. if machine.Status.NodeRef != nil { - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeReadyV1Beta2Condition, - Status: metav1.ConditionFalse, // setting to false to keep it consistent with node below. - Reason: clusterv1.MachineNodeDeletedV1Beta2Reason, - Message: fmt.Sprintf("Node %s has been deleted while the machine still exists", machine.Status.NodeRef.Name), - }) - - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeHealthyV1Beta2Condition, - Status: metav1.ConditionFalse, // setting to false to give more relevance in the ready condition summary. - Reason: clusterv1.MachineNodeDeletedV1Beta2Reason, - Message: fmt.Sprintf("Node %s has been deleted while the machine still exists", machine.Status.NodeRef.Name), - }) + // Setting MachineNodeHealthyV1Beta2Condition to False to give it more relevance in the Ready condition summary. + // Setting MachineNodeReadyV1Beta2Condition to False to keep it consistent with MachineNodeHealthyV1Beta2Condition. + setNodeConditions(machine, metav1.ConditionFalse, + clusterv1.MachineNodeDeletedV1Beta2Reason, + fmt.Sprintf("Node %s has been deleted while the machine still exists", machine.Status.NodeRef.Name)) return } // If the machine is at the end of the provisioning phase, with ProviderID set, but still waiting // for a matching Node to exists, surface this. if ptr.Deref(machine.Spec.ProviderID, "") != "" { - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeReadyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeDoesNotExistV1Beta2Reason, - Message: fmt.Sprintf("Waiting for a Node with spec.providerID %s to exist", *machine.Spec.ProviderID), - }) + setNodeConditions(machine, metav1.ConditionUnknown, + clusterv1.MachineNodeDoesNotExistV1Beta2Reason, + fmt.Sprintf("Waiting for a Node with spec.providerID %s to exist", *machine.Spec.ProviderID)) + return + } + // If the machine is at the beginning of the provisioning phase, with ProviderID not yet set, surface this. + setNodeConditions(machine, metav1.ConditionUnknown, + clusterv1.MachineNodeDoesNotExistV1Beta2Reason, + fmt.Sprintf("Waiting for %s to report spec.providerID", machine.Spec.InfrastructureRef.Kind)) +} + +func setNodeConditions(machine *clusterv1.Machine, status metav1.ConditionStatus, reason, msg string) { + for _, conditionType := range []string{clusterv1.MachineNodeReadyV1Beta2Condition, clusterv1.MachineNodeHealthyV1Beta2Condition} { v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeHealthyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeDoesNotExistV1Beta2Reason, - Message: fmt.Sprintf("Waiting for a Node with spec.providerID %s to exist", *machine.Spec.ProviderID), + Type: conditionType, + Status: status, + Reason: reason, + Message: msg, }) - return } +} - // If the machine is at the beginning of the provisioning phase, with ProviderID not yet set, surface this. - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeReadyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeDoesNotExistV1Beta2Reason, - Message: fmt.Sprintf("Waiting for %s to report spec.providerID", machine.Spec.InfrastructureRef.Kind), - }) +func lastProbeSuccessMessage(lastProbeSuccessTime time.Time) string { + if lastProbeSuccessTime.IsZero() { + return "" + } + return fmt.Sprintf("Last successful probe at %s", lastProbeSuccessTime.Format(time.RFC3339)) +} - v1beta2conditions.Set(machine, metav1.Condition{ - Type: clusterv1.MachineNodeHealthyV1Beta2Condition, - Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeDoesNotExistV1Beta2Reason, - Message: fmt.Sprintf("Waiting for %s to report spec.providerID", machine.Spec.InfrastructureRef.Kind), - }) +func maxTime(t1, t2 time.Time) time.Time { + if t1.After(t2) { + return t1 + } + return t2 } // summarizeNodeV1Beta2Conditions summarizes a Node's conditions (NodeReady, NodeMemoryPressure, NodeDiskPressure, NodePIDPressure). diff --git a/internal/controllers/machine/machine_controller_status_test.go b/internal/controllers/machine/machine_controller_status_test.go index a8cfa271d94e..25f7c0f5a3ef 100644 --- a/internal/controllers/machine/machine_controller_status_test.go +++ b/internal/controllers/machine/machine_controller_status_test.go @@ -570,6 +570,8 @@ func TestSummarizeNodeV1Beta2Conditions(t *testing.T) { } func TestSetNodeHealthyAndReadyConditions(t *testing.T) { + now := time.Now() + defaultMachine := clusterv1.Machine{ ObjectMeta: metav1.ObjectMeta{ Name: "machine-test", @@ -584,18 +586,78 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, } - now := time.Now() + defaultCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-test", + Namespace: metav1.NamespaceDefault, + }, + Status: clusterv1.ClusterStatus{ + InfrastructureReady: true, + Conditions: clusterv1.Conditions{ + {Type: clusterv1.ControlPlaneInitializedCondition, Status: corev1.ConditionTrue, + LastTransitionTime: metav1.Time{Time: now.Add(-5 * time.Second)}}, + }, + }, + } testCases := []struct { name string + cluster *clusterv1.Cluster machine *clusterv1.Machine node *corev1.Node nodeGetErr error lastProbeSuccessTime time.Time expectConditions []metav1.Condition }{ + { + name: "Cluster status.infrastructureReady is false", + cluster: func() *clusterv1.Cluster { + c := defaultCluster.DeepCopy() + c.Status.InfrastructureReady = false + return c + }(), + machine: defaultMachine.DeepCopy(), + expectConditions: []metav1.Condition{ + { + Type: clusterv1.MachineNodeHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: clusterv1.MachineNodeInspectionFailedV1Beta2Reason, + Message: "Waiting for Cluster status.infrastructureReady to be true", + }, + { + Type: clusterv1.MachineNodeReadyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: clusterv1.MachineNodeInspectionFailedV1Beta2Reason, + Message: "Waiting for Cluster status.infrastructureReady to be true", + }, + }, + }, + { + name: "Cluster control plane is not initialized", + cluster: func() *clusterv1.Cluster { + c := defaultCluster.DeepCopy() + conditions.MarkFalse(c, clusterv1.ControlPlaneInitializedCondition, "", clusterv1.ConditionSeverityError, "") + return c + }(), + machine: defaultMachine.DeepCopy(), + expectConditions: []metav1.Condition{ + { + Type: clusterv1.MachineNodeHealthyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: clusterv1.MachineNodeInspectionFailedV1Beta2Reason, + Message: "Waiting for Cluster control plane to be initialized", + }, + { + Type: clusterv1.MachineNodeReadyV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: clusterv1.MachineNodeInspectionFailedV1Beta2Reason, + Message: "Waiting for Cluster control plane to be initialized", + }, + }, + }, { name: "get NodeHealthy and NodeReady from node", + cluster: defaultCluster, machine: defaultMachine.DeepCopy(), node: &corev1.Node{ Status: corev1.NodeStatus{ @@ -625,6 +687,7 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, { name: "get NodeHealthy and NodeReady from node (Node is ready & healthy)", + cluster: defaultCluster, machine: defaultMachine.DeepCopy(), node: &corev1.Node{ Status: corev1.NodeStatus{ @@ -653,6 +716,7 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, { name: "NodeReady missing from node", + cluster: defaultCluster, machine: defaultMachine.DeepCopy(), node: &corev1.Node{ Status: corev1.NodeStatus{ @@ -679,7 +743,8 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, }, { - name: "node that existed not found while machine is deleting", + name: "node that existed not found while machine is deleting", + cluster: defaultCluster, machine: func() *clusterv1.Machine { m := defaultMachine.DeepCopy() m.SetDeletionTimestamp(&metav1.Time{Time: time.Now()}) @@ -706,7 +771,8 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, }, { - name: "node not found while machine is deleting", + name: "node not found while machine is deleting", + cluster: defaultCluster, machine: func() *clusterv1.Machine { m := defaultMachine.DeepCopy() m.SetDeletionTimestamp(&metav1.Time{Time: time.Now()}) @@ -730,7 +796,8 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, }, { - name: "node missing while machine is still running", + name: "node missing while machine is still running", + cluster: defaultCluster, machine: func() *clusterv1.Machine { m := defaultMachine.DeepCopy() m.Status.NodeRef = &corev1.ObjectReference{ @@ -756,7 +823,8 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, }, { - name: "machine with ProviderID set, Node still missing", + name: "machine with ProviderID set, Node still missing", + cluster: defaultCluster, machine: func() *clusterv1.Machine { m := defaultMachine.DeepCopy() m.Spec.ProviderID = ptr.To("foo://test-node-1") @@ -781,6 +849,7 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, { name: "machine with ProviderID not yet set, waiting for it", + cluster: defaultCluster, machine: defaultMachine.DeepCopy(), node: nil, lastProbeSuccessTime: now, @@ -800,7 +869,16 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, }, { - name: "connection down, don't update existing conditions (remote conditions grace period not passed yet)", + name: "connection down, preserve conditions as they have been set before (remote conditions grace period not passed yet)", + cluster: func() *clusterv1.Cluster { + c := defaultCluster.DeepCopy() + for i, condition := range c.Status.Conditions { + if condition.Type == clusterv1.ControlPlaneInitializedCondition { + c.Status.Conditions[i].LastTransitionTime.Time = now.Add(-4 * time.Minute) + } + } + return c + }(), machine: func() *clusterv1.Machine { m := defaultMachine.DeepCopy() v1beta2conditions.Set(m, metav1.Condition{ @@ -818,8 +896,10 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }(), node: nil, nodeGetErr: errors.Wrapf(clustercache.ErrClusterNotConnected, "error getting client"), - lastProbeSuccessTime: now.Add(-3 * time.Minute), // remoteConditionsGracePeriod is 5m + lastProbeSuccessTime: now.Add(-3 * time.Minute), // Conditions have not been updated. + // remoteConditionsGracePeriod is 5m + // control plane is initialized since 4m ago, last probe success was 3m ago. expectConditions: []metav1.Condition{ { Type: clusterv1.MachineNodeHealthyV1Beta2Condition, @@ -835,49 +915,88 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { }, }, { - name: "connection down, set conditions if they haven't been set before (remote conditions grace period not passed yet)", + name: "connection down, set conditions as they haven't been set before (remote conditions grace period not passed yet)", + cluster: func() *clusterv1.Cluster { + c := defaultCluster.DeepCopy() + for i, condition := range c.Status.Conditions { + if condition.Type == clusterv1.ControlPlaneInitializedCondition { + c.Status.Conditions[i].LastTransitionTime.Time = now.Add(-4 * time.Minute) + } + } + return c + }(), machine: defaultMachine.DeepCopy(), node: nil, nodeGetErr: errors.Wrapf(clustercache.ErrClusterNotConnected, "error getting client"), - lastProbeSuccessTime: now.Add(-3 * time.Minute), // remoteConditionsGracePeriod is 5m + lastProbeSuccessTime: now.Add(-3 * time.Minute), + // Conditions have been set. + // remoteConditionsGracePeriod is 5m + // control plane is initialized since 4m ago, last probe success was 3m ago. expectConditions: []metav1.Condition{ { Type: clusterv1.MachineNodeReadyV1Beta2Condition, Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeRemoteConnectionDownV1Beta2Reason, - Message: "Cannot determine Node state, connection to the cluster is down", + Reason: clusterv1.MachineNodeConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-3*time.Minute).Format(time.RFC3339)), }, { Type: clusterv1.MachineNodeHealthyV1Beta2Condition, Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeRemoteConnectionDownV1Beta2Reason, - Message: "Cannot determine Node state, connection to the cluster is down", + Reason: clusterv1.MachineNodeConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-3*time.Minute).Format(time.RFC3339)), }, }, }, { - name: "connection down, set conditions to unknown (remote conditions grace period passed)", - machine: defaultMachine.DeepCopy(), + name: "connection down, set conditions to unknown (remote conditions grace period passed)", + cluster: func() *clusterv1.Cluster { + c := defaultCluster.DeepCopy() + for i, condition := range c.Status.Conditions { + if condition.Type == clusterv1.ControlPlaneInitializedCondition { + c.Status.Conditions[i].LastTransitionTime.Time = now.Add(-7 * time.Minute) + } + } + return c + }(), + machine: func() *clusterv1.Machine { + m := defaultMachine.DeepCopy() + v1beta2conditions.Set(m, metav1.Condition{ + Type: clusterv1.MachineNodeHealthyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: v1beta2conditions.MultipleInfoReportedReason, + }) + v1beta2conditions.Set(m, metav1.Condition{ + Type: clusterv1.MachineNodeReadyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: "KubeletReady", + Message: "kubelet is posting ready status (from Node)", + }) + return m + }(), node: nil, nodeGetErr: errors.Wrapf(clustercache.ErrClusterNotConnected, "error getting client"), - lastProbeSuccessTime: now.Add(-6 * time.Minute), // remoteConditionsGracePeriod is 5m + lastProbeSuccessTime: now.Add(-6 * time.Minute), + // Conditions have been updated. + // remoteConditionsGracePeriod is 5m + // control plane is initialized since 7m ago, last probe success was 6m ago. expectConditions: []metav1.Condition{ { Type: clusterv1.MachineNodeReadyV1Beta2Condition, Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeRemoteConnectionFailedV1Beta2Reason, - Message: fmt.Sprintf("Remote connection down since %s", now.Add(-6*time.Minute).Format(time.RFC3339)), + Reason: clusterv1.MachineNodeConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-6*time.Minute).Format(time.RFC3339)), }, { Type: clusterv1.MachineNodeHealthyV1Beta2Condition, Status: metav1.ConditionUnknown, - Reason: clusterv1.MachineNodeRemoteConnectionFailedV1Beta2Reason, - Message: fmt.Sprintf("Remote connection down since %s", now.Add(-6*time.Minute).Format(time.RFC3339)), + Reason: clusterv1.MachineNodeConnectionDownV1Beta2Reason, + Message: fmt.Sprintf("Last successful probe at %s", now.Add(-6*time.Minute).Format(time.RFC3339)), }, }, }, { name: "internal error occurred when trying to get Node", + cluster: defaultCluster, machine: defaultMachine.DeepCopy(), nodeGetErr: errors.Errorf("error creating watch machine-watchNodes for *v1.Node"), lastProbeSuccessTime: now, @@ -902,7 +1021,7 @@ func TestSetNodeHealthyAndReadyConditions(t *testing.T) { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) - setNodeHealthyAndReadyConditions(ctx, tc.machine, tc.node, tc.nodeGetErr, tc.lastProbeSuccessTime, 5*time.Minute) + setNodeHealthyAndReadyConditions(ctx, tc.cluster, tc.machine, tc.node, tc.nodeGetErr, tc.lastProbeSuccessTime, 5*time.Minute) g.Expect(tc.machine.GetV1Beta2Conditions()).To(v1beta2conditions.MatchConditions(tc.expectConditions, v1beta2conditions.IgnoreLastTransitionTime(true))) }) } diff --git a/internal/controllers/machine/suite_test.go b/internal/controllers/machine/suite_test.go index 35fb52d9aadc..b6e00091e0d6 100644 --- a/internal/controllers/machine/suite_test.go +++ b/internal/controllers/machine/suite_test.go @@ -24,13 +24,9 @@ import ( "time" . "github.com/onsi/gomega" - "github.com/onsi/gomega/types" - "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -113,42 +109,3 @@ func TestMain(m *testing.M) { SetupReconcilers: setupReconcilers, })) } - -func ContainRefOfGroupKind(group, kind string) types.GomegaMatcher { - return &refGroupKindMatcher{ - kind: kind, - group: group, - } -} - -type refGroupKindMatcher struct { - kind string - group string -} - -func (matcher *refGroupKindMatcher) Match(actual interface{}) (success bool, err error) { - ownerRefs, ok := actual.([]metav1.OwnerReference) - if !ok { - return false, errors.Errorf("expected []metav1.OwnerReference; got %T", actual) - } - - for _, ref := range ownerRefs { - gv, err := schema.ParseGroupVersion(ref.APIVersion) - if err != nil { - return false, nil //nolint:nilerr // If we can't get the group version we can't match, but it's not a failure - } - if ref.Kind == matcher.kind && gv.Group == clusterv1.GroupVersion.Group { - return true, nil - } - } - - return false, nil -} - -func (matcher *refGroupKindMatcher) FailureMessage(actual interface{}) (message string) { - return fmt.Sprintf("Expected %+v to contain refs of Group %s and Kind %s", actual, matcher.group, matcher.kind) -} - -func (matcher *refGroupKindMatcher) NegatedFailureMessage(actual interface{}) (message string) { - return fmt.Sprintf("Expected %+v not to contain refs of Group %s and Kind %s", actual, matcher.group, matcher.kind) -} diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index ddbddbaf5e64..11cd135b7460 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -89,8 +89,8 @@ type Reconciler struct { } func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { - if r.Client == nil || r.APIReader == nil || r.ClusterCache == nil { - return errors.New("Client, APIReader and ClusterCache must not be nil") + if r.Client == nil || r.APIReader == nil || r.ClusterCache == nil || r.RuntimeClient == nil { + return errors.New("Client, APIReader, ClusterCache and RuntimeClient must not be nil") } predicateLog := ctrl.LoggerFrom(ctx).WithValues("controller", "topology/cluster") diff --git a/internal/controllers/topology/cluster/suite_test.go b/internal/controllers/topology/cluster/suite_test.go index b92128a834fe..715f4a52bd47 100644 --- a/internal/controllers/topology/cluster/suite_test.go +++ b/internal/controllers/topology/cluster/suite_test.go @@ -88,9 +88,10 @@ func TestMain(m *testing.M) { } if err := (&Reconciler{ - Client: mgr.GetClient(), - APIReader: mgr.GetAPIReader(), - ClusterCache: clusterCache, + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + ClusterCache: clusterCache, + RuntimeClient: fakeruntimeclient.NewRuntimeClientBuilder().Build(), }).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: 1}); err != nil { panic(fmt.Sprintf("unable to create topology cluster reconciler: %v", err)) } diff --git a/main.go b/main.go index 9cc34c5f103e..f53712e4d524 100644 --- a/main.go +++ b/main.go @@ -296,6 +296,11 @@ func main() { setupLog.Error(errors.Errorf("--remote-conditions-grace-period must be greater than --remote-connection-grace-period"), "Unable to start manager") os.Exit(1) } + if remoteConditionsGracePeriod < 2*time.Minute { + // A minimum of 2m is enforced to ensure the ClusterCache always drops the connection before the grace period is reached. + setupLog.Error(errors.Errorf("--remote-conditions-grace-period must be at least 2m"), "Unable to start manager") + os.Exit(1) + } if err := version.CheckKubernetesVersion(restConfig, minVer); err != nil { setupLog.Error(err, "Unable to start manager") diff --git a/util/collections/machine_filters.go b/util/collections/machine_filters.go index a93ab03388ee..be1f0d2d6493 100644 --- a/util/collections/machine_filters.go +++ b/util/collections/machine_filters.go @@ -150,7 +150,7 @@ func HasDeletionTimestamp(machine *clusterv1.Machine) bool { // IsUnhealthyAndOwnerRemediated returns a filter to find all machines that have a MachineHealthCheckSucceeded condition set to False, // indicating a problem was detected on the machine, and the MachineOwnerRemediated condition set to False, indicating that the machine owner is -// responsible of performing remediation for the machine. +// responsible for performing remediation for the machine. func IsUnhealthyAndOwnerRemediated(machine *clusterv1.Machine) bool { if machine == nil { return false From 54180d29ede1ff8acdf9e7a79faab63c5a0d4d8b Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Mon, 28 Oct 2024 14:14:19 +0100 Subject: [PATCH 2/4] Fix review findings --- controlplane/kubeadm/internal/controllers/controller.go | 6 ++++++ controlplane/kubeadm/main.go | 3 +++ internal/controllers/machine/machine_controller.go | 4 ++++ internal/controllers/machine/machine_controller_status.go | 2 ++ main.go | 3 +++ 5 files changed, 18 insertions(+) diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/controlplane/kubeadm/internal/controllers/controller.go index 03fad8444c22..df336c3594e4 100644 --- a/controlplane/kubeadm/internal/controllers/controller.go +++ b/controlplane/kubeadm/internal/controllers/controller.go @@ -100,6 +100,10 @@ func (r *KubeadmControlPlaneReconciler) SetupWithManager(ctx context.Context, mg if r.Client == nil || r.SecretCachingClient == nil || r.ClusterCache == nil || r.EtcdDialTimeout == time.Duration(0) || r.EtcdCallTimeout == time.Duration(0) || r.RemoteConditionsGracePeriod < 2*time.Minute { + // A minimum of 2m is enforced to ensure the ClusterCache always drops the connection before the grace period is reached. + // In the worst case the ClusterCache will take FailureThreshold x (Interval + Timeout) = 5x(10s+5s) = 75s to drop a + // connection. There might be some additional delays in health checking under high load. So we use 2m as a minimum + // to have some buffer. return errors.New("Client, SecretCachingClient and ClusterCache must not be nil and " + "EtcdDialTimeout and EtcdCallTimeout must not be 0 and " + "RemoteConditionsGracePeriod must not be < 2m") @@ -867,6 +871,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileControlPlaneConditions(ctx cont if err != nil { if errors.Is(err, clustercache.ErrClusterNotConnected) { // If conditions are not set, set them to ConnectionDown. + // Note: This will allow to keep reporting last known status in case there are temporary connection errors. + // However, if connection errors persist more than r.RemoteConditionsGracePeriod, conditions will be overridden. setConditionsToUnknown(setConditionsToUnknownInput{ ControlPlane: controlPlane, Overwrite: false, // Don't overwrite. diff --git a/controlplane/kubeadm/main.go b/controlplane/kubeadm/main.go index 5fc6bc911687..c54a651820f2 100644 --- a/controlplane/kubeadm/main.go +++ b/controlplane/kubeadm/main.go @@ -223,6 +223,9 @@ func main() { if remoteConditionsGracePeriod < 2*time.Minute { // A minimum of 2m is enforced to ensure the ClusterCache always drops the connection before the grace period is reached. + // In the worst case the ClusterCache will take FailureThreshold x (Interval + Timeout) = 5x(10s+5s) = 75s to drop a + // connection. There might be some additional delays in health checking under high load. So we use 2m as a minimum + // to have some buffer. setupLog.Error(errors.Errorf("--remote-conditions-grace-period must be at least 2m"), "Unable to start manager") os.Exit(1) } diff --git a/internal/controllers/machine/machine_controller.go b/internal/controllers/machine/machine_controller.go index 3353eeb2f9d9..f2d0ed1bff3a 100644 --- a/internal/controllers/machine/machine_controller.go +++ b/internal/controllers/machine/machine_controller.go @@ -113,6 +113,10 @@ type Reconciler struct { func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { if r.Client == nil || r.APIReader == nil || r.ClusterCache == nil || r.RemoteConditionsGracePeriod < 2*time.Minute { + // A minimum of 2m is enforced to ensure the ClusterCache always drops the connection before the grace period is reached. + // In the worst case the ClusterCache will take FailureThreshold x (Interval + Timeout) = 5x(10s+5s) = 75s to drop a + // connection. There might be some additional delays in health checking under high load. So we use 2m as a minimum + // to have some buffer. return errors.New("Client, APIReader and ClusterCache must not be nil and RemoteConditionsGracePeriod must not be < 2m") } diff --git a/internal/controllers/machine/machine_controller_status.go b/internal/controllers/machine/machine_controller_status.go index c6f844b00738..274c9e086bda 100644 --- a/internal/controllers/machine/machine_controller_status.go +++ b/internal/controllers/machine/machine_controller_status.go @@ -257,6 +257,8 @@ func setNodeHealthyAndReadyConditions(ctx context.Context, cluster *clusterv1.Cl if nodeGetErr != nil { if errors.Is(nodeGetErr, clustercache.ErrClusterNotConnected) { // If conditions are not set, set them to ConnectionDown. + // Note: This will allow to keep reporting last known status in case there are temporary connection errors. + // However, if connection errors persist more than remoteConditionsGracePeriod, conditions will be overridden. if !v1beta2conditions.Has(machine, clusterv1.MachineNodeReadyV1Beta2Condition) || !v1beta2conditions.Has(machine, clusterv1.MachineNodeHealthyV1Beta2Condition) { setNodeConditions(machine, metav1.ConditionUnknown, diff --git a/main.go b/main.go index f53712e4d524..473d0ffdb3c0 100644 --- a/main.go +++ b/main.go @@ -298,6 +298,9 @@ func main() { } if remoteConditionsGracePeriod < 2*time.Minute { // A minimum of 2m is enforced to ensure the ClusterCache always drops the connection before the grace period is reached. + // In the worst case the ClusterCache will take FailureThreshold x (Interval + Timeout) = 5x(10s+5s) = 75s to drop a + // connection. There might be some additional delays in health checking under high load. So we use 2m as a minimum + // to have some buffer. setupLog.Error(errors.Errorf("--remote-conditions-grace-period must be at least 2m"), "Unable to start manager") os.Exit(1) } From 3d25bbda9abbd121e9960f2fa84ddb006164a189 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Mon, 28 Oct 2024 16:01:33 +0100 Subject: [PATCH 3/4] Fix review findings --- .../api/v1beta1/v1beta2_condition_consts.go | 16 ++++- .../internal/controllers/controller.go | 12 ++++ .../internal/controllers/controller_test.go | 61 +++++++++++++++++++ .../kubeadm/internal/controllers/status.go | 5 ++ .../internal/controllers/status_test.go | 1 + ...240916-improve-status-in-CAPI-resources.md | 1 + 6 files changed, 95 insertions(+), 1 deletion(-) diff --git a/controlplane/kubeadm/api/v1beta1/v1beta2_condition_consts.go b/controlplane/kubeadm/api/v1beta1/v1beta2_condition_consts.go index 981f6f787eca..81c51b106392 100644 --- a/controlplane/kubeadm/api/v1beta1/v1beta2_condition_consts.go +++ b/controlplane/kubeadm/api/v1beta1/v1beta2_condition_consts.go @@ -20,11 +20,25 @@ import clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" // KubeadmControlPlane's Available condition and corresponding reasons that will be used in v1Beta2 API version. const ( - // KubeadmControlPlaneAvailableV1Beta2Condition True if the control plane can be reached, EtcdClusterHealthy is true, + // KubeadmControlPlaneAvailableV1Beta2Condition is True if the control plane can be reached, EtcdClusterHealthy is true, // and CertificatesAvailable is true. KubeadmControlPlaneAvailableV1Beta2Condition = clusterv1.AvailableV1Beta2Condition ) +// KubeadmControlPlane's Initialized condition and corresponding reasons that will be used in v1Beta2 API version. +const ( + // KubeadmControlPlaneInitializedV1Beta2Condition is True when the control plane is functional enough to accept + // requests. This information is usually used as a signal for starting all the provisioning operations that + // depend on a functional API server, but do not require a full HA control plane to exist. + KubeadmControlPlaneInitializedV1Beta2Condition = "Initialized" + + // KubeadmControlPlaneInitializedV1Beta2Reason surfaces when the control plane is initialized. + KubeadmControlPlaneInitializedV1Beta2Reason = "Initialized" + + // KubeadmControlPlaneNotInitializedV1Beta2Reason surfaces when the control plane is not initialized. + KubeadmControlPlaneNotInitializedV1Beta2Reason = "NotInitialized" +) + // KubeadmControlPlane's CertificatesAvailable condition and corresponding reasons that will be used in v1Beta2 API version. const ( // KubeadmControlPlaneCertificatesAvailableV1Beta2Condition True if all the cluster certificates exist. diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/controlplane/kubeadm/internal/controllers/controller.go index df336c3594e4..84510d92f760 100644 --- a/controlplane/kubeadm/internal/controllers/controller.go +++ b/controlplane/kubeadm/internal/controllers/controller.go @@ -344,6 +344,7 @@ func patchKubeadmControlPlane(ctx context.Context, patchHelper *patch.Helper, kc }}, patch.WithOwnedV1Beta2Conditions{Conditions: []string{ controlplanev1.KubeadmControlPlaneAvailableV1Beta2Condition, + controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, controlplanev1.KubeadmControlPlaneCertificatesAvailableV1Beta2Condition, controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition, @@ -489,6 +490,11 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, controlPl // Create new Machine w/ init log.Info("Initializing control plane", "desired", desiredReplicas, "existing", numMachines) conditions.MarkFalse(controlPlane.KCP, controlplanev1.AvailableCondition, controlplanev1.WaitingForKubeadmInitReason, clusterv1.ConditionSeverityInfo, "") + v1beta2conditions.Set(controlPlane.KCP, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: controlplanev1.KubeadmControlPlaneNotInitializedV1Beta2Reason, + }) return r.initializeControlPlane(ctx, controlPlane) // We are scaling up case numMachines < desiredReplicas && numMachines > 0: @@ -826,6 +832,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileControlPlaneConditions(ctx cont // Note: The Machine controller uses the ControlPlaneInitialized condition on the Cluster instead for // the same check. We don't use the ControlPlaneInitialized condition from the Cluster here because KCP // Reconcile does (currently) not get triggered from condition changes to the Cluster object. + // TODO: Once we moved to v1beta2 conditions we should use the `Initialized` condition instead. controlPlaneInitialized := conditions.Get(controlPlane.KCP, controlplanev1.AvailableCondition) if !controlPlane.KCP.Status.Initialized || controlPlaneInitialized == nil || controlPlaneInitialized.Status != corev1.ConditionTrue { @@ -873,6 +880,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileControlPlaneConditions(ctx cont // If conditions are not set, set them to ConnectionDown. // Note: This will allow to keep reporting last known status in case there are temporary connection errors. // However, if connection errors persist more than r.RemoteConditionsGracePeriod, conditions will be overridden. + // Note: Usually EtcdClusterHealthy and ControlPlaneComponentsHealthy have already been set before we reach this code, + // which means that usually we don't set any conditions here (because we use Overwrite: false). setConditionsToUnknown(setConditionsToUnknownInput{ ControlPlane: controlPlane, Overwrite: false, // Don't overwrite. @@ -917,6 +926,9 @@ type setConditionsToUnknownInput struct { } func setConditionsToUnknown(input setConditionsToUnknownInput) { + // Note: We are not checking if conditions on the Machines are already set, we just check the KCP conditions instead. + // This means if Overwrite is set to false, we only set the EtcdMemberHealthy condition if the EtcdClusterHealthy condition is not set. + // The same applies to ControlPlaneComponentsHealthy and the control plane component conditions on the Machines. etcdClusterHealthySet := v1beta2conditions.Has(input.ControlPlane.KCP, controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition) controlPlaneComponentsHealthySet := v1beta2conditions.Has(input.ControlPlane.KCP, controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyV1Beta2Condition) diff --git a/controlplane/kubeadm/internal/controllers/controller_test.go b/controlplane/kubeadm/internal/controllers/controller_test.go index 82fa2e66f5e7..b2020b6755ea 100644 --- a/controlplane/kubeadm/internal/controllers/controller_test.go +++ b/controlplane/kubeadm/internal/controllers/controller_test.go @@ -1379,6 +1379,7 @@ kubernetesVersion: metav1.16.1 g.Expect(kcp.Status.Selector).NotTo(BeEmpty()) g.Expect(kcp.Status.Replicas).To(BeEquivalentTo(1)) g.Expect(conditions.IsFalse(kcp, controlplanev1.AvailableCondition)).To(BeTrue()) + g.Expect(v1beta2conditions.IsFalse(kcp, controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition)).To(BeTrue()) s, err := secret.GetFromNamespacedName(ctx, env, client.ObjectKey{Namespace: cluster.Namespace, Name: "foo"}, secret.ClusterCA) g.Expect(err).ToNot(HaveOccurred()) @@ -1873,6 +1874,16 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin {Type: controlplanev1.AvailableCondition, Status: corev1.ConditionTrue, LastTransitionTime: metav1.Time{Time: now.Add(-5 * time.Second)}}, }, + V1Beta2: &controlplanev1.KubeadmControlPlaneV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, + LastTransitionTime: metav1.Time{Time: now.Add(-5 * time.Second)}, + }, + }, + }, }, } @@ -1892,10 +1903,20 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin kcp := defaultKCP.DeepCopy() kcp.Status.Initialized = false conditions.MarkFalse(kcp, controlplanev1.AvailableCondition, "", clusterv1.ConditionSeverityError, "") + v1beta2conditions.Set(kcp, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: controlplanev1.KubeadmControlPlaneNotInitializedV1Beta2Reason, + }) return kcp }(), }, expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: controlplanev1.KubeadmControlPlaneNotInitializedV1Beta2Reason, + }, { Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, Status: metav1.ConditionUnknown, @@ -1921,6 +1942,11 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin kcp.Status.Conditions[i].LastTransitionTime.Time = now.Add(-4 * time.Minute) } } + for i, condition := range kcp.Status.V1Beta2.Conditions { + if condition.Type == controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition { + kcp.Status.V1Beta2.Conditions[i].LastTransitionTime.Time = now.Add(-4 * time.Minute) + } + } v1beta2conditions.Set(kcp, metav1.Condition{ Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, Status: metav1.ConditionTrue, @@ -1964,6 +1990,11 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin expectErr: "cannot get client for the workload cluster: error getting REST config: " + "connection to the workload cluster is down", expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, + }, { Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, Status: metav1.ConditionTrue, @@ -1994,6 +2025,11 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin kcp.Status.Conditions[i].LastTransitionTime.Time = now.Add(-4 * time.Minute) } } + for i, condition := range kcp.Status.V1Beta2.Conditions { + if condition.Type == controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition { + kcp.Status.V1Beta2.Conditions[i].LastTransitionTime.Time = now.Add(-4 * time.Minute) + } + } return kcp }(), Machines: map[string]*clusterv1.Machine{ @@ -2011,6 +2047,11 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin expectErr: "cannot get client for the workload cluster: error getting REST config: " + "connection to the workload cluster is down", expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, + }, { Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, Status: metav1.ConditionUnknown, @@ -2068,6 +2109,11 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin kcp.Status.Conditions[i].LastTransitionTime.Time = now.Add(-7 * time.Minute) } } + for i, condition := range kcp.Status.V1Beta2.Conditions { + if condition.Type == controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition { + kcp.Status.V1Beta2.Conditions[i].LastTransitionTime.Time = now.Add(-7 * time.Minute) + } + } v1beta2conditions.Set(kcp, metav1.Condition{ Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, Status: metav1.ConditionTrue, @@ -2107,6 +2153,11 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin // control plane is initialized since 7m ago, last probe success was 6m ago. expectErr: "connection to the workload cluster is down", expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, + }, { Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, Status: metav1.ConditionUnknown, @@ -2169,6 +2220,11 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin lastProbeSuccessTime: now.Add(-3 * time.Minute), expectErr: "cannot get client for the workload cluster: failed to get secret; etcd CA bundle", expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, + }, { Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, Status: metav1.ConditionUnknown, @@ -2230,6 +2286,11 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneConditions(t *testin }, lastProbeSuccessTime: now.Add(-3 * time.Minute), expectKCPConditions: []metav1.Condition{ + { + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, + }, { Type: controlplanev1.KubeadmControlPlaneEtcdClusterHealthyV1Beta2Condition, Status: metav1.ConditionUnknown, diff --git a/controlplane/kubeadm/internal/controllers/status.go b/controlplane/kubeadm/internal/controllers/status.go index 721f4b1abe48..f1b750015b48 100644 --- a/controlplane/kubeadm/internal/controllers/status.go +++ b/controlplane/kubeadm/internal/controllers/status.go @@ -108,6 +108,11 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, contro if status.HasKubeadmConfig { controlPlane.KCP.Status.Initialized = true conditions.MarkTrue(controlPlane.KCP, controlplanev1.AvailableCondition) + v1beta2conditions.Set(controlPlane.KCP, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, + }) } if controlPlane.KCP.Status.ReadyReplicas > 0 { diff --git a/controlplane/kubeadm/internal/controllers/status_test.go b/controlplane/kubeadm/internal/controllers/status_test.go index 467679f24e78..637ff0b77746 100644 --- a/controlplane/kubeadm/internal/controllers/status_test.go +++ b/controlplane/kubeadm/internal/controllers/status_test.go @@ -769,6 +769,7 @@ func TestKubeadmControlPlaneReconciler_updateStatusAllMachinesReady(t *testing.T g.Expect(kcp.Status.Initialized).To(BeTrue()) g.Expect(conditions.IsTrue(kcp, controlplanev1.AvailableCondition)).To(BeTrue()) g.Expect(conditions.IsTrue(kcp, controlplanev1.MachinesCreatedCondition)).To(BeTrue()) + g.Expect(v1beta2conditions.IsTrue(kcp, controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition)).To(BeTrue()) g.Expect(kcp.Status.Ready).To(BeTrue()) } diff --git a/docs/proposals/20240916-improve-status-in-CAPI-resources.md b/docs/proposals/20240916-improve-status-in-CAPI-resources.md index b996ca7084ef..7e548c46361f 100644 --- a/docs/proposals/20240916-improve-status-in-CAPI-resources.md +++ b/docs/proposals/20240916-improve-status-in-CAPI-resources.md @@ -1026,6 +1026,7 @@ Notes: | Condition | Note | |---------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `Available` | True if the control plane can be reached, `EtcdClusterHealthy` is true, and `CertificatesAvailable` is true | +| `Initialized` | True when the control plane is functional enough to accept requests. This information is usually used as a signal for starting all the provisioning operations that depend on a functional API server, but do not require a full HA control plane to exist. | | `CertificatesAvailable` | True if all the cluster certificates exist. | | `EtcdClusterHealthy` | This condition surfaces issues to the etcd cluster hosted on machines managed by this object, if any. It is computed as aggregation of Machine's `EtcdMemberHealthy` conditions plus additional checks validating potential issues to etcd quorum | | `ControlPlaneComponentsHealthy` | This condition surfaces issues to Kubernetes control plane components hosted on machines managed by this object. It is computed as aggregation of Machine's `APIServerPodHealthy`, `ControllerManagerPodHealthy`, `SchedulerPodHealthy`, `EtcdPodHealthy` conditions plus additional checks on control plane machines and nodes | From 6344197ab29230ff616338c629d92ea66dd446cb Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Mon, 28 Oct 2024 18:15:44 +0100 Subject: [PATCH 4/4] Fix review findings --- .../internal/controllers/controller.go | 5 --- .../kubeadm/internal/controllers/status.go | 23 +++++++--- .../internal/controllers/status_test.go | 45 ++++++++++++++++++- 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/controlplane/kubeadm/internal/controllers/controller.go index 84510d92f760..891cc27ac9e3 100644 --- a/controlplane/kubeadm/internal/controllers/controller.go +++ b/controlplane/kubeadm/internal/controllers/controller.go @@ -490,11 +490,6 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, controlPl // Create new Machine w/ init log.Info("Initializing control plane", "desired", desiredReplicas, "existing", numMachines) conditions.MarkFalse(controlPlane.KCP, controlplanev1.AvailableCondition, controlplanev1.WaitingForKubeadmInitReason, clusterv1.ConditionSeverityInfo, "") - v1beta2conditions.Set(controlPlane.KCP, metav1.Condition{ - Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, - Status: metav1.ConditionFalse, - Reason: controlplanev1.KubeadmControlPlaneNotInitializedV1Beta2Reason, - }) return r.initializeControlPlane(ctx, controlPlane) // We are scaling up case numMachines < desiredReplicas && numMachines > 0: diff --git a/controlplane/kubeadm/internal/controllers/status.go b/controlplane/kubeadm/internal/controllers/status.go index f1b750015b48..34013ad8cf93 100644 --- a/controlplane/kubeadm/internal/controllers/status.go +++ b/controlplane/kubeadm/internal/controllers/status.go @@ -108,11 +108,6 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, contro if status.HasKubeadmConfig { controlPlane.KCP.Status.Initialized = true conditions.MarkTrue(controlPlane.KCP, controlplanev1.AvailableCondition) - v1beta2conditions.Set(controlPlane.KCP, metav1.Condition{ - Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, - }) } if controlPlane.KCP.Status.ReadyReplicas > 0 { @@ -166,6 +161,7 @@ func (r *KubeadmControlPlaneReconciler) updateV1beta2Status(ctx context.Context, // any reason those functions are not called before, e.g. an error, this func relies on existing Machine's condition. setReplicas(ctx, controlPlane.KCP, controlPlane.Machines) + setInitializedCondition(ctx, controlPlane.KCP) setScalingUpCondition(ctx, controlPlane.KCP, controlPlane.Machines, controlPlane.InfraMachineTemplateIsNotFound, controlPlane.PreflightCheckResults) setScalingDownCondition(ctx, controlPlane.KCP, controlPlane.Machines, controlPlane.PreflightCheckResults) setMachinesReadyCondition(ctx, controlPlane.KCP, controlPlane.Machines) @@ -198,6 +194,23 @@ func setReplicas(_ context.Context, kcp *controlplanev1.KubeadmControlPlane, mac kcp.Status.V1Beta2.UpToDateReplicas = ptr.To(upToDateReplicas) } +func setInitializedCondition(_ context.Context, kcp *controlplanev1.KubeadmControlPlane) { + if kcp.Status.Initialized { + v1beta2conditions.Set(kcp, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, + }) + return + } + + v1beta2conditions.Set(kcp, metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: controlplanev1.KubeadmControlPlaneNotInitializedV1Beta2Reason, + }) +} + func setScalingUpCondition(_ context.Context, kcp *controlplanev1.KubeadmControlPlane, machines collections.Machines, infrastructureObjectNotFound bool, preflightChecks internal.PreflightCheckResults) { if kcp.Spec.Replicas == nil { v1beta2conditions.Set(kcp, metav1.Condition{ diff --git a/controlplane/kubeadm/internal/controllers/status_test.go b/controlplane/kubeadm/internal/controllers/status_test.go index 637ff0b77746..d5ac66272b51 100644 --- a/controlplane/kubeadm/internal/controllers/status_test.go +++ b/controlplane/kubeadm/internal/controllers/status_test.go @@ -75,6 +75,50 @@ func TestSetReplicas(t *testing.T) { g.Expect(*kcp.Status.V1Beta2.UpToDateReplicas).To(Equal(int32(4))) } +func Test_setInitializedCondition(t *testing.T) { + tests := []struct { + name string + controlPlane *internal.ControlPlane + expectCondition metav1.Condition + }{ + { + name: "KCP not initialized", + controlPlane: &internal.ControlPlane{ + KCP: &controlplanev1.KubeadmControlPlane{}, + }, + expectCondition: metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: controlplanev1.KubeadmControlPlaneNotInitializedV1Beta2Reason, + }, + }, + { + name: "KCP initialized", + controlPlane: &internal.ControlPlane{ + KCP: &controlplanev1.KubeadmControlPlane{ + Status: controlplanev1.KubeadmControlPlaneStatus{Initialized: true}, + }, + }, + expectCondition: metav1.Condition{ + Type: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneInitializedV1Beta2Reason, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + setInitializedCondition(ctx, tt.controlPlane.KCP) + + condition := v1beta2conditions.Get(tt.controlPlane.KCP, controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition) + g.Expect(condition).ToNot(BeNil()) + g.Expect(*condition).To(v1beta2conditions.MatchCondition(tt.expectCondition, v1beta2conditions.IgnoreLastTransitionTime(true))) + }) + } +} + func Test_setScalingUpCondition(t *testing.T) { tests := []struct { name string @@ -769,7 +813,6 @@ func TestKubeadmControlPlaneReconciler_updateStatusAllMachinesReady(t *testing.T g.Expect(kcp.Status.Initialized).To(BeTrue()) g.Expect(conditions.IsTrue(kcp, controlplanev1.AvailableCondition)).To(BeTrue()) g.Expect(conditions.IsTrue(kcp, controlplanev1.MachinesCreatedCondition)).To(BeTrue()) - g.Expect(v1beta2conditions.IsTrue(kcp, controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition)).To(BeTrue()) g.Expect(kcp.Status.Ready).To(BeTrue()) }