Skip to content

Commit

Permalink
Merge pull request #759 from cpanato/GH-757
Browse files Browse the repository at this point in the history 
💎 cloud: Refactor Disks scope to interface
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jul 7, 2020
2 parents e45d6c5 + e06c352 commit 9bf0121
Show file tree
Hide file tree
Showing 10 changed files with 418 additions and 163 deletions.
1 change: 1 addition & 0 deletions cloud/interfaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package azure

import (
"context"

"github.com/Azure/go-autorest/autorest"
infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha3"
)
Expand Down
9 changes: 9 additions & 0 deletions cloud/scope/machine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package scope
import (
"context"
"encoding/base64"

"github.com/Azure/go-autorest/autorest"
"github.com/go-logr/logr"
"github.com/pkg/errors"
Expand Down Expand Up @@ -124,6 +125,14 @@ func (m *MachineScope) NICSpecs() []azure.NICSpec {
return []azure.NICSpec{spec}
}

// DiskSpecs returns the public IP specs.
func (m *MachineScope) DiskSpecs() []azure.DiskSpec {
spec := azure.DiskSpec{
Name: azure.GenerateOSDiskName(m.Name()),
}
return []azure.DiskSpec{spec}
}

// Location returns the AzureCluster location.
func (m *MachineScope) Location() string {
return m.ClusterScope.Location()
Expand Down
39 changes: 16 additions & 23 deletions cloud/services/disks/disks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,37 +19,30 @@ package disks
import (
"context"

"github.com/pkg/errors"
"k8s.io/klog"
azure "sigs.k8s.io/cluster-api-provider-azure/cloud"
)

// Spec specification for disk
type Spec struct {
Name string
}
"github.com/pkg/errors"
)

// Reconcile on disk is currently no-op. OS disks should only be deleted and will create with the VM automatically.
func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
func (s *Service) Reconcile(ctx context.Context) error {
return nil
}

// Delete deletes the disk associated with a VM.
func (s *Service) Delete(ctx context.Context, spec interface{}) error {
diskSpec, ok := spec.(*Spec)
if !ok {
return errors.New("invalid disk specification")
}
klog.V(2).Infof("deleting disk %s", diskSpec.Name)
err := s.Client.Delete(ctx, s.Scope.ResourceGroup(), diskSpec.Name)
if err != nil && azure.ResourceNotFound(err) {
// already deleted
return nil
func (s *Service) Delete(ctx context.Context) error {
for _, diskSpec := range s.Scope.DiskSpecs() {
s.Scope.V(2).Info("deleting disk", "disk", diskSpec.Name)
err := s.Client.Delete(ctx, s.Scope.ResourceGroup(), diskSpec.Name)
if err != nil && azure.ResourceNotFound(err) {
// already deleted
return nil
}
if err != nil {
return errors.Wrapf(err, "failed to delete disk %s in resource group %s", diskSpec.Name, s.Scope.ResourceGroup())
}

s.Scope.V(2).Info("successfully deleted disk", "disk", diskSpec.Name)
}
if err != nil {
return errors.Wrapf(err, "failed to delete disk %s in resource group %s", diskSpec.Name, s.Scope.ResourceGroup())
}

klog.V(2).Infof("successfully deleted disk %s", diskSpec.Name)
return nil
}
153 changes: 47 additions & 106 deletions cloud/services/disks/disks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,110 +21,72 @@ import (
"net/http"
"testing"

"github.com/Azure/go-autorest/autorest"
. "github.com/onsi/gomega"
azure "sigs.k8s.io/cluster-api-provider-azure/cloud"
"sigs.k8s.io/cluster-api-provider-azure/cloud/services/disks/mock_disks"

"github.com/Azure/go-autorest/autorest"
"github.com/golang/mock/gomock"

network "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2019-06-01/network"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes/scheme"
infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha3"
"sigs.k8s.io/cluster-api-provider-azure/cloud/scope"
"k8s.io/klog/klogr"
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
)

func init() {
clusterv1.AddToScheme(scheme.Scheme)
}

const (
subscriptionID = "123"
)

func TestInvalidDiskSpec(t *testing.T) {
g := NewWithT(t)

mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

disksMock := mock_disks.NewMockClient(mockCtrl)

cluster := &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
}

client := fake.NewFakeClientWithScheme(scheme.Scheme, cluster)

clusterScope, err := scope.NewClusterScope(scope.ClusterScopeParams{
AzureClients: scope.AzureClients{
Authorizer: autorest.NullAuthorizer{},
},
Client: client,
Cluster: cluster,
AzureCluster: &infrav1.AzureCluster{
Spec: infrav1.AzureClusterSpec{
Location: "test-location",
ResourceGroup: "my-rg",
SubscriptionID: subscriptionID,
NetworkSpec: infrav1.NetworkSpec{
Vnet: infrav1.VnetSpec{Name: "my-vnet", ResourceGroup: "my-rg"},
},
},
},
})
g.Expect(err).NotTo(HaveOccurred())

s := &Service{
Scope: clusterScope,
Client: disksMock,
}

// Wrong Spec
wrongSpec := &network.PublicIPAddress{}

err = s.Delete(context.TODO(), &wrongSpec)
g.Expect(err).To(HaveOccurred())
g.Expect(err).To(MatchError("invalid disk specification"))
_ = clusterv1.AddToScheme(scheme.Scheme)
}

func TestDeleteDisk(t *testing.T) {
testcases := []struct {
name string
disksSpec Spec
expectedError string
expect func(m *mock_disks.MockClientMockRecorder)
expect func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder)
}{
{
name: "delete the disk",
disksSpec: Spec{
Name: "my-disk",
},
name: "delete the disk",
expectedError: "",
expect: func(m *mock_disks.MockClientMockRecorder) {
m.Delete(context.TODO(), "my-rg", "my-disk")
expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder) {
s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New())
s.DiskSpecs().Return([]azure.DiskSpec{
{
Name: "my-disk-1",
},
{
Name: "honk-disk",
},
})
s.ResourceGroup().AnyTimes().Return("my-rg")
m.Delete(context.TODO(), "my-rg", "my-disk-1")
m.Delete(context.TODO(), "my-rg", "honk-disk")
},
},
{
name: "disk already deleted",
disksSpec: Spec{
Name: "my-disk",
},
name: "disk already deleted",
expectedError: "",
expect: func(m *mock_disks.MockClientMockRecorder) {
m.Delete(context.TODO(), "my-rg", "my-disk").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found"))
expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder) {
s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New())
s.DiskSpecs().Return([]azure.DiskSpec{
{
Name: "my-disk-1",
},
})
s.ResourceGroup().AnyTimes().Return("my-rg")
m.Delete(context.TODO(), "my-rg", "my-disk-1").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found"))
},
},
{
name: "error while trying to delete the disk",
disksSpec: Spec{
Name: "my-disk",
},
expectedError: "failed to delete disk my-disk in resource group my-rg: #: Internal Server Error: StatusCode=500",
expect: func(m *mock_disks.MockClientMockRecorder) {
m.Delete(context.TODO(), "my-rg", "my-disk").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error"))
name: "error while trying to delete the disk",
expectedError: "failed to delete disk my-disk-1 in resource group my-rg: #: Internal Server Error: StatusCode=500",
expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder) {
s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New())
s.DiskSpecs().Return([]azure.DiskSpec{
{
Name: "my-disk-1",
},
})
s.ResourceGroup().AnyTimes().Return("my-rg")
m.Delete(context.TODO(), "my-rg", "my-disk-1").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error"))
},
},
}
Expand All @@ -133,41 +95,20 @@ func TestDeleteDisk(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)

