Skip to content

Commit 60f9fc4

Browse files
rxbnrxbn
committed
refactor: rely on child agents for VM ownership
1 parent 3bd2677 commit 60f9fc4

2 files changed

Lines changed: 61 additions & 120 deletions

File tree

‎internal/controller/vsphereagentpool_controller.go‎

Lines changed: 10 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -139,42 +139,40 @@ func (r *VsphereAgentPoolReconciler) Reconcile(ctx context.Context, req ctrl.Req
139139
return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
140140
}
141141

142-
agents, err := r.listMatchingAgents(ctx, &pool)
142+
ownedVMs, err := r.listVsphereAgentVMs(ctx, &pool)
143143
if err != nil {
144-
r.setStatusError(&pool, "AgentListFailed", err.Error())
144+
r.setStatusError(&pool, "VsphereAgentListFailed", err.Error())
145145
if statusErr := r.updateStatus(ctx, &pool, PoolPlan{}); statusErr != nil {
146146
return ctrl.Result{}, statusErr
147147
}
148148
return ctrl.Result{}, err
149149
}
150-
machines, err := r.listNodePoolMachines(ctx, &pool)
150+
pool.Status.OwnedVMs = ownedVMs
151+
152+
agents, err := r.listMatchingAgents(ctx, &pool)
151153
if err != nil {
152-
r.setStatusError(&pool, "MachineListFailed", err.Error())
154+
r.setStatusError(&pool, "AgentListFailed", err.Error())
153155
if statusErr := r.updateStatus(ctx, &pool, PoolPlan{}); statusErr != nil {
154156
return ctrl.Result{}, statusErr
155157
}
156158
return ctrl.Result{}, err
157159
}
158-
agentMachineDemand, err := r.countAgentMachineDemand(ctx, &pool)
160+
machines, err := r.listNodePoolMachines(ctx, &pool)
159161
if err != nil {
160-
r.setStatusError(&pool, "AgentMachineListFailed", err.Error())
162+
r.setStatusError(&pool, "MachineListFailed", err.Error())
161163
if statusErr := r.updateStatus(ctx, &pool, PoolPlan{}); statusErr != nil {
162164
return ctrl.Result{}, statusErr
163165
}
164166
return ctrl.Result{}, err
165167
}
166-
ownedVMs, err := r.listVsphereAgentVMs(ctx, &pool)
168+
agentMachineDemand, err := r.countAgentMachineDemand(ctx, &pool)
167169
if err != nil {
168-
r.setStatusError(&pool, "VsphereAgentListFailed", err.Error())
170+
r.setStatusError(&pool, "AgentMachineListFailed", err.Error())
169171
if statusErr := r.updateStatus(ctx, &pool, PoolPlan{}); statusErr != nil {
170172
return ctrl.Result{}, statusErr
171173
}
172174
return ctrl.Result{}, err
173175
}
174-
if len(ownedVMs) == 0 && len(pool.Status.OwnedVMs) > 0 {
175-
ownedVMs = pool.Status.OwnedVMs
176-
}
177-
pool.Status.OwnedVMs = ownedVMs
178176
pool.Status.OwnedVMs = refreshOwnedVMStatuses(&pool, agents, machines)
179177

180178
plan := buildPlan(&pool, PoolSnapshot{
@@ -229,22 +227,6 @@ func (r *VsphereAgentPoolReconciler) reconcileDelete(ctx context.Context, pool *
229227
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
230228
}
231229

232-
if cleanupEnabled(pool) && len(pool.Status.OwnedVMs) > 0 {
233-
provider, err := r.provider(ctx, pool)
234-
if err != nil {
235-
return ctrl.Result{}, err
236-
}
237-
for _, vm := range pool.Status.OwnedVMs {
238-
if vm.Name == "" {
239-
continue
240-
}
241-
if err := provider.DeleteVM(ctx, pool, vm); err != nil {
242-
recordVMOperation("delete", err)
243-
return ctrl.Result{}, err
244-
}
245-
recordVMOperation("delete", nil)
246-
}
247-
}
248230
controllerutil.RemoveFinalizer(pool, finalizerName)
249231
return ctrl.Result{}, r.patchFinalizer(ctx, pool)
250232
}

‎internal/controller/vsphereagentpool_controller_test.go‎

Lines changed: 51 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -274,75 +274,14 @@ func TestPoolDeleteWaitsForVsphereAgentFinalizers(t *testing.T) {
274274
}
275275
}
276276

