Skip to content
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

Make vnets reconcile/delete async #1921

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Make vnets reconcile/delete async #1921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a03b9e2
Make vnets reconcile/delete async
Dec 14, 2021
021814e
Add Getter interface for getting resources in other services
Jan 28, 2022
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
16 changes: 11 additions & 5 deletions azure/scope/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import (
"sigs.k8s.io/cluster-api-provider-azure/azure/services/loadbalancers"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/natgateways"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/routetables"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualnetworks"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/vnetpeerings"
"sigs.k8s.io/cluster-api-provider-azure/util/futures"
"sigs.k8s.io/cluster-api-provider-azure/util/tele"
Expand Down Expand Up @@ -362,11 +363,14 @@ func (s *ClusterScope) VnetPeeringSpecs() []azure.ResourceSpecGetter {
}

// VNetSpec returns the virtual network spec.
func (s *ClusterScope) VNetSpec() azure.VNetSpec {
return azure.VNetSpec{
ResourceGroup: s.Vnet().ResourceGroup,
Name: s.Vnet().Name,
CIDRs: s.Vnet().CIDRBlocks,
func (s *ClusterScope) VNetSpec() azure.ResourceSpecGetter {
return &virtualnetworks.VNetSpec{
ResourceGroup: s.Vnet().ResourceGroup,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that we have not copied Peerings from the old spec. Is this a conscious decision? we were setting it previously here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, what was happening before was that we were doing a full deep copy into the vnet stored in AzureCluster.Spec which meant we had to include every single field that had been set before otherwise we would risk overwriting it. This is not good because if a new feature gets added like vnet peerings there could be a bug where the vnet write erases the user configuration (and in fact we did run into that issue during vnet peering implementation). This is hopefully simplifying the code while keeping it functionally equivalent. We only set the vnet, ID, tags and cidrs from the existing vnet while keeping the rest of the VnetSpec the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay, makes sense. Thanks for the explanation!

Name: s.Vnet().Name,
CIDRs: s.Vnet().CIDRBlocks,
Location: s.Location(),
ClusterName: s.ClusterName(),
AdditionalTags: s.AdditionalTags(),
}
}

Expand Down Expand Up @@ -662,6 +666,7 @@ func (s *ClusterScope) PatchObject(ctx context.Context) error {
infrav1.NATGatewaysReadyCondition,
infrav1.LoadBalancersReadyCondition,
infrav1.BastionHostReadyCondition,
infrav1.VNetReadyCondition,
),
)

Expand All @@ -678,6 +683,7 @@ func (s *ClusterScope) PatchObject(ctx context.Context) error {
infrav1.NATGatewaysReadyCondition,
infrav1.LoadBalancersReadyCondition,
infrav1.BastionHostReadyCondition,
infrav1.VNetReadyCondition,
}})
}

Expand Down
14 changes: 9 additions & 5 deletions azure/scope/managedcontrolplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
"sigs.k8s.io/cluster-api-provider-azure/azure"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/groups"
"sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualnetworks"
infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1"
"sigs.k8s.io/cluster-api-provider-azure/util/futures"
"sigs.k8s.io/cluster-api-provider-azure/util/tele"
Expand Down Expand Up @@ -226,11 +227,14 @@ func (s *ManagedControlPlaneScope) GroupSpec() azure.ResourceSpecGetter {
}