t.Parallel()
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
scopeMock := mock_disks.NewMockDiskScope(mockCtrl)
clientMock := mock_disks.NewMockClient(mockCtrl)

disksMock := mock_disks.NewMockClient(mockCtrl)

cluster := &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
}

client := fake.NewFakeClientWithScheme(scheme.Scheme, cluster)

tc.expect(disksMock.EXPECT())

clusterScope, err := scope.NewClusterScope(scope.ClusterScopeParams{
AzureClients: scope.AzureClients{
Authorizer: autorest.NullAuthorizer{},
},
Client: client,
Cluster: cluster,
AzureCluster: &infrav1.AzureCluster{
Spec: infrav1.AzureClusterSpec{
Location: "test-location",
ResourceGroup: "my-rg",
SubscriptionID: subscriptionID,
},
},
})
g.Expect(err).NotTo(HaveOccurred())
tc.expect(scopeMock.EXPECT(), clientMock.EXPECT())

s := &Service{
Scope: clusterScope,
Client: disksMock,
Scope: scopeMock,
Client: clientMock,
}

err = s.Delete(context.TODO(), &tc.disksSpec)
err := s.Delete(context.TODO())
if tc.expectedError != "" {
g.Expect(err).To(HaveOccurred())
g.Expect(err).To(MatchError(tc.expectedError))
Expand Down
64 changes: 64 additions & 0 deletions cloud/services/disks/mock_disks/client_mock.go
View file

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

Loading

0 comments on commit 9bf0121

Please sign in to comment.