Retry VMSS Flex validation if no parent MP is found

Author: mboersma

exp/api/v1beta1/azuremachinepool_webhook.go

@@ -17,7 +17,6 @@ limitations under the License.
 package v1beta1
 
 import (
-	"context"
 	"fmt"
 	"reflect"
 
@@ -30,7 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 	"sigs.k8s.io/cluster-api-provider-azure/feature"
-	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
+	"sigs.k8s.io/cluster-api-provider-azure/util/azure"
 	capifeature "sigs.k8s.io/cluster-api/feature"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -249,22 +248,15 @@ func (amp *AzureMachinePool) ValidateOrchestrationMode(c client.Client) func() e
 	return func() error {
 		// Only Flexible orchestration mode requires validation.
 		if amp.Spec.OrchestrationMode == infrav1.OrchestrationModeType(compute.OrchestrationModeFlexible) {
-			// Find the owner MachinePool
-			ownerMachinePool := &expv1.MachinePool{}
-			key := client.ObjectKey{
-				Namespace: amp.Namespace,
-				Name:      amp.Name,
-			}
-			ctx := context.Background()
-			if err := c.Get(ctx, key, ownerMachinePool); err != nil {
-				return errors.Wrap(err, "failed to get owner MachinePool")
+			parent, err := azure.FindParentMachinePoolWithRetry(amp.Name, c, 5)
+			if err != nil {
+				return errors.Wrap(err, "failed to find parent MachinePool")
 			}
-
 			// Kubernetes must be >= 1.26.0 for cloud-provider-azure Helm chart support.
-			if ownerMachinePool.Spec.Template.Spec.Version == nil {
+			if parent.Spec.Template.Spec.Version == nil {
 				return errors.New("could not find Kubernetes version in MachinePool")
 			}
-			k8sVersion, err := semver.ParseTolerant(*ownerMachinePool.Spec.Template.Spec.Version)
+			k8sVersion, err := semver.ParseTolerant(*parent.Spec.Template.Spec.Version)
 			if err != nil {
 				return errors.Wrap(err, "failed to parse Kubernetes version")
 			}

exp/api/v1beta1/azuremachinepool_webhook_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 	guuid "github.com/google/uuid"
 	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
 	"golang.org/x/crypto/ssh"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/uuid"
@@ -52,10 +53,11 @@ func TestAzureMachinePool_ValidateCreate(t *testing.T) {
 	g := NewWithT(t)
 
 	tests := []struct {
-		name    string
-		amp     *AzureMachinePool
-		version string
-		wantErr bool
+		name          string
+		amp           *AzureMachinePool
+		version       string
+		ownerNotFound bool
+		wantErr       bool
 	}{
 		{
 			name:    "valid",
@@ -201,10 +203,17 @@ func TestAzureMachinePool_ValidateCreate(t *testing.T) {
 			version: "v1.25.6",
 			wantErr: true,
 		},
+		{
+			name:          "azuremachinepool with Flexible orchestration mode and invalid Kubernetes version, no owner",
+			amp:           createMachinePoolWithOrchestrationMode(compute.OrchestrationModeFlexible),
+			version:       "v1.25.6",
+			ownerNotFound: true,
+			wantErr:       true,
+		},
 	}
 
 	for _, tc := range tests {
-		client := mockClient{Version: tc.version}
+		client := mockClient{Version: tc.version, ReturnError: tc.ownerNotFound}
 		t.Run(tc.name, func(t *testing.T) {
 			err := tc.amp.ValidateCreate(client)
 			if tc.wantErr {
@@ -218,11 +227,18 @@ func TestAzureMachinePool_ValidateCreate(t *testing.T) {
 
 type mockClient struct {
 	client.Client
-	Version string
+	Version     string
+	ReturnError bool
 }
 
-func (m mockClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
-	obj.(*expv1.MachinePool).Spec.Template.Spec.Version = &m.Version
+func (m mockClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
+	if m.ReturnError {
+		return errors.New("MachinePool.cluster.x-k8s.io \"mock-machinepool-mp-0\" not found")
+	}
+	mp := &expv1.MachinePool{}
+	mp.Spec.Template.Spec.Version = &m.Version
+	list.(*expv1.MachinePoolList).Items = []expv1.MachinePool{*mp}
+
 	return nil
 }
 

util/azure/azure.go

@@ -17,8 +17,10 @@ limitations under the License.
 package azure
 
 import (
+	"context"
 	"os"
 	"strings"
+	"time"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
@@ -27,6 +29,9 @@ import (
 	azureautorest "github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/go-autorest/autorest/azure/auth"
 	"github.com/jongio/azidext/go/azidext"
+	"github.com/pkg/errors"
+	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 // AzureSystemNodeLabelPrefix is a standard node label prefix for Azure features, e.g., kubernetes.azure.com/scalesetpriority.
@@ -91,3 +96,33 @@ func GetAuthorizer(settings auth.EnvironmentSettings) (autorest.Authorizer, erro
 	}
 	return azidext.NewTokenCredentialAdapter(cred, []string{scope}), nil
 }
+
+// FindParentMachinePool finds the parent MachinePool for the AzureMachinePool.
+func FindParentMachinePool(ampName string, cli client.Client) (*expv1.MachinePool, error) {
+	ctx := context.Background()
+	machinePoolList := &expv1.MachinePoolList{}
+	if err := cli.List(ctx, machinePoolList); err != nil {
+		return nil, errors.Wrapf(err, "failed to list MachinePools for %s", ampName)
+	}
+	for _, mp := range machinePoolList.Items {
+		if mp.Spec.Template.Spec.InfrastructureRef.Name == ampName {
+			return &mp, nil
+		}
+	}
+	return nil, errors.Errorf("failed to get MachinePool for %s", ampName)
+}
+
+// FindParentMachinePoolWithRetry finds the parent MachinePool for the AzureMachinePool with retry.
+func FindParentMachinePoolWithRetry(ampName string, cli client.Client, maxAttempts int) (*expv1.MachinePool, error) {
+	for i := 1; ; i++ {
+		p, err := FindParentMachinePool(ampName, cli)
+		if err != nil {
+			if i >= maxAttempts {
+				return nil, errors.Wrap(err, "failed to find parent MachinePool")
+			}
+			time.Sleep(1 * time.Second)
+			continue
+		}
+		return p, nil
+	}
+}

util/azure/azure_test.go

@@ -0,0 +1,126 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package azure
+
+import (
+	"context"
+	"testing"
+
+	. "github.com/onsi/gomega"
+	utilfeature "k8s.io/component-base/featuregate/testing"
+	"sigs.k8s.io/cluster-api-provider-azure/feature"
+	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
+	capifeature "sigs.k8s.io/cluster-api/feature"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func TestFindParentMachinePool(t *testing.T) {
+	defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, capifeature.MachinePool, true)()
+	g := NewWithT(t)
+	client := mockClient{}
+
+	tests := []struct {
+		name    string
+		mpName  string
+		wantErr bool
+	}{
+		{
+			name:    "valid",
+			mpName:  "mock-machinepool-mp-0",
+			wantErr: false,
+		},
+		{
+			name:    "invalid",
+			mpName:  "mock-machinepool-mp-1",
+			wantErr: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			mp, err := FindParentMachinePool(tc.mpName, client)
+			if tc.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(mp).NotTo(BeNil())
+			}
+		})
+	}
+}
+
+func TestFindParentMachinePoolWithRetry(t *testing.T) {
+	defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, capifeature.MachinePool, true)()
+	g := NewWithT(t)
+	client := mockClient{}
+
+	tests := []struct {
+		name        string
+		mpName      string
+		maxAttempts int
+		wantErr     bool
+	}{
+		{
+			name:        "valid",
+			mpName:      "mock-machinepool-mp-0",
+			maxAttempts: 1,
+			wantErr:     false,
+		},
+		{
+			name:        "valid with retries",
+			mpName:      "mock-machinepool-mp-0",
+			maxAttempts: 5,
+			wantErr:     false,
+		},
+		{
+			name:        "invalid",
+			mpName:      "mock-machinepool-mp-1",
+			maxAttempts: 1,
+			wantErr:     true,
+		},
+		{
+			name:        "invalid with retries",
+			mpName:      "mock-machinepool-mp-1",
+			maxAttempts: 5,
+			wantErr:     true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			mp, err := FindParentMachinePoolWithRetry(tc.mpName, client, tc.maxAttempts)
+			if tc.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(mp).NotTo(BeNil())
+			}
+		})
+	}
+}
+
+type mockClient struct {
+	client.Client
+}
+
+func (m mockClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
+	mp := &expv1.MachinePool{}
+	mp.Spec.Template.Spec.InfrastructureRef.Name = "mock-machinepool-mp-0"
+	list.(*expv1.MachinePoolList).Items = []expv1.MachinePool{*mp}
+
+	return nil
+}