277-
func TestPoolDeleteCleansUpLegacyStatusVMsAfterChildrenGone(t *testing.T) {
277+
func TestPoolDeleteRemovesFinalizerAfterChildrenGone(t *testing.T) {
278278
ctx := context.Background()
279279
scheme := runtime.NewScheme()
280280
if err := agentforgev1alpha1.AddToScheme(scheme); err != nil {
281281
t.Fatal(err)
282282
}
283-
if err := corev1.AddToScheme(scheme); err != nil {
284-
t.Fatal(err)
285-
}
286-
287-
pool := reconcileTestPool()
288-
pool.DeletionTimestamp = &metav1.Time{Time: time.Now()}
289-
pool.Status.OwnedVMs = []agentforgev1alpha1.OwnedVMStatus{newOwnedVMStatus("legacy-vm")}
290-
secret := &corev1.Secret{
291-
ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "vsphere-credentials"},
292-
Data: map[string][]byte{
293-
"server": []byte("vcenter.example.invalid"),
294-
"username": []byte("user"),
295-
"password": []byte("pass"),
296-
},
297-
}
298-
299-
k8sClient := fake.NewClientBuilder().
300-
WithScheme(scheme).
301-
WithObjects(pool, secret).
302-
WithStatusSubresource(pool).
303-
Build()
304-
305-
provider := &fakeVMProvider{}
306-
reconciler := &VsphereAgentPoolReconciler{
307-
Client: k8sClient,
308-
Scheme: scheme,
309-
Recorder: events.NewFakeRecorder(10),
310-
ProviderFactory: func(context.Context, *agentforgev1alpha1.VsphereAgentPool, *corev1.Secret) (VMProvider, error) {
311-
return provider, nil
312-
},
313-
}
314-
315-
_, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: testNodePool}})
316-
if err != nil {
317-
t.Fatalf("reconcile returned error: %v", err)
318-
}
319-
if provider.deleteVMCalls != 1 || provider.deletedVMNames[0] != "legacy-vm" {
320-
t.Fatalf("deleted VMs = %#v, want legacy-vm", provider.deletedVMNames)
321-
}
322-
var updatedPool agentforgev1alpha1.VsphereAgentPool
323-
if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: testNodePool}, &updatedPool); err != nil {
324-
if apierrors.IsNotFound(err) {
325-
return
326-
}
327-
t.Fatal(err)
328-
}
329-
if controllerutil.ContainsFinalizer(&updatedPool, finalizerName) {
330-
t.Fatalf("pool finalizers = %#v, want pool finalizer removed after legacy VM cleanup", updatedPool.Finalizers)
331-
}
332-
}
333-
334-
func TestPoolDeleteRetainsLegacyStatusVMsWhenCleanupPolicyRetain(t *testing.T) {
335-
ctx := context.Background()
336-
scheme := runtime.NewScheme()
337-
if err := agentforgev1alpha1.AddToScheme(scheme); err != nil {
338-
t.Fatal(err)
339-
}
340-
if err := corev1.AddToScheme(scheme); err != nil {
341-
t.Fatal(err)
342-
}
343283

344284
pool := reconcileTestPool()
345-
pool.Spec.CleanupPolicy = agentforgev1alpha1.CleanupPolicyRetain
346285
pool.DeletionTimestamp = &metav1.Time{Time: time.Now()}
347286
pool.Status.OwnedVMs = []agentforgev1alpha1.OwnedVMStatus{newOwnedVMStatus("legacy-vm")}
348287

@@ -367,7 +306,7 @@ func TestPoolDeleteRetainsLegacyStatusVMsWhenCleanupPolicyRetain(t *testing.T) {
367306
t.Fatalf("reconcile returned error: %v", err)
368307
}
369308
if provider.deleteVMCalls != 0 {
370-
t.Fatalf("DeleteVM calls = %d, want retained VM", provider.deleteVMCalls)
309+
t.Fatalf("DeleteVM calls = %d, want child VsphereAgents to own VM deletion", provider.deleteVMCalls)
371310
}
372311
var updatedPool agentforgev1alpha1.VsphereAgentPool
373312
if err := k8sClient.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: testNodePool}, &updatedPool); err != nil {
@@ -377,7 +316,7 @@ func TestPoolDeleteRetainsLegacyStatusVMsWhenCleanupPolicyRetain(t *testing.T) {
377316
t.Fatal(err)
378317
}
379318
if controllerutil.ContainsFinalizer(&updatedPool, finalizerName) {
380-
t.Fatalf("pool finalizers = %#v, want pool finalizer removed after retaining legacy VM", updatedPool.Finalizers)
319+
t.Fatalf("pool finalizers = %#v, want pool finalizer removed after child cleanup is complete", updatedPool.Finalizers)
381320
}
382321
}
383322

@@ -1072,22 +1011,21 @@ func TestReconcilePatchesCandidateAgentFromInfraEnv(t *testing.T) {
10721011
}
10731012

