Skip to content

Make scaleset reconcile/delete async #3111

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
k8s-ci-robot merged 1 commit into from
Aug 16, 2023
Merged

Make scaleset reconcile/delete async #3111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions azure/converters/vmss.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ const (
)

// SDKToVMSS converts an Azure SDK VirtualMachineScaleSet to the AzureMachinePool type.
func SDKToVMSS(sdkvmss compute.VirtualMachineScaleSet, sdkinstances []compute.VirtualMachineScaleSetVM) *azure.VMSS {
vmss := &azure.VMSS{
func SDKToVMSS(sdkvmss compute.VirtualMachineScaleSet, sdkinstances []compute.VirtualMachineScaleSetVM) azure.VMSS {
vmss := azure.VMSS{
ID: ptr.Deref(sdkvmss.ID, ""),
Name: ptr.Deref(sdkvmss.Name, ""),
State: infrav1.ProvisioningState(ptr.Deref(sdkvmss.ProvisioningState, "")),
Expand Down
6 changes: 3 additions & 3 deletions azure/converters/vmss_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func Test_SDKToVMSS(t *testing.T) {
cases := []struct {
Name string
SubjectFactory func(*gomega.GomegaWithT) (compute.VirtualMachineScaleSet, []compute.VirtualMachineScaleSetVM)
Expect func(*gomega.GomegaWithT, *azure.VMSS)
Expect func(*gomega.GomegaWithT, azure.VMSS)
}{
{
Name: "ShouldPopulateWithData",
Expand Down Expand Up @@ -84,7 +84,7 @@ func Test_SDKToVMSS(t *testing.T) {
},
}
},
Expect: func(g *gomega.GomegaWithT, actual *azure.VMSS) {
Expect: func(g *gomega.GomegaWithT, actual azure.VMSS) {
expected := azure.VMSS{
ID: "vmssID",
Name: "vmssName",
Expand All @@ -107,7 +107,7 @@ func Test_SDKToVMSS(t *testing.T) {
State: "Succeeded",
}
}
g.Expect(actual).To(gomega.Equal(&expected))
g.Expect(actual).To(gomega.Equal(expected))
},
},
}
Expand Down
68 changes: 55 additions & 13 deletions azure/scope/machinepool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ type (
MachinePool *expv1.MachinePool
AzureMachinePool *infrav1exp.AzureMachinePool
ClusterScope azure.ClusterScoper
Caches *MachinePoolCache
Cache *MachinePoolCache
}

// MachinePoolScope defines a scope defined around a machine pool and its cluster.
Expand All @@ -77,16 +77,20 @@ type (
cache *MachinePoolCache
}

// MachinePoolCache stores common machine pool information so we don't have to hit the API multiple times within the same reconcile loop.
MachinePoolCache struct {
VMSKU resourceskus.SKU
}

// NodeStatus represents the status of a Kubernetes node.
NodeStatus struct {
Ready bool
Version string
}

// MachinePoolCache stores common machine pool information so we don't have to hit the API multiple times within the same reconcile loop.
MachinePoolCache struct {
BootstrapData string
HasBootstrapDataChanges bool
VMImage *infrav1.Image
VMSKU resourceskus.SKU
MaxSurge int
}
)

// NewMachinePoolScope creates a new MachinePoolScope from the supplied parameters.
Expand Down Expand Up @@ -133,6 +137,27 @@ func (m *MachinePoolScope) InitMachinePoolCache(ctx context.Context) error {
var err error
m.cache = &MachinePoolCache{}

m.cache.BootstrapData, err = m.GetBootstrapData(ctx)
if err != nil {
return err
}

m.cache.HasBootstrapDataChanges, err = m.HasBootstrapDataChanges(ctx)
if err != nil {
return err
}

m.cache.VMImage, err = m.GetVMImage(ctx)
if err != nil {
return err
}
m.SaveVMImageToStatus(m.cache.VMImage)

m.cache.MaxSurge, err = m.MaxSurge()
if err != nil {
return err
}

skuCache, err := resourceskus.GetCache(m, m.Location())
if err != nil {
return err
Expand All @@ -148,9 +173,19 @@ func (m *MachinePoolScope) InitMachinePoolCache(ctx context.Context) error {
}

// ScaleSetSpec returns the scale set spec.
func (m *MachinePoolScope) ScaleSetSpec() azure.ScaleSetSpec {
return azure.ScaleSetSpec{
func (m *MachinePoolScope) ScaleSetSpec(ctx context.Context) azure.ResourceSpecGetter {
ctx, log, done := tele.StartSpanWithLogger(ctx, "scope.MachinePoolScope.ScaleSetSpec")
defer done()

shouldPatchCustomData := false
if m.HasReplicasExternallyManaged(ctx) {
shouldPatchCustomData = m.cache.HasBootstrapDataChanges
log.V(4).Info("has bootstrap data changed?", "shouldPatchCustomData", shouldPatchCustomData)
}

return &scalesets.ScaleSetSpec{
Name: m.Name(),
ResourceGroup: m.ResourceGroup(),
Size: m.AzureMachinePool.Spec.Template.VMSize,
Capacity: int64(ptr.Deref[int32](m.MachinePool.Spec.Replicas, 0)),
SSHKeyData: m.AzureMachinePool.Spec.Template.SSHPublicKey,
Expand All @@ -172,6 +207,16 @@ func (m *MachinePoolScope) ScaleSetSpec() azure.ScaleSetSpec {
NetworkInterfaces: m.AzureMachinePool.Spec.Template.NetworkInterfaces,
IPv6Enabled: m.IsIPv6Enabled(),
OrchestrationMode: m.AzureMachinePool.Spec.OrchestrationMode,
Location: m.AzureMachinePool.Spec.Location,
SubscriptionID: m.SubscriptionID(),
VMSSExtensionSpecs: m.VMSSExtensionSpecs(),
HasReplicasExternallyManaged: m.HasReplicasExternallyManaged(ctx),
ClusterName: m.ClusterName(),
AdditionalTags: m.AzureMachinePool.Spec.AdditionalTags,
SKU: m.cache.VMSKU,
VMImage: m.cache.VMImage,
BootstrapData: m.cache.BootstrapData,
ShouldPatchCustomData: shouldPatchCustomData,
}
}

Expand Down Expand Up @@ -614,11 +659,8 @@ func (m *MachinePoolScope) GetBootstrapData(ctx context.Context) (string, error)
}

// calculateBootstrapDataHash calculates the sha256 hash of the bootstrap data.
func (m *MachinePoolScope) calculateBootstrapDataHash(ctx context.Context) (string, error) {
bootstrapData, err := m.GetBootstrapData(ctx)
if err != nil {
return "", err
}
func (m *MachinePoolScope) calculateBootstrapDataHash(_ context.Context) (string, error) {
bootstrapData := m.cache.BootstrapData
h := sha256.New()
n, err := io.WriteString(h, bootstrapData)
if err != nil || n == 0 {
Expand Down
16 changes: 13 additions & 3 deletions azure/services/roleassignments/roleassignments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ type Service struct {
Scope RoleAssignmentScope
virtualMachinesGetter async.Getter
async.Reconciler
virtualMachineScaleSetClient scalesets.Client
virtualMachineScaleSetGetter async.Getter
}

// New creates a new service.
Expand All @@ -56,7 +56,7 @@ func New(scope RoleAssignmentScope) *Service {
return &Service{
Scope: scope,
virtualMachinesGetter: virtualmachines.NewClient(scope),
virtualMachineScaleSetClient: scalesets.NewClient(scope),
virtualMachineScaleSetGetter: scalesets.NewClient(scope),
Reconciler: async.New(scope, client, client),
}
}
Expand Down Expand Up @@ -141,10 +141,20 @@ func (s *Service) getVMSSPrincipalID(ctx context.Context) (*string, error) {
ctx, log, done := tele.StartSpanWithLogger(ctx, "roleassignments.Service.getVMPrincipalID")
defer done()
log.V(2).Info("fetching principal ID for VMSS")
resultVMSS, err := s.virtualMachineScaleSetClient.Get(ctx, s.Scope.ResourceGroup(), s.Scope.Name())
spec := &scalesets.ScaleSetSpec{
Name: s.Scope.Name(),
ResourceGroup: s.Scope.ResourceGroup(),
}

resultVMSSIface, err := s.virtualMachineScaleSetGetter.Get(ctx, spec)
if err != nil {
return nil, errors.Wrap(err, "failed to get principal ID for VMSS")
}
Comment on lines 150 to 152
Copy link
Copy Markdown
Member

@nawazkh nawazkh Aug 11, 2023

Choose a reason for hiding this comment

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

Should this be updated to below?

Suggested change
if err != nil {
return nil, errors.Wrap(err, "failed to get principal ID for VMSS")
}
if err != nil {
return nil, errors.Wrap(err, "failed to get resultVMSSIface")
}

Copy link
Copy Markdown
Contributor Author

@Jont828 Jont828 Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On second thought, I think we want to leave it as is. Before, we were giving the error when we failed to get the VM, which we're still doing here. The error err should explain that we failed the Get() call while the wrapper message gives context as to why we needed to fetch the VMSS to begin with.

resultVMSS, ok := resultVMSSIface.(compute.VirtualMachineScaleSet)
if !ok {
return nil, errors.Errorf("%T is not a compute.VirtualMachineScaleSet", resultVMSSIface)
}

return resultVMSS.Identity.PrincipalID, nil
}

Expand Down
13 changes: 9 additions & 4 deletions azure/services/roleassignments/roleassignments_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"sigs.k8s.io/cluster-api-provider-azure/azure"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/roleassignments/mock_roleassignments"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesets"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesets/mock_scalesets"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines"
gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock"
Expand All @@ -55,6 +56,10 @@ var (

emptyRoleAssignmentSpec = RoleAssignmentSpec{}
fakeRoleAssignmentSpecs = []azure.ResourceSpecGetter{&fakeRoleAssignment1, &fakeRoleAssignment2, &emptyRoleAssignmentSpec}
fakeVMSSSpec = scalesets.ScaleSetSpec{
Name: "test-vmss",
ResourceGroup: "my-rg",
}
)

func TestReconcileRoleAssignmentsVM(t *testing.T) {
Expand Down Expand Up @@ -169,7 +174,7 @@ func TestReconcileRoleAssignmentsVMSS(t *testing.T) {
s.RoleAssignmentResourceType().Return(azure.VirtualMachineScaleSet)
s.ResourceGroup().Return("my-rg")
s.Name().Return("test-vmss")
mvmss.Get(gomockinternal.AContext(), "my-rg", "test-vmss").Return(compute.VirtualMachineScaleSet{
mvmss.Get(gomockinternal.AContext(), &fakeVMSSSpec).Return(compute.VirtualMachineScaleSet{
Identity: &compute.VirtualMachineScaleSetIdentity{
PrincipalID: &fakePrincipalID,
},
Expand All @@ -187,7 +192,7 @@ func TestReconcileRoleAssignmentsVMSS(t *testing.T) {
s.ResourceGroup().Return("my-rg")
s.Name().Return("test-vmss")
s.HasSystemAssignedIdentity().Return(true)
mvmss.Get(gomockinternal.AContext(), "my-rg", "test-vmss").Return(compute.VirtualMachineScaleSet{},
mvmss.Get(gomockinternal.AContext(), &fakeVMSSSpec).Return(compute.VirtualMachineScaleSet{},
autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: http.StatusInternalServerError}, "Internal Server Error"))
},
},
Expand All @@ -202,7 +207,7 @@ func TestReconcileRoleAssignmentsVMSS(t *testing.T) {
s.RoleAssignmentResourceType().Return(azure.VirtualMachineScaleSet)
s.ResourceGroup().Return("my-rg")
s.Name().Return("test-vmss")
mvmss.Get(gomockinternal.AContext(), "my-rg", "test-vmss").Return(compute.VirtualMachineScaleSet{
mvmss.Get(gomockinternal.AContext(), &fakeVMSSSpec).Return(compute.VirtualMachineScaleSet{
Identity: &compute.VirtualMachineScaleSetIdentity{
PrincipalID: &fakePrincipalID,
},
Expand All @@ -229,7 +234,7 @@ func TestReconcileRoleAssignmentsVMSS(t *testing.T) {
s := &Service{
Scope: scopeMock,
Reconciler: asyncMock,
virtualMachineScaleSetClient: vmMock,
virtualMachineScaleSetGetter: vmMock,
}

err := s.Reconcile(context.TODO())
Expand Down
Loading