Skip to content

Commit ec7284a

Browse files
mrogmrog
committed
Updates per PR comments
1 parent 82d41ad commit ec7284a

File tree

4 files changed

+66
-53
lines changed

4 files changed

+66
-53
lines changed

‎pkg/cloud/affinity_groups_test.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424
"github.com/golang/mock/gomock"
2525
. "github.com/onsi/ginkgo"
2626
. "github.com/onsi/gomega"
27-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2827
capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"
2928
)
3029

@@ -49,9 +48,9 @@ var _ = Describe("AffinityGroup Unit Tests", func() {
4948
fakeAG = &cloud.AffinityGroup{
5049
Name: "FakeAffinityGroup",
5150
Type: cloud.AffinityGroupType}
52-
cluster = &infrav1.CloudStackCluster{
53-
Spec: infrav1.CloudStackClusterSpec{Zone: "Zone1", Network: "SharedGuestNet1"},
54-
ObjectMeta: metav1.ObjectMeta{UID: "0"}}
51+
cluster = &infrav1.CloudStackCluster{Spec: infrav1.CloudStackClusterSpec{
52+
Zone: "Zone1", Network: "SharedGuestNet1"}}
53+
cluster.ObjectMeta.SetUID("0")
5554
machine = &infrav1.CloudStackMachine{Spec: infrav1.CloudStackMachineSpec{
5655
Offering: "Medium Instance", Template: "Ubuntu20"}}
5756
machine.ObjectMeta.SetName("rejoshed-affinity-group-test-vm")

‎pkg/cloud/cluster.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,5 +95,8 @@ func (c *client) GetOrCreateCluster(csCluster *infrav1.CloudStackCluster) (retEr
9595
}
9696

9797
func (c *client) DisposeClusterResources(csCluster *infrav1.CloudStackCluster) (retError error) {
98-
return c.UntagAndDestroyNetwork(csCluster)
98+
if err := c.RemoveClusterTagFromNetwork(csCluster); err != nil {
99+
return err
100+
}
101+
return c.DeleteNetworkIfNotInUse(csCluster)
99102
}

‎pkg/cloud/network.go

Lines changed: 45 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -63,33 +63,8 @@ func generateNetworkTagName(csCluster *infrav1.CloudStackCluster) string {
6363
}
6464

6565
func (c *client) GetOrCreateNetwork(csCluster *infrav1.CloudStackCluster) (retErr error) {
66-
// Tag the network so we can clean it up later
67-
tagNetwork := func(csCluster *infrav1.CloudStackCluster, addCreatedByTag bool) error {
68-
clusterTagName := generateNetworkTagName(csCluster)
69-
newTags := map[string]string{}
70-
71-
existingTags, err := c.GetNetworkTags(csCluster.Status.NetworkID)
72-
if err != nil {
73-
return err
74-
}
75-
76-
if existingTags[clusterTagName] == "" {
77-
newTags[clusterTagName] = "1"
78-
}
79-
80-
if addCreatedByTag {
81-
newTags[createdByCapcTagName] = "1"
82-
}
83-
84-
if len(newTags) > 0 {
85-
return c.AddNetworkTags(csCluster.Status.NetworkID, newTags)
86-
}
87-
88-
return nil
89-
}
90-
9166
if retErr = c.ResolveNetwork(csCluster); retErr == nil { // Found network.
92-
return tagNetwork(csCluster, false)
67+
return addClusterTags(c, csCluster, false)
9368
} else if !strings.Contains(retErr.Error(), "No match found") { // Some other error.
9469
return retErr
9570
} // Network not found.
@@ -115,32 +90,66 @@ func (c *client) GetOrCreateNetwork(csCluster *infrav1.CloudStackCluster) (retEr
11590
csCluster.Status.NetworkID = resp.Id
11691
csCluster.Status.NetworkType = resp.Type
11792

118-
return tagNetwork(csCluster, true)
93+
return addClusterTags(c, csCluster, true)
94+
}
95+
96+
func addClusterTags(c *client, csCluster *infrav1.CloudStackCluster, addCreatedByTag bool) error {
97+
clusterTagName := generateNetworkTagName(csCluster)
98+
newTags := map[string]string{}
99+
100+
existingTags, err := c.GetNetworkTags(csCluster.Status.NetworkID)
101+
if err != nil {
102+
return err
103+
}
104+
105+
if existingTags[clusterTagName] == "" {
106+
newTags[clusterTagName] = "1"
107+
}
108+
109+
if addCreatedByTag && existingTags[createdByCapcTagName] == "" {
110+
newTags[createdByCapcTagName] = "1"
111+
}
112+
113+
if len(newTags) > 0 {
114+
return c.AddNetworkTags(csCluster.Status.NetworkID, newTags)
115+
}
116+
117+
return nil
119118
}
120119

121-
func (c *client) UntagAndDestroyNetwork(csCluster *infrav1.CloudStackCluster) (retError error) {
120+
func (c *client) RemoveClusterTagFromNetwork(csCluster *infrav1.CloudStackCluster) (retError error) {
122121
tags, err := c.GetNetworkTags(csCluster.Status.NetworkID)
123122
if err != nil {
124123
return err
125124
}
126125

127-
var clusterTagCount int
128126
clusterTagName := generateNetworkTagName(csCluster)
129-
for tagName, tagValue := range tags {
127+
if tagValue := tags[clusterTagName]; tagValue != "" {
128+
if err = c.DeleteNetworkTags(csCluster.Status.NetworkID, map[string]string{clusterTagName: tagValue}); err != nil {
129+
return err
130+
}
131+
}
132+
133+
return nil
134+
}
135+
136+
func (c *client) DeleteNetworkIfNotInUse(csCluster *infrav1.CloudStackCluster) (retError error) {
137+
tags, err := c.GetNetworkTags(csCluster.Status.NetworkID)
138+
if err != nil {
139+
return err
140+
}
141+
142+
var clusterTagCount int
143+
for tagName := range tags {
130144
if strings.HasPrefix(tagName, clusterTagNamePrefix) {
131145
clusterTagCount++
132-
if tagName == clusterTagName {
133-
if err = c.DeleteNetworkTags(csCluster.Status.NetworkID, map[string]string{tagName: tagValue}); err != nil {
134-
return err
135-
}
136-
clusterTagCount--
137-
}
138146
}
139147
}
140148

141149
if clusterTagCount == 0 && tags[createdByCapcTagName] != "" {
142150
return c.DestroyNetwork(csCluster)
143151
}
152+
144153
return nil
145154
}
146155

‎pkg/cloud/network_test.go

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,16 @@ var _ = Describe("Network", func() {
8181
mockCtrl.Finish()
8282
})
8383

84+
// Sets expectations network tag creation. To be used by tests that get/create networks.
85+
expectNetworkTags := func(networkId string) {
86+
listTagsParams := &cloudstack.ListTagsParams{}
87+
createTagsParams := &cloudstack.CreateTagsParams{}
88+
rs.EXPECT().NewListTagsParams().Return(listTagsParams)
89+
rs.EXPECT().ListTags(listTagsParams).Return(&cloudstack.ListTagsResponse{}, nil)
90+
rs.EXPECT().NewCreateTagsParams([]string{networkId}, "network", gomock.Any()).Return(createTagsParams)
91+
rs.EXPECT().CreateTags(createTagsParams).Return(&cloudstack.CreateTagsResponse{}, nil)
92+
}
93+
8494
Context("for an existing network", func() {
8595
It("resolves network details in cluster status", func() {
8696
ns.EXPECT().GetNetworkID(fakeNetName).Return(fakeNetId, 1, nil)
@@ -93,21 +103,15 @@ var _ = Describe("Network", func() {
93103
It("does not call to create a new network via GetOrCreateNetwork", func() {
94104
ns.EXPECT().GetNetworkID(fakeNetName).Return(fakeNetId, 1, nil)
95105
ns.EXPECT().GetNetworkByID(fakeNetId).Return(&cloudstack.Network{Type: isolatedNetworkType}, 1, nil)
96-
rs.EXPECT().NewListTagsParams().Return(&cloudstack.ListTagsParams{})
97-
rs.EXPECT().ListTags(gomock.Any()).Return(&cloudstack.ListTagsResponse{}, nil)
98-
rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).Return(&cloudstack.CreateTagsParams{})
99-
rs.EXPECT().CreateTags(gomock.Any()).Return(&cloudstack.CreateTagsResponse{}, nil)
106+
expectNetworkTags(fakeNetId)
100107

101108
Ω(client.GetOrCreateNetwork(csCluster)).Should(Succeed())
102109
})
103110

104111
It("resolves network details with network ID instead of network name", func() {
105112
ns.EXPECT().GetNetworkID(gomock.Any()).Return("", -1, errors.New("No match found for blah."))
106113
ns.EXPECT().GetNetworkByID(fakeNetId).Return(&cloudstack.Network{Type: isolatedNetworkType}, 1, nil)
107-
rs.EXPECT().NewListTagsParams().Return(&cloudstack.ListTagsParams{})
108-
rs.EXPECT().ListTags(gomock.Any()).Return(&cloudstack.ListTagsResponse{}, nil)
109-
rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).Return(&cloudstack.CreateTagsParams{})
110-
rs.EXPECT().CreateTags(gomock.Any()).Return(&cloudstack.CreateTagsResponse{}, nil)
114+
expectNetworkTags(fakeNetId)
111115

112116
csCluster.Spec.Network = fakeNetId
113117
Ω(client.GetOrCreateNetwork(csCluster)).Should(Succeed())
@@ -122,10 +126,8 @@ var _ = Describe("Network", func() {
122126
ns.EXPECT().NewCreateNetworkParams(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
123127
Return(&cloudstack.CreateNetworkParams{})
124128
ns.EXPECT().CreateNetwork(gomock.Any()).Return(&cloudstack.CreateNetworkResponse{Id: netId}, nil)
125-
rs.EXPECT().NewListTagsParams().Return(&cloudstack.ListTagsParams{})
126-
rs.EXPECT().ListTags(gomock.Any()).Return(&cloudstack.ListTagsResponse{}, nil)
127-
rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).Return(&cloudstack.CreateTagsParams{})
128-
rs.EXPECT().CreateTags(gomock.Any()).Return(&cloudstack.CreateTagsResponse{}, nil)
129+
expectNetworkTags(netId)
130+
129131
Ω(client.GetOrCreateNetwork(csCluster)).Should(Succeed())
130132
})
131133
})

0 commit comments

Comments
 (0)