-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Deleting out-of-date machines #40
Conversation
controllers/controller_test.go
Outdated
|
||
machineList := &clusterv1.MachineList{} | ||
g.Expect(fakeClient.List(context.Background(), machineList, client.InNamespace("test"))).To(Succeed()) | ||
g.Expect(len(machineList.Items)).To(Equal(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this improves the error message in case of error
g.Expect(len(machineList.Items)).To(Equal(0)) | |
g.Expect(machineList.Items).To(BeEmpty()) |
controllers/controller_test.go
Outdated
machine2.OwnerReferences = []metav1.OwnerReference{ | ||
{ | ||
Kind: "Cluster", | ||
APIVersion: clusterv1.GroupVersion.String(), | ||
Name: cluster.Name, | ||
UID: cluster.GetUID(), | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this ownership is not realistic afaik
machine2.OwnerReferences = []metav1.OwnerReference{ | |
{ | |
Kind: "Cluster", | |
APIVersion: clusterv1.GroupVersion.String(), | |
Name: cluster.Name, | |
UID: cluster.GetUID(), | |
}, | |
} | |
notOwnedMachine.OwnerReferences = []metav1.OwnerReference{} |
controllers/controller_test.go
Outdated
etcdadmCluster.Status.Initialized = true | ||
// etcdadm controller has also registered that the status.Initialized field is true, so it has set InitializedCondition to true | ||
conditions.MarkTrue(etcdadmCluster, etcdv1.InitializedCondition) | ||
machine := newEtcdMachine(etcdadmCluster, cluster) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
machine := newEtcdMachine(etcdadmCluster, cluster) | |
ownedMachine := newEtcdMachine(etcdadmCluster, cluster) |
controllers/controller_test.go
Outdated
// etcdadm controller has also registered that the status.Initialized field is true, so it has set InitializedCondition to true | ||
conditions.MarkTrue(etcdadmCluster, etcdv1.InitializedCondition) | ||
machine := newEtcdMachine(etcdadmCluster, cluster) | ||
machine2 := newEtcdMachine(etcdadmCluster, cluster) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
machine2 := newEtcdMachine(etcdadmCluster, cluster) | |
notOwnedMachine := newEtcdMachine(etcdadmCluster, cluster) |
controllers/controller.go
Outdated
@@ -342,7 +342,7 @@ func (r *EtcdadmClusterReconciler) reconcileDelete(ctx context.Context, etcdClus | |||
|
|||
ownedMachines := etcdMachines.Filter(collections.OwnedMachines(etcdCluster)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can move this to after the length check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
Issue #, if available:
https://github.com/aws/eks-anywhere-internal/issues/1626
Description of changes:
We should only remove the finalizer on the EtcdadmCluster when all machines have been deleted. Otherwise there may be a situation where out-of-date machines never get cleaned up.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.