diff --git a/config/crd/bases/cluster.x-k8s.io_machinepools.yaml b/config/crd/bases/cluster.x-k8s.io_machinepools.yaml index d38bb91a2b44..ba81835620a1 100644 --- a/config/crd/bases/cluster.x-k8s.io_machinepools.yaml +++ b/config/crd/bases/cluster.x-k8s.io_machinepools.yaml @@ -1118,7 +1118,6 @@ spec: be ready. Defaults to 0 (machine instance will be considered available as soon as it is ready) - NOTE: No logic is implemented for this field and it currently has no behaviour. format: int32 type: integer providerIDList: diff --git a/exp/api/v1beta1/machinepool_types.go b/exp/api/v1beta1/machinepool_types.go index a2fc2957ba3f..0c7839998afa 100644 --- a/exp/api/v1beta1/machinepool_types.go +++ b/exp/api/v1beta1/machinepool_types.go @@ -49,7 +49,6 @@ type MachinePoolSpec struct { // be ready. // Defaults to 0 (machine instance will be considered available as soon as it // is ready) - // NOTE: No logic is implemented for this field and it currently has no behaviour. // +optional MinReadySeconds *int32 `json:"minReadySeconds,omitempty"` diff --git a/exp/internal/controllers/machinepool_controller_noderef.go b/exp/internal/controllers/machinepool_controller_noderef.go index e45fe3d6b76a..a2afac9557ba 100644 --- a/exp/internal/controllers/machinepool_controller_noderef.go +++ b/exp/internal/controllers/machinepool_controller_noderef.go @@ -23,11 +23,13 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/controllers/noderefutil" expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" "sigs.k8s.io/cluster-api/internal/util/taints" "sigs.k8s.io/cluster-api/util" @@ -36,9 +38,7 @@ import ( "sigs.k8s.io/cluster-api/util/patch" ) -var ( - errNoAvailableNodes = errors.New("cannot find nodes with matching ProviderIDs in ProviderIDList") -) +var errNoAvailableNodes = errors.New("cannot find nodes with matching ProviderIDs in ProviderIDList") type getNodeReferencesResult struct { references []corev1.ObjectReference @@ -82,7 +82,7 @@ func (r *MachinePoolReconciler) reconcileNodeRefs(ctx context.Context, cluster * } // Get the Node references. - nodeRefsResult, err := r.getNodeReferences(ctx, clusterClient, mp.Spec.ProviderIDList) + nodeRefsResult, err := r.getNodeReferences(ctx, clusterClient, mp.Spec.ProviderIDList, mp.Spec.MinReadySeconds) if err != nil { if err == errNoAvailableNodes { log.Info("Cannot assign NodeRefs to MachinePool, no matching Nodes") @@ -153,7 +153,7 @@ func (r *MachinePoolReconciler) deleteRetiredNodes(ctx context.Context, c client return nil } -func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client.Client, providerIDList []string) (getNodeReferencesResult, error) { +func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client.Client, providerIDList []string, minReadySeconds *int32) (getNodeReferencesResult, error) { log := ctrl.LoggerFrom(ctx, "providerIDList", len(providerIDList)) var ready, available int @@ -185,9 +185,11 @@ func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client. continue } if node, ok := nodeRefsMap[providerID]; ok { - available++ - if nodeIsReady(&node) { + if noderefutil.IsNodeReady(&node) { ready++ + if noderefutil.IsNodeAvailable(&node, *minReadySeconds, metav1.Now()) { + available++ + } } nodeRefs = append(nodeRefs, corev1.ObjectReference{ APIVersion: corev1.SchemeGroupVersion.String(), @@ -236,12 +238,3 @@ func (r *MachinePoolReconciler) patchNodes(ctx context.Context, c client.Client, } return nil } - -func nodeIsReady(node *corev1.Node) bool { - for _, n := range node.Status.Conditions { - if n.Type == corev1.NodeReady { - return n.Status == corev1.ConditionTrue - } - } - return false -} diff --git a/exp/internal/controllers/machinepool_controller_noderef_test.go b/exp/internal/controllers/machinepool_controller_noderef_test.go index 41605d5b757d..b573cb47ba38 100644 --- a/exp/internal/controllers/machinepool_controller_noderef_test.go +++ b/exp/internal/controllers/machinepool_controller_noderef_test.go @@ -23,6 +23,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/record" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -44,6 +45,14 @@ func TestMachinePoolGetNodeReference(t *testing.T) { Spec: corev1.NodeSpec{ ProviderID: "aws://us-east-1/id-node-1", }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, }, &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ @@ -52,6 +61,22 @@ func TestMachinePoolGetNodeReference(t *testing.T) { Spec: corev1.NodeSpec{ ProviderID: "aws://us-west-2/id-node-2", }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, + }, + &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-3", + }, + Spec: corev1.NodeSpec{ + ProviderID: "aws://us-west-2/id-node-3", + }, }, &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ @@ -60,6 +85,14 @@ func TestMachinePoolGetNodeReference(t *testing.T) { Spec: corev1.NodeSpec{ ProviderID: "gce://us-central1/gce-id-node-2", }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, }, &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ @@ -68,6 +101,14 @@ func TestMachinePoolGetNodeReference(t *testing.T) { Spec: corev1.NodeSpec{ ProviderID: "azure://westus2/id-node-4", }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, }, &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ @@ -76,6 +117,14 @@ func TestMachinePoolGetNodeReference(t *testing.T) { Spec: corev1.NodeSpec{ ProviderID: "azure://westus2/id-nodepool1/0", }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, }, &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ @@ -84,16 +133,25 @@ func TestMachinePoolGetNodeReference(t *testing.T) { Spec: corev1.NodeSpec{ ProviderID: "azure://westus2/id-nodepool2/0", }, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, }, } client := fake.NewClientBuilder().WithObjects(nodeList...).Build() testCases := []struct { - name string - providerIDList []string - expected *getNodeReferencesResult - err error + name string + providerIDList []string + expected *getNodeReferencesResult + err error + minReadySeconds int32 }{ { name: "valid provider id, valid aws node", @@ -102,6 +160,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) { references: []corev1.ObjectReference{ {Name: "node-1"}, }, + available: 1, + ready: 1, }, }, { @@ -111,6 +171,19 @@ func TestMachinePoolGetNodeReference(t *testing.T) { references: []corev1.ObjectReference{ {Name: "node-2"}, }, + available: 1, + ready: 1, + }, + }, + { + name: "valid provider id, valid aws node, nodeReady condition set to false", + providerIDList: []string{"aws://us-west-2/id-node-3"}, + expected: &getNodeReferencesResult{ + references: []corev1.ObjectReference{ + {Name: "node-3"}, + }, + available: 0, + ready: 0, }, }, { @@ -120,6 +193,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) { references: []corev1.ObjectReference{ {Name: "gce-node-2"}, }, + available: 1, + ready: 1, }, }, { @@ -129,6 +204,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) { references: []corev1.ObjectReference{ {Name: "azure-node-4"}, }, + available: 1, + ready: 1, }, }, { @@ -139,6 +216,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) { {Name: "node-1"}, {Name: "azure-node-4"}, }, + available: 2, + ready: 2, }, }, { @@ -163,6 +242,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) { references: []corev1.ObjectReference{ {Name: "azure-nodepool1-0"}, }, + available: 1, + ready: 1, }, }, { @@ -173,15 +254,37 @@ func TestMachinePoolGetNodeReference(t *testing.T) { {Name: "azure-nodepool1-0"}, {Name: "azure-nodepool2-0"}, }, + available: 2, + ready: 2, }, }, + { + name: "valid provider id, valid aws node, with minReadySeconds", + providerIDList: []string{"aws://us-east-1/id-node-1"}, + expected: &getNodeReferencesResult{ + references: []corev1.ObjectReference{{Name: "node-1"}}, + available: 0, + ready: 1, + }, + minReadySeconds: 20, + }, + { + name: "valid provider id, valid aws node, with minReadySeconds equals 0", + providerIDList: []string{"aws://us-east-1/id-node-1"}, + expected: &getNodeReferencesResult{ + references: []corev1.ObjectReference{{Name: "node-1"}}, + available: 1, + ready: 1, + }, + minReadySeconds: 0, + }, } for _, test := range testCases { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - result, err := r.getNodeReferences(ctx, client, test.providerIDList) + result, err := r.getNodeReferences(ctx, client, test.providerIDList, ptr.To(test.minReadySeconds)) if test.err == nil { g.Expect(err).ToNot(HaveOccurred()) } else { @@ -195,6 +298,9 @@ func TestMachinePoolGetNodeReference(t *testing.T) { g.Expect(result.references).To(HaveLen(len(test.expected.references)), "Expected NodeRef count to be %v, got %v", len(result.references), len(test.expected.references)) + g.Expect(result.available).To(Equal(test.expected.available), "Expected available node count to be %v, got %v", test.expected.available, result.available) + g.Expect(result.ready).To(Equal(test.expected.ready), "Expected ready node count to be %v, got %v", test.expected.ready, result.ready) + for n := range test.expected.references { g.Expect(result.references[n].Name).To(Equal(test.expected.references[n].Name), "Expected NodeRef's name to be %v, got %v", result.references[n].Name, test.expected.references[n].Name) g.Expect(result.references[n].Namespace).To(Equal(test.expected.references[n].Namespace), "Expected NodeRef's namespace to be %v, got %v", result.references[n].Namespace, test.expected.references[n].Namespace) diff --git a/exp/internal/controllers/machinepool_controller_phases_test.go b/exp/internal/controllers/machinepool_controller_phases_test.go index 1e4e3d64a74a..19b98c7f75b6 100644 --- a/exp/internal/controllers/machinepool_controller_phases_test.go +++ b/exp/internal/controllers/machinepool_controller_phases_test.go @@ -1616,6 +1616,7 @@ func TestReconcileMachinePoolScaleToFromZero(t *testing.T) { }, }, }, + MinReadySeconds: ptr.To[int32](0), }, Status: expv1.MachinePoolStatus{}, }