// VNetSpec returns the virtual network spec.
func (s *ManagedControlPlaneScope) VNetSpec() azure.VNetSpec {
return azure.VNetSpec{
ResourceGroup: s.Vnet().ResourceGroup,
Name: s.Vnet().Name,
CIDRs: s.Vnet().CIDRBlocks,
func (s *ManagedControlPlaneScope) VNetSpec() azure.ResourceSpecGetter {
return &virtualnetworks.VNetSpec{
ResourceGroup: s.Vnet().ResourceGroup,
Name: s.Vnet().Name,
CIDRs: s.Vnet().CIDRBlocks,
Location: s.Location(),
ClusterName: s.ClusterName(),
AdditionalTags: s.AdditionalTags(),
}
}

Expand Down
7 changes: 6 additions & 1 deletion azure/services/async/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,16 @@ type FutureHandler interface {
Result(ctx context.Context, future azureautorest.FutureAPI, futureType string) (result interface{}, err error)
}

// Getter is an interface that can get a resource.
type Getter interface {
Get(ctx context.Context, spec azure.ResourceSpecGetter) (result interface{}, err error)
}

// Creator is a client that can create or update a resource asynchronously.
type Creator interface {
FutureHandler
Getter
CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, parameters interface{}) (result interface{}, future azureautorest.FutureAPI, err error)
Get(ctx context.Context, spec azure.ResourceSpecGetter) (result interface{}, err error)
}

// Deleter is a client that can delete a resource asynchronously.
Expand Down
38 changes: 38 additions & 0 deletions azure/services/async/mock_async/async_mock.go
View file
Open in desktop

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

6 changes: 3 additions & 3 deletions azure/services/availabilitysets/availabilitysets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type AvailabilitySetScope interface {
// Service provides operations on Azure resources.
type Service struct {
Scope AvailabilitySetScope
Client
async.Getter
async.Reconciler
resourceSKUCache *resourceskus.Cache
}
Expand All @@ -51,7 +51,7 @@ func New(scope AvailabilitySetScope, skuCache *resourceskus.Cache) *Service {
client := NewClient(scope)
return &Service{
Scope: scope,
Client: client,
Getter: client,
resourceSKUCache: skuCache,
Reconciler: async.New(scope, client, client),
}
Expand Down Expand Up @@ -88,7 +88,7 @@ func (s *Service) Delete(ctx context.Context) error {
if setSpec := s.Scope.AvailabilitySetSpec(); setSpec == nil {
log.V(2).Info("skip deletion when no availability set spec is found")
} else {
existingSet, err := s.Client.Get(ctx, setSpec)
existingSet, err := s.Get(ctx, setSpec)
if err != nil {
if !azure.ResourceNotFound(err) {
resultingErr = errors.Wrapf(err, "failed to get availability set %s in resource group %s", setSpec.ResourceName(), setSpec.ResourceGroupName())
Expand Down
24 changes: 12 additions & 12 deletions azure/services/availabilitysets/availabilitysets_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,12 @@ func TestDeleteAvailabilitySets(t *testing.T) {
testcases := []struct {
name string
expectedError string
expect func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder)
expect func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder)
}{
{
name: "deletes availability set",
expectedError: "",
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
s.AvailabilitySetSpec().Return(&fakeSetSpec)
gomock.InOrder(
m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(compute.AvailabilitySet{}, nil),
Expand All @@ -166,15 +166,15 @@ func TestDeleteAvailabilitySets(t *testing.T) {
{
name: "noop if AvailabilitySetSpec returns nil",
expectedError: "",
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
s.AvailabilitySetSpec().Return(nil)
s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil)
},
},
{
name: "delete proceeds with missing required value in availability set spec",
expectedError: "",
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
s.AvailabilitySetSpec().Return(&fakeSetSpecMissing)
gomock.InOrder(
m.Get(gomockinternal.AContext(), &fakeSetSpecMissing).Return(compute.AvailabilitySet{}, nil),
Expand All @@ -186,7 +186,7 @@ func TestDeleteAvailabilitySets(t *testing.T) {
{
name: "noop if availability set has vms",
expectedError: "",
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
s.AvailabilitySetSpec().Return(&fakeSetSpec)
gomock.InOrder(
m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(fakeSetWithVMs, nil),
Expand All @@ -197,7 +197,7 @@ func TestDeleteAvailabilitySets(t *testing.T) {
{
name: "availability set not found",
expectedError: "",
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
s.AvailabilitySetSpec().Return(&fakeSetSpec)
gomock.InOrder(
m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(nil, notFoundError),
Expand All @@ -208,7 +208,7 @@ func TestDeleteAvailabilitySets(t *testing.T) {
{
name: "error in getting availability set",
expectedError: "failed to get availability set test-as in resource group test-rg: #: Internal Server Error: StatusCode=500",
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
s.AvailabilitySetSpec().Return(&fakeSetSpec)
gomock.InOrder(
m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(nil, internalError),
Expand All @@ -219,7 +219,7 @@ func TestDeleteAvailabilitySets(t *testing.T) {
{
name: "availability set get result is not an availability set",
expectedError: "string is not a compute.AvailabilitySet",
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
s.AvailabilitySetSpec().Return(&fakeSetSpec)
gomock.InOrder(
m.Get(gomockinternal.AContext(), &fakeSetSpec).Return("not an availability set", nil),
Expand All @@ -230,7 +230,7 @@ func TestDeleteAvailabilitySets(t *testing.T) {
{
name: "error in deleting availability set",
expectedError: "#: Internal Server Error: StatusCode=500",
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_async.MockGetterMockRecorder, r *mock_async.MockReconcilerMockRecorder) {
s.AvailabilitySetSpec().Return(&fakeSetSpec)
gomock.InOrder(
m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(compute.AvailabilitySet{}, nil),
Expand All @@ -249,14 +249,14 @@ func TestDeleteAvailabilitySets(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
scopeMock := mock_availabilitysets.NewMockAvailabilitySetScope(mockCtrl)
clientMock := mock_availabilitysets.NewMockClient(mockCtrl)
getterMock := mock_async.NewMockGetter(mockCtrl)
asyncMock := mock_async.NewMockReconciler(mockCtrl)

tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), asyncMock.EXPECT())
tc.expect(scopeMock.EXPECT(), getterMock.EXPECT(), asyncMock.EXPECT())

s := &Service{
Scope: scopeMock,
Client: clientMock,
Getter: getterMock,
Reconciler: asyncMock,
}

Expand Down
11 changes: 0 additions & 11 deletions azure/services/availabilitysets/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,11 @@ import (
"sigs.k8s.io/cluster-api-provider-azure/util/tele"
)

// Client wraps go-sdk.
type Client interface {
Get(ctx context.Context, spec azure.ResourceSpecGetter) (result interface{}, err error)
CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, parameters interface{}) (result interface{}, future azureautorest.FutureAPI, err error)
DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter) (future azureautorest.FutureAPI, err error)
IsDone(ctx context.Context, future azureautorest.FutureAPI) (isDone bool, err error)
Result(ctx context.Context, future azureautorest.FutureAPI, futureType string) (result interface{}, err error)
}

// AzureClient contains the Azure go-sdk Client.
type AzureClient struct {
availabilitySets compute.AvailabilitySetsClient
}

var _ Client = (*AzureClient)(nil)

// NewClient creates a new Resource SKUs Client from subscription ID.
func NewClient(auth azure.Authorizer) *AzureClient {
return &AzureClient{
Expand Down
108 changes: 0 additions & 108 deletions azure/services/availabilitysets/mock_availabilitysets/client_mock.go
View file
Open in desktop

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

Loading