Skip to content

Commit

Permalink
Merge pull request #1013 from shysank/refactor/1001
Browse files Browse the repository at this point in the history 
Cleanup disks service
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Oct 29, 2020
2 parents 2bcdc7b + ee66de2 commit 3c5cb5b
Show file tree
Hide file tree
Showing 8 changed files with 87 additions and 122 deletions.
14 changes: 7 additions & 7 deletions cloud/services/disks/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,21 @@ import (
)

// Client wraps go-sdk
type Client interface {
type client interface {
Delete(context.Context, string, string) error
}

// AzureClient contains the Azure go-sdk Client
type AzureClient struct {
type azureClient struct {
disks compute.DisksClient
}

var _ Client = &AzureClient{}
var _ client = (*azureClient)(nil)

// NewClient creates a new VM client from subscription ID.
func NewClient(auth azure.Authorizer) *AzureClient {
// newClient creates a new VM client from subscription ID.
func newClient(auth azure.Authorizer) *azureClient {
c := newDisksClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer())
return &AzureClient{c}
return &azureClient{c}
}

// newDisksClient creates a new disks client from subscription ID.
Expand All @@ -51,7 +51,7 @@ func newDisksClient(subscriptionID string, baseURI string, authorizer autorest.A
}

// Delete removes the disk client
func (ac *AzureClient) Delete(ctx context.Context, resourceGroupName, name string) error {
func (ac *azureClient) Delete(ctx context.Context, resourceGroupName, name string) error {
future, err := ac.disks.Delete(ctx, resourceGroupName, name)
if err != nil {
return err
Expand Down
28 changes: 25 additions & 3 deletions cloud/services/disks/disks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,33 @@ package disks
import (
"context"

azure "sigs.k8s.io/cluster-api-provider-azure/cloud"

"github.com/go-logr/logr"
"github.com/pkg/errors"

azure "sigs.k8s.io/cluster-api-provider-azure/cloud"
)

// DiskScope defines the scope interface for a disk service.
type DiskScope interface {
logr.Logger
azure.ClusterDescriber
DiskSpecs() []azure.DiskSpec
}

// Service provides operations on azure resources
type Service struct {
Scope DiskScope
client
}

// New creates a new disks service.
func New(scope DiskScope) *Service {
return &Service{
Scope: scope,
client: newClient(scope),
}
}

// 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) error {
return nil
Expand All @@ -33,7 +55,7 @@ func (s *Service) Reconcile(ctx context.Context) error {
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)
err := s.client.Delete(ctx, s.Scope.ResourceGroup(), diskSpec.Name)
if err != nil && azure.ResourceNotFound(err) {
// already deleted
continue
Expand Down
90 changes: 38 additions & 52 deletions cloud/services/disks/disks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,12 @@ func TestDeleteDisk(t *testing.T) {
testcases := []struct {
name string
expectedError string
expect func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder)
expect func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder)
}{
{
name: "delete the disk",
expectedError: "",
expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder) {
expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) {
s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New())
s.DiskSpecs().Return([]azure.DiskSpec{
{
Expand All @@ -64,7 +64,7 @@ func TestDeleteDisk(t *testing.T) {
{
name: "disk already deleted",
expectedError: "",
expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder) {
expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) {
s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New())
s.DiskSpecs().Return([]azure.DiskSpec{
{
Expand All @@ -82,7 +82,7 @@ func TestDeleteDisk(t *testing.T) {
{
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) {
expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) {
s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New())
s.DiskSpecs().Return([]azure.DiskSpec{
{
Expand All @@ -107,13 +107,13 @@ func TestDeleteDisk(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()
scopeMock := mock_disks.NewMockDiskScope(mockCtrl)
clientMock := mock_disks.NewMockClient(mockCtrl)
clientMock := mock_disks.NewMockclient(mockCtrl)

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

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

err := s.Delete(context.TODO())
Expand All @@ -129,43 +129,24 @@ func TestDeleteDisk(t *testing.T) {

func TestDiskSpecs(t *testing.T) {
testcases := []struct {
name string
azureMachine *infrav1.AzureMachine
expectedDisks []azure.DiskSpec
name string
azureMachineModifyFunc func(*infrav1.AzureMachine)
expectedDisks []azure.DiskSpec
}{
{
name: "only os disk",
azureMachine: &infrav1.AzureMachine{
ObjectMeta: metav1.ObjectMeta{
Name: "my-azure-machine",
},
Spec: infrav1.AzureMachineSpec{
OSDisk: infrav1.OSDisk{
DiskSizeGB: 30,
OSType: "Linux",
},
},
},
name: "only os disk",
azureMachineModifyFunc: func(m *infrav1.AzureMachine) {},
expectedDisks: []azure.DiskSpec{
{
Name: "my-azure-machine_OSDisk",
},
},
}, {
name: "os and data disks",
azureMachine: &infrav1.AzureMachine{
ObjectMeta: metav1.ObjectMeta{
Name: "my-azure-machine",
},
Spec: infrav1.AzureMachineSpec{
OSDisk: infrav1.OSDisk{
DiskSizeGB: 30,
OSType: "Linux",
},
DataDisks: []infrav1.DataDisk{{
NameSuffix: "etcddisk",
}},
},
azureMachineModifyFunc: func(m *infrav1.AzureMachine) {
m.Spec.DataDisks = []infrav1.DataDisk{{
NameSuffix: "etcddisk",
}}
},
expectedDisks: []azure.DiskSpec{
{
Expand All @@ -177,23 +158,14 @@ func TestDiskSpecs(t *testing.T) {
},
}, {
name: "os and multiple data disks",
azureMachine: &infrav1.AzureMachine{
ObjectMeta: metav1.ObjectMeta{
Name: "my-azure-machine",
},
Spec: infrav1.AzureMachineSpec{
OSDisk: infrav1.OSDisk{
DiskSizeGB: 30,
OSType: "Linux",
azureMachineModifyFunc: func(m *infrav1.AzureMachine) {
m.Spec.DataDisks = []infrav1.DataDisk{
{
NameSuffix: "etcddisk",
},
DataDisks: []infrav1.DataDisk{
{
NameSuffix: "etcddisk",
},
{
NameSuffix: "otherdisk",
}},
},
{
NameSuffix: "otherdisk",
}}
},
expectedDisks: []azure.DiskSpec{
{
Expand Down Expand Up @@ -231,11 +203,25 @@ func TestDiskSpecs(t *testing.T) {
Name: "my-machine",
},
}

azureMachine := &infrav1.AzureMachine{
ObjectMeta: metav1.ObjectMeta{
Name: "my-azure-machine",
},
Spec: infrav1.AzureMachineSpec{
OSDisk: infrav1.OSDisk{
DiskSizeGB: 30,
OSType: "Linux",
},
},
}
tc.azureMachineModifyFunc(azureMachine)

initObjects := []runtime.Object{
cluster,
machine,
azureCluster,
tc.azureMachine,
azureMachine,
}
client := fake.NewFakeClientWithScheme(scheme, initObjects...)
clusterScope, err := scope.NewClusterScope(scope.ClusterScopeParams{
Expand All @@ -251,7 +237,7 @@ func TestDiskSpecs(t *testing.T) {
Client: client,
ClusterScope: clusterScope,
Machine: machine,
AzureMachine: tc.azureMachine,
AzureMachine: azureMachine,
})
g.Expect(err).NotTo(HaveOccurred())

Expand Down
28 changes: 14 additions & 14 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.

2 changes: 1 addition & 1 deletion cloud/services/disks/mock_disks/disks_mock.go
View file

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

2 changes: 1 addition & 1 deletion cloud/services/disks/mock_disks/doc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ limitations under the License.

// Run go generate to regenerate this mock.
//go:generate ../../../../hack/tools/bin/mockgen -destination client_mock.go -package mock_disks -source ../client.go Client
//go:generate ../../../../hack/tools/bin/mockgen -destination disks_mock.go -package mock_disks -source ../service.go DiskScope
//go:generate ../../../../hack/tools/bin/mockgen -destination disks_mock.go -package mock_disks -source ../disks.go DiskScope
//go:generate /usr/bin/env bash -c "cat ../../../../hack/boilerplate/boilerplate.generatego.txt client_mock.go > _client_mock.go && mv _client_mock.go client_mock.go"
//go:generate /usr/bin/env bash -c "cat ../../../../hack/boilerplate/boilerplate.generatego.txt disks_mock.go > _disks_mock.go && mv _disks_mock.go disks_mock.go"
package mock_disks //nolint
43 changes: 0 additions & 43 deletions cloud/services/disks/service.go
View file

This file was deleted.

2 changes: 1 addition & 1 deletion controllers/azuremachine_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func newAzureMachineService(machineScope *scope.MachineScope, clusterScope *scop
networkInterfacesSvc: networkinterfaces.NewService(machineScope, cache),
virtualMachinesSvc: virtualmachines.NewService(machineScope, cache),
roleAssignmentsSvc: roleassignments.NewService(machineScope),
disksSvc: disks.NewService(machineScope),
disksSvc: disks.New(machineScope),
publicIPsSvc: publicips.NewService(machineScope),
tagsSvc: tags.NewService(machineScope),
skuCache: cache,
Expand Down

0 comments on commit 3c5cb5b

Please sign in to comment.