Skip to content

Commit 7047cdf

Browse files
mandremandre
committed
Move cleanup to the DeferCleanup function
As opposed to the AfterEach() function, the DeferCleanup() is not executed when the test is skipped. Previously, the cleanup in the webhook tests would wipe all the machinesets existing in the cluster on unsupported platforms (OpenStack) due to an unitialized selector, even when the tests were skipped.
1 parent ea989e2 commit 7047cdf

File tree

7 files changed

+105
-80
lines changed

7 files changed

+105
-80
lines changed

‎pkg/autoscaler/autoscaler.go

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -148,27 +148,28 @@ var _ = Describe("Autoscaler should", framework.LabelAutoscaler, Serial, func()
148148

149149
// Anything we create we must cleanup
150150
cleanupObjects = make(map[string]runtimeclient.Object)
151-
})
152151

153-
AfterEach(func() {
154-
var machineSets []*machinev1.MachineSet
152+
// Make sure to clean up the resources we created
153+
DeferCleanup(func() {
154+
var machineSets []*machinev1.MachineSet
155155

156-
for name, obj := range cleanupObjects {
157-
if machineSet, ok := obj.(*machinev1.MachineSet); ok {
158-
// Once we delete a MachineSet we should make sure that the
159-
// all of its machines are deleted as well.
160-
// Collect MachineSets to wait for.
161-
machineSets = append(machineSets, machineSet)
162-
}
156+
for name, obj := range cleanupObjects {
157+
if machineSet, ok := obj.(*machinev1.MachineSet); ok {
158+
// Once we delete a MachineSet we should make sure that the
159+
// all of its machines are deleted as well.
160+
// Collect MachineSets to wait for.
161+
machineSets = append(machineSets, machineSet)
162+
}
163163

164-
Expect(deleteObject(name, obj)).To(Succeed(), "Failed to delete object %v", name)
165-
}
164+
Expect(deleteObject(name, obj)).To(Succeed(), "Failed to delete object %v", name)
165+
}
166166

167-
if len(machineSets) > 0 {
168-
// Wait for all MachineSets and their Machines to be deleted.
169-
By("Waiting for MachineSets to be deleted...")
170-
framework.WaitForMachineSetsDeleted(ctx, client, machineSets...)
171-
}
167+
if len(machineSets) > 0 {
168+
// Wait for all MachineSets and their Machines to be deleted.
169+
By("Waiting for MachineSets to be deleted...")
170+
framework.WaitForMachineSetsDeleted(ctx, client, machineSets...)
171+
}
172+
})
172173
})
173174

