Skip to content

Commit

Permalink
Merge pull request #9837 from Dhairya-Arora01/minReadySeconds
Browse files Browse the repository at this point in the history 
🌱 MinReadySeconds for machinepools
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jun 3, 2024
2 parents 6941e8c + 2e9b3b1 commit cfb5e27
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 23 deletions.
1 change: 0 additions & 1 deletion config/crd/bases/cluster.x-k8s.io_machinepools.yaml
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion exp/api/v1beta1/machinepool_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"`

Expand Down
25 changes: 9 additions & 16 deletions exp/internal/controllers/machinepool_controller_noderef.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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
}
116 changes: 111 additions & 5 deletions exp/internal/controllers/machinepool_controller_noderef_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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",
Expand All @@ -102,6 +160,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) {
references: []corev1.ObjectReference{
{Name: "node-1"},
},
available: 1,
ready: 1,
},
},
{
Expand All @@ -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,
},
},
{
Expand All @@ -120,6 +193,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) {
references: []corev1.ObjectReference{
{Name: "gce-node-2"},
},
available: 1,
ready: 1,
},
},
{
Expand All @@ -129,6 +204,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) {
references: []corev1.ObjectReference{
{Name: "azure-node-4"},
},
available: 1,
ready: 1,
},
},
{
Expand All @@ -139,6 +216,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) {
{Name: "node-1"},
{Name: "azure-node-4"},
},
available: 2,
ready: 2,
},
},
{
Expand All @@ -163,6 +242,8 @@ func TestMachinePoolGetNodeReference(t *testing.T) {
references: []corev1.ObjectReference{
{Name: "azure-nodepool1-0"},
},
available: 1,
ready: 1,
},
},
{
Expand All @@ -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 {
Expand All @@ -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)
Expand Down
1 change: 1 addition & 0 deletions exp/internal/controllers/machinepool_controller_phases_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1616,6 +1616,7 @@ func TestReconcileMachinePoolScaleToFromZero(t *testing.T) {
},
},
},
MinReadySeconds: ptr.To[int32](0),
},
Status: expv1.MachinePoolStatus{},
}
Expand Down

0 comments on commit cfb5e27

Please sign in to comment.