Skip to content

🌱 MinReadySeconds for machinepools #9837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 3, 2024
Merged

🌱 MinReadySeconds for machinepools #9837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
089e073
implement minReadySeconds for machinepool
Dhairya-Arora01 May 25, 2024
d0f12a1
validate ready and available node counts
Dhairya-Arora01 May 28, 2024
2e9b3b1
test case with nodeReady condition as false
Dhairya-Arora01 May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion config/crd/bases/cluster.x-k8s.io_machinepools.yaml
View file
Open in desktop

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
Open in desktop
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
Open in desktop
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
Open in desktop
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"}},
Copy link
Member

@sbueringer sbueringer May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in a previous comment. Let's start validating expected.available/ready against result (somewhere ~ in l.219)

Today this test never validates the available and ready fields.

In fact. At the moment they are always 0 because we never set conditions correctly to get ready or available Nodes. Let's change that as well.

(otherwise we don't have any test coverage for ready/available)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to follow the force-pushes but I see that the test is now validating available and ready fields based on a different minReadySeconds. That looks good!!

I think the only thing missing is a test case when the NodeReady condition is false, both ready and available should be 0.

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
Open in desktop
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
Loading