174175
Context("use a ClusterAutoscaler that has 100 maximum total nodes count", framework.LabelPeriodic, func() {

‎pkg/infra/infra.go

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,20 +165,24 @@ var _ = Describe("Managed cluster should", framework.LabelMachines, func() {
165165
client, err = framework.LoadClient()
166166
Expect(err).ToNot(HaveOccurred(), "Controller-runtime client should be able to be created")
167167

168+
// Reset the machineSet between each test
169+
machineSet = nil
170+
171+
// Make sure to clean up the resources we created
172+
DeferCleanup(func() {
173+
if machineSet != nil {
174+
By("Deleting the new MachineSet")
175+
Expect(client.Delete(ctx, machineSet)).To(Succeed(), "MachineSet should be able to be deleted")
176+
framework.WaitForMachineSetsDeleted(ctx, client, machineSet)
177+
}
178+
})
168179
})
169180

170181
AfterEach(func() {
171182
specReport := CurrentSpecReport()
172183
if specReport.Failed() {
173184
Expect(gatherer.WithSpecReport(specReport).GatherAll()).To(Succeed(), "StateGatherer should be able to gather resources")
174185
}
175-
176-
if machineSet != nil {
177-
By("Deleting the new MachineSet")
178-
Expect(client.Delete(ctx, machineSet)).To(Succeed(), "MachineSet should be able to be deleted")
179-
framework.WaitForMachineSetsDeleted(ctx, client, machineSet)
180-
}
181-
182186
})
183187

184188
When("machineset has one replica", func() {

‎pkg/infra/lifecyclehooks.go

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,19 @@ var _ = Describe("Lifecycle Hooks should", framework.LabelMachines, func() {
5858
// Create machine set
5959
machineSet, err = framework.CreateMachineSet(client, machineSetParams)
6060
Expect(err).ToNot(HaveOccurred(), "MachineSet should be able to be created")
61+
62+
// Make sure to clean up the machineSet, if we create one.
63+
DeferCleanup(func() {
64+
By("Deleting the machineset")
65+
cascadeDelete := metav1.DeletePropagationForeground
66+
Expect(client.Delete(context.Background(), machineSet, &runtimeclient.DeleteOptions{
67+
PropagationPolicy: &cascadeDelete,
68+
})).To(Succeed(), "MachineSet should be able to be deleted")
69+
70+
By("Waiting for the MachineSet to be deleted...")
71+
framework.WaitForMachineSetsDeleted(ctx, client, machineSet)
72+
})
73+
6174
// Wait for machine to be running
6275
framework.WaitForMachineSet(ctx, client, machineSet.GetName())
6376

@@ -71,6 +84,15 @@ var _ = Describe("Lifecycle Hooks should", framework.LabelMachines, func() {
7184
})
7285
Expect(client.Create(context.Background(), workload)).To(Succeed(), "Could not create workload job")
7386

87+
// Make sure to clean up the workload job, if we create one.
88+
DeferCleanup(func() {
89+
cascadeDelete := metav1.DeletePropagationForeground
90+
By("Deleting workload job")
91+
Expect(client.Delete(context.Background(), workload, &runtimeclient.DeleteOptions{
92+
PropagationPolicy: &cascadeDelete,
93+
})).To(Succeed(), "Workload job should be able to be deleted")
94+
})
95+
7496
By("Waiting for job pod to start running on machine.")
7597
Eventually(func() (bool, error) {
7698
jobPodList, err := framework.GetPods(client, map[string]string{lifecycleHooksPodLabel: ""})
@@ -91,20 +113,6 @@ var _ = Describe("Lifecycle Hooks should", framework.LabelMachines, func() {
91113
if specReport.Failed() {
92114
Expect(gatherer.WithSpecReport(specReport).GatherAll()).To(Succeed(), "StateGatherer should be able to gather resources")
93115
}
94-
95-
By("Deleting the machineset")
96-
cascadeDelete := metav1.DeletePropagationForeground
97-
Expect(client.Delete(context.Background(), machineSet, &runtimeclient.DeleteOptions{
98-
PropagationPolicy: &cascadeDelete,
99-
})).To(Succeed(), "MachineSet should be able to be deleted")
100-
101-
By("Waiting for the MachineSet to be deleted...")
102-
framework.WaitForMachineSetsDeleted(ctx, client, machineSet)
103-
104-
By("Deleting workload job")
105-
Expect(client.Delete(context.Background(), workload, &runtimeclient.DeleteOptions{
106-
PropagationPolicy: &cascadeDelete,
107-
})).To(Succeed(), "Workload job should be able to be deleted")
108116
})
109117

110118
// Machines required for test: 1

‎pkg/infra/spot.go

Lines changed: 22 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,28 @@ var _ = Describe("Running on Spot", framework.LabelMachines, framework.LabelSpot
5252
BeforeEach(func() {
5353
delObjects = make(map[string]runtimeclient.Object)
5454

55+
// Make sure to clean up the resources we created
56+
DeferCleanup(func() {
57+
var machineSets []*machinev1.MachineSet
58+
59+
for _, obj := range delObjects {
60+
if machineSet, ok := obj.(*machinev1.MachineSet); ok {
61+
// Once we delete a MachineSet we should make sure that the
62+
// all of its machines are deleted as well.
63+
// Collect MachineSets to wait for.
64+
machineSets = append(machineSets, machineSet)
65+
}
66+
67+
Expect(deleteObject(client, obj)).To(Succeed(), "Should be able to cleanup test objects")
68+
}
69+
70+
if len(machineSets) > 0 {
71+
// Wait for all MachineSets and their Machines to be deleted.
72+
By("Waiting for MachineSets to be deleted...")
73+
framework.WaitForMachineSetsDeleted(ctx, client, machineSets...)
74+
}
75+
})
76+
5577
var err error
5678

5779
gatherer, err = framework.NewGatherer()
@@ -117,25 +139,6 @@ var _ = Describe("Running on Spot", framework.LabelMachines, framework.LabelSpot
117139
if specReport.Failed() {
118140
Expect(gatherer.WithSpecReport(specReport).GatherAll()).To(Succeed(), "StateGatherer should be able to gather resources")
119141
}
120-
121-
var machineSets []*machinev1.MachineSet
122-
123-
for _, obj := range delObjects {
124-
if machineSet, ok := obj.(*machinev1.MachineSet); ok {
125-
// Once we delete a MachineSet we should make sure that the
126-
// all of its machines are deleted as well.
127-
// Collect MachineSets to wait for.
128-
machineSets = append(machineSets, machineSet)
129-
}
130-
131-
Expect(deleteObject(client, obj)).To(Succeed(), "Should be able to cleanup test objects")
132-
}
133-
134-
if len(machineSets) > 0 {
135-
// Wait for all MachineSets and their Machines to be deleted.
136-
By("Waiting for MachineSets to be deleted...")
137-
framework.WaitForMachineSetsDeleted(ctx, client, machineSets...)
138-
}
139142
})
140143

141144
// Machines required for test: 1

‎pkg/infra/webhooks.go

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -70,23 +70,26 @@ var _ = Describe("Webhooks", framework.LabelMachines, func() {
7070
return framework.IsValidatingWebhookConfigurationSynced(ctx, client)
7171
}, framework.WaitShort).Should(BeTrue(), "ValidingWebhookConfiguration must be synced before running these tests")
7272
})
73+
74+
// Make sure to clean up the resources we created
75+
DeferCleanup(func() {
76+
machineSets, err := framework.GetMachineSets(client, testSelector)
77+
Expect(err).ToNot(HaveOccurred(), "Should be able to list test MachineSets")
78+
Expect(framework.DeleteMachineSets(client, machineSets...)).To(Succeed(), "Should be able to delete test MachineSets")
79+
framework.WaitForMachineSetsDeleted(ctx, client, machineSets...)
80+
81+
machines, err := framework.GetMachines(ctx, client, testSelector)
82+
Expect(err).ToNot(HaveOccurred(), "Should be able to get test Machines")
83+
Expect(framework.DeleteMachines(ctx, client, machines...)).To(Succeed(), "Should be able to delete test Machines")
84+
framework.WaitForMachinesDeleted(client, machines...)
85+
})
7386
})
7487

7588
AfterEach(func() {
7689
specReport := CurrentSpecReport()
7790
if specReport.Failed() {
7891
Expect(gatherer.WithSpecReport(specReport).GatherAll()).To(Succeed(), "StateGatherer should be able to gather resources")
7992
}
80-
81-
machineSets, err := framework.GetMachineSets(client, testSelector)
82-
Expect(err).ToNot(HaveOccurred(), "Should be able to list test MachineSets")
83-
Expect(framework.DeleteMachineSets(client, machineSets...)).To(Succeed(), "Should be able to delete test MachineSets")
84-
framework.WaitForMachineSetsDeleted(ctx, client, machineSets...)
85-
86-
machines, err := framework.GetMachines(ctx, client, testSelector)
87-
Expect(err).ToNot(HaveOccurred(), "Should be able to get test Machines")
88-
Expect(framework.DeleteMachines(ctx, client, machines...)).To(Succeed(), "Should be able to delete test Machines")
89-
framework.WaitForMachinesDeleted(client, machines...)
9093
})
9194

9295
// Machines required for test: 1

‎pkg/machinehealthcheck/machinehealthcheck.go

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,17 @@ var _ = Describe("MachineHealthCheck", framework.LabelMachineHealthChecks, func(
5555
machineSet, err = framework.CreateMachineSet(client, machineSetParams)
5656
Expect(err).ToNot(HaveOccurred(), "failed to create a new machineSet resource")
5757

58+
// Make sure to clean up the resources we created
59+
DeferCleanup(func() {
60+
By("Deleting the MachineHealthCheck resource")
61+
Expect(client.Delete(context.Background(), machinehealthcheck)).To(Succeed(), "failed to delete MHC")
62+
63+
By("Deleting the new MachineSet")
64+
Expect(client.Delete(context.Background(), machineSet)).To(Succeed(), "failed to delete machineSet")
65+
66+
framework.WaitForMachineSetsDeleted(ctx, client, machineSet)
67+
})
68+
5869
framework.WaitForMachineSet(ctx, client, machineSet.GetName())
5970
})
6071

@@ -63,14 +74,6 @@ var _ = Describe("MachineHealthCheck", framework.LabelMachineHealthChecks, func(
6374
if specReport.Failed() {
6475
Expect(gatherer.WithSpecReport(specReport).GatherAll()).To(Succeed(), "failed to gather spec report")
6576
}
66-
67-
By("Deleting the MachineHealthCheck resource")
68-
Expect(client.Delete(context.Background(), machinehealthcheck)).To(Succeed(), "failed to delete MHC")
69-
70-
By("Deleting the new MachineSet")
71-
Expect(client.Delete(context.Background(), machineSet)).To(Succeed(), "failed to delete machineSet")
72-
73-
framework.WaitForMachineSetsDeleted(ctx, client, machineSet)
7477
})
7578

7679
// Machines required for test: 3

‎pkg/providers/aws.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,18 +50,21 @@ var _ = Describe("MetadataServiceOptions", framework.LabelCloudProviderSpecific,
5050
if platform != configv1.AWSPlatformType {
5151
Skip(fmt.Sprintf("skipping AWS specific tests on %s", platform))
5252
}
53+
54+
// Make sure to clean up the resources we created
55+
DeferCleanup(func() {
56+
Expect(framework.DeleteMachineSets(client, toDelete...)).To(Succeed())
57+
toDelete = make([]*machinev1.MachineSet, 0, 3)
58+
59+
framework.WaitForMachineSetsDeleted(ctx, client, toDelete...)
60+
})
5361
})
5462

5563
AfterEach(func() {
5664
specReport := CurrentSpecReport()
5765
if specReport.Failed() {
5866
Expect(gatherer.WithSpecReport(specReport).GatherAll()).To(Succeed())
5967
}
60-
61-
Expect(framework.DeleteMachineSets(client, toDelete...)).To(Succeed())
62-
toDelete = make([]*machinev1.MachineSet, 0, 3)
63-
64-
framework.WaitForMachineSetsDeleted(ctx, client, toDelete...)
6568
})
6669

6770
createMachineSet := func(metadataAuth string) (*machinev1.MachineSet, error) {

0 commit comments

Comments
 (0)