10741013
pool := reconcileTestPool()
1075-
pool.Status.OwnedVMs = []agentforgev1alpha1.OwnedVMStatus{
1076-
{
1077-
Name: "demo-worker-ab12",
1078-
Phase: phaseProvisioning,
1079-
MACAddress: "00-50-56-b2-a1-9f",
1080-
},
1014+
ownedVM := agentforgev1alpha1.OwnedVMStatus{
1015+
Name: "demo-worker-ab12",
1016+
Phase: phaseProvisioning,
1017+
MACAddress: "00-50-56-b2-a1-9f",
10811018
}
1019+
vsphereAgent := testVsphereAgentForVM(pool, ownedVM)
10821020
am := testAgentMachine(testControlPlaneNamespace, testNodePool, "demo/demo-worker")
10831021
infraEnv := testInfraEnv(testNamespace, testInfraEnvName, "https://example.invalid/discovery.iso")
10841022
agent := testCandidateAgent(testNamespace, "abcdef12-3456-7890-abcd-ef1234567890")
10851023
setAgentPrimaryMAC(t, agent, "00:50:56:b2:a1:9f")
10861024

10871025
k8sClient := fake.NewClientBuilder().
10881026
WithScheme(scheme).
1089-
WithObjects(pool, am, infraEnv, agent).
1090-
WithStatusSubresource(pool).
1027+
WithObjects(pool, am, infraEnv, agent, vsphereAgent).
1028+
WithStatusSubresource(pool, vsphereAgent).
10911029
Build()
10921030

10931031
reconciler := &VsphereAgentPoolReconciler{
@@ -1151,22 +1089,21 @@ func TestReconcilePatchesCandidateAgentWithPoolDiscriminatorLabel(t *testing.T)
11511089

11521090
pool := reconcileTestPool()
11531091
pool.Spec.Agent.Labels[poolLabelKey] = "worker-32c128g"
1154-
pool.Status.OwnedVMs = []agentforgev1alpha1.OwnedVMStatus{
1155-
{
1156-
Name: "demo-worker-32c128g-ab12",
1157-
Phase: phaseProvisioning,
1158-
MACAddress: "00-50-56-b2-a1-9f",
1159-
},
1092+
ownedVM := agentforgev1alpha1.OwnedVMStatus{
1093+
Name: "demo-worker-32c128g-ab12",
1094+
Phase: phaseProvisioning,
1095+
MACAddress: "00-50-56-b2-a1-9f",
11601096
}
1097+
vsphereAgent := testVsphereAgentForVM(pool, ownedVM)
11611098
am := testAgentMachine(testControlPlaneNamespace, testNodePool, "demo/demo-worker")
11621099
infraEnv := testInfraEnv(testNamespace, testInfraEnvName, "https://example.invalid/discovery.iso")
11631100
agent := testCandidateAgent(testNamespace, "abcdef12-3456-7890-abcd-ef1234567890")
11641101
setAgentPrimaryMAC(t, agent, "00:50:56:b2:a1:9f")
11651102

11661103
k8sClient := fake.NewClientBuilder().
11671104
WithScheme(scheme).
1168-
WithObjects(pool, am, infraEnv, agent).
1169-
WithStatusSubresource(pool).
1105+
WithObjects(pool, am, infraEnv, agent, vsphereAgent).
1106+
WithStatusSubresource(pool, vsphereAgent).
11701107
Build()
11711108

11721109
reconciler := &VsphereAgentPoolReconciler{
@@ -1473,13 +1410,18 @@ func TestReconcileDoesNotDeleteProvisioningOwnedVMsWithoutDeletedMachine(t *test
14731410
}
14741411

14751412
pool := reconcileTestPool()
1476-
pool.Status.OwnedVMs = []agentforgev1alpha1.OwnedVMStatus{
1413+
ownedVMs := []agentforgev1alpha1.OwnedVMStatus{
14771414
{Name: "demo-worker-one1", Phase: phaseBound, AgentRef: testAgentRef("demo-worker-one1")},
14781415
{Name: "demo-worker-two2", Phase: phaseBound, AgentRef: testAgentRef("demo-worker-two2")},
14791416
{Name: "demo-worker-thr3", Phase: phaseBound, AgentRef: testAgentRef("demo-worker-thr3")},
14801417
{Name: "demo-worker-old1", Phase: phaseProvisioning, Reason: "AgentNotDiscovered", LastTransitionTime: metav1.Now()},
14811418
{Name: "demo-worker-old2", Phase: phaseProvisioning, Reason: "AgentNotDiscovered", LastTransitionTime: metav1.Now()},
14821419
}
1420+
vsphereAgent1 := testVsphereAgentForVM(pool, ownedVMs[0])
1421+
vsphereAgent2 := testVsphereAgentForVM(pool, ownedVMs[1])
1422+
vsphereAgent3 := testVsphereAgentForVM(pool, ownedVMs[2])
1423+
vsphereAgent4 := testVsphereAgentForVM(pool, ownedVMs[3])
1424+
vsphereAgent5 := testVsphereAgentForVM(pool, ownedVMs[4])
14831425
am := testAgentMachine(testControlPlaneNamespace, testNodePool, "demo/demo-worker")
14841426
infraEnv := testInfraEnv(testNamespace, testInfraEnvName, "https://example.invalid/discovery.iso")
14851427
secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "vsphere-credentials"}}
@@ -1489,8 +1431,8 @@ func TestReconcileDoesNotDeleteProvisioningOwnedVMsWithoutDeletedMachine(t *test
14891431

14901432
k8sClient := fake.NewClientBuilder().
14911433
WithScheme(scheme).
1492-
WithObjects(pool, am, infraEnv, secret, agent1, agent2, agent3).
1493-
WithStatusSubresource(pool).
1434+
WithObjects(pool, am, infraEnv, secret, agent1, agent2, agent3, vsphereAgent1, vsphereAgent2, vsphereAgent3, vsphereAgent4, vsphereAgent5).
1435+
WithStatusSubresource(pool, vsphereAgent1, vsphereAgent2, vsphereAgent3, vsphereAgent4, vsphereAgent5).
14941436
Build()
14951437

14961438
provider := &fakeVMProvider{}
@@ -1540,22 +1482,21 @@ func TestReconcileMarksReturnedAgentReleased(t *testing.T) {
15401482
}
15411483

15421484
pool := reconcileTestPool()
1543-
pool.Status.OwnedVMs = []agentforgev1alpha1.OwnedVMStatus{
1544-
{
1545-
Name: "demo-worker-ab12",
1546-
Phase: phaseBound,
1547-
Reason: "AgentBound",
1548-
AgentRef: &corev1.ObjectReference{Name: "demo-worker-ab12"},
1549-
},
1485+
ownedVM := agentforgev1alpha1.OwnedVMStatus{
1486+
Name: "demo-worker-ab12",
1487+
Phase: phaseBound,
1488+
Reason: "AgentBound",
1489+
AgentRef: &corev1.ObjectReference{Name: "demo-worker-ab12"},
15501490
}
1491+
vsphereAgent := testVsphereAgentForVM(pool, ownedVM)
15511492
am := testAgentMachine(testControlPlaneNamespace, testNodePool, "demo/demo-worker")
15521493
infraEnv := testInfraEnv(testNamespace, testInfraEnvName, "https://example.invalid/discovery.iso")
15531494
agent := testAgent(testNamespace, "demo-worker-ab12", false, true)
15541495

15551496
k8sClient := fake.NewClientBuilder().
15561497
WithScheme(scheme).
1557-
WithObjects(pool, am, infraEnv, agent).
1558-
WithStatusSubresource(pool).
1498+
WithObjects(pool, am, infraEnv, agent, vsphereAgent).
1499+
WithStatusSubresource(pool, vsphereAgent).
15591500
Build()
15601501

15611502
reconciler := &VsphereAgentPoolReconciler{
@@ -2233,6 +2174,24 @@ func setAgentPrimaryMAC(t *testing.T, agent *unstructured.Unstructured, mac stri
22332174
}
22342175
}
22352176

2177+
func testVsphereAgentForVM(pool *agentforgev1alpha1.VsphereAgentPool, vm agentforgev1alpha1.OwnedVMStatus) *agentforgev1alpha1.VsphereAgent {
2178+
return &agentforgev1alpha1.VsphereAgent{
2179+
ObjectMeta: metav1.ObjectMeta{
2180+
Namespace: pool.Namespace,
2181+
Name: vm.Name,
2182+
Labels: map[string]string{
2183+
vsphereAgentPoolNameLabel: pool.Name,
2184+
},
2185+
},
2186+
Spec: agentforgev1alpha1.VsphereAgentSpec{
2187+
PoolRef: agentforgev1alpha1.LocalObjectReference{Name: pool.Name},
2188+
},
2189+
Status: agentforgev1alpha1.VsphereAgentStatus{
2190+
VM: vm,
2191+
},
2192+
}
2193+
}
2194+
22362195
func findCondition(conditions []metav1.Condition, conditionType string) *metav1.Condition {
22372196
for i := range conditions {
22382197
if conditions[i].Type == conditionType {

0 commit comments

Comments
 (0)