Skip to content

Commit

Permalink
Merge pull request #3080 from mboersma/vmss-flex-no-owner-validation
Browse files Browse the repository at this point in the history
Retry VMSS Flex validation if no parent MP is found
  • Loading branch information
k8s-ci-robot committed Feb 1, 2023
2 parents 6d099b0 + 3274c05 commit 9259e5b
Show file tree
Hide file tree
Showing 4 changed files with 191 additions and 22 deletions.
20 changes: 6 additions & 14 deletions exp/api/v1beta1/azuremachinepool_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package v1beta1

import (
"context"
"fmt"
"reflect"

Expand All @@ -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"
Expand Down Expand Up @@ -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")
}
Expand Down
32 changes: 24 additions & 8 deletions exp/api/v1beta1/azuremachinepool_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute"
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"
Expand All @@ -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",
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}

Expand Down
35 changes: 35 additions & 0 deletions util/azure/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand Down Expand Up @@ -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
}
}
126 changes: 126 additions & 0 deletions util/azure/azure_test.go
Original file line number Diff line number Diff line change
@@ -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
}

0 comments on commit 9259e5b

Please sign in to comment.