Skip to content
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.

Sign up for GitHub

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

🐛 Early bind ProviderID #11985

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

🐛 Early bind ProviderID #11985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7bffa2b
early bind ProviderID
daimaxiaxie Mar 18, 2025
c6920ff
Revert "early bind ProviderID"
daimaxiaxie Mar 24, 2025
e99297a
delete node by ProviderID
daimaxiaxie Mar 24, 2025
d436c77
update comment
daimaxiaxie Mar 27, 2025
2f0bc16
use nil
daimaxiaxie Mar 27, 2025
dff0573
use unstructured
daimaxiaxie Mar 27, 2025
c2dc2eb
update unstrustured
daimaxiaxie Mar 27, 2025
c3a1ef6
clean
daimaxiaxie Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions internal/controllers/machine/machine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope) (ctrl.Result
s.deletingReason = clusterv1.MachineDeletingV1Beta2Reason
s.deletingMessage = "Deletion started"

err := r.isDeleteNodeAllowed(ctx, cluster, m)
err := r.isDeleteNodeAllowed(ctx, cluster, m, s.infraMachine)
isDeleteNodeAllowed := err == nil
if err != nil {
switch err {
Expand Down Expand Up @@ -713,14 +713,22 @@ func (r *Reconciler) nodeVolumeDetachTimeoutExceeded(machine *clusterv1.Machine)

// isDeleteNodeAllowed returns nil only if the Machine's NodeRef is not nil
// and if the Machine is not the last control plane node in the cluster.
func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error {
func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine, infraMachine *unstructured.Unstructured) error {
log := ctrl.LoggerFrom(ctx)
// Return early if the cluster is being deleted.
if !cluster.DeletionTimestamp.IsZero() {
return errClusterIsBeingDeleted
}

if machine.Status.NodeRef == nil && machine.Spec.ProviderID != nil {
var providerID string
if machine.Spec.ProviderID != nil {
providerID = *machine.Spec.ProviderID
} else if infraMachine != nil {
// Fallback to retrieve from infraMachine.
_ = util.UnstructuredUnmarshalField(infraMachine, &providerID, "spec", "providerID")
}

if machine.Status.NodeRef == nil && providerID != "" {
// If we don't have a node reference, but a provider id has been set,
// try to retrieve the node one more time.
//
Expand All @@ -731,7 +739,7 @@ func (r *Reconciler) isDeleteNodeAllowed(ctx context.Context, cluster *clusterv1
if err != nil {
log.Error(err, "Failed to get cluster client while deleting Machine and checking for nodes")
} else {
node, err := r.getNode(ctx, remoteClient, *machine.Spec.ProviderID)
node, err := r.getNode(ctx, remoteClient, providerID)
if err != nil && err != ErrNodeNotFound {
log.Error(err, "Failed to get node while deleting Machine")
} else if err == nil {
Expand Down
55 changes: 53 additions & 2 deletions internal/controllers/machine/machine_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2530,6 +2530,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
name string
cluster *clusterv1.Cluster
machine *clusterv1.Machine
infraMachine *unstructured.Unstructured
expectedError error
}{
{
Expand All @@ -2556,6 +2557,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
},
Status: clusterv1.MachineStatus{},
},
infraMachine: nil,
expectedError: errNilNodeRef,
},
{
Expand Down Expand Up @@ -2586,6 +2588,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
},
},
},
infraMachine: nil,
expectedError: errNoControlPlaneNodes,
},
{
Expand Down Expand Up @@ -2618,6 +2621,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
},
},
},
infraMachine: nil,
expectedError: errNoControlPlaneNodes,
},
{
Expand Down Expand Up @@ -2648,6 +2652,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
},
},
},
infraMachine: nil,
expectedError: nil,
},
{
Expand All @@ -2661,6 +2666,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
},
},
machine: &clusterv1.Machine{},
infraMachine: nil,
expectedError: errClusterIsBeingDeleted,
},
{
Expand Down Expand Up @@ -2699,6 +2705,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
},
},
},
infraMachine: nil,
expectedError: nil,
},
{
Expand Down Expand Up @@ -2737,6 +2744,7 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
},
},
},
infraMachine: nil,
expectedError: errControlPlaneIsBeingDeleted,
},
{
Expand Down Expand Up @@ -2775,8 +2783,40 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
},
},
},
infraMachine: nil,
expectedError: errControlPlaneIsBeingDeleted,
},
{
name: "no nodeRef, infrastructure machine has providerID",
cluster: &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Namespace: metav1.NamespaceDefault,
},
},
machine: &clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "created",
Namespace: metav1.NamespaceDefault,
Labels: map[string]string{
clusterv1.ClusterNameLabel: "test-cluster",
},
Finalizers: []string{clusterv1.MachineFinalizer},
},
Spec: clusterv1.MachineSpec{
ClusterName: "test-cluster",
InfrastructureRef: corev1.ObjectReference{},
Bootstrap: clusterv1.Bootstrap{DataSecretName: ptr.To("data")},
},
Status: clusterv1.MachineStatus{},
},
infraMachine: &unstructured.Unstructured{Object: map[string]interface{}{
"spec": map[string]interface{}{
"providerID": "test-node-a",
},
}},
expectedError: nil,
},
}

emp := &unstructured.Unstructured{
Expand Down Expand Up @@ -2815,6 +2855,15 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
empBeingDeleted.SetDeletionTimestamp(&metav1.Time{Time: time.Now()})
empBeingDeleted.SetFinalizers([]string{"block-deletion"})

testNodeA := &corev1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node-a",
},
Spec: corev1.NodeSpec{
ProviderID: "test-node-a",
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)
Expand Down Expand Up @@ -2874,11 +2923,13 @@ func TestIsDeleteNodeAllowed(t *testing.T) {
mcpBeingDeleted,
empBeingDeleted,
).Build()
remoteClient := fake.NewClientBuilder().WithIndex(&corev1.Node{}, "spec.providerID", index.NodeByProviderID).WithObjects(testNodeA).Build()
mr := &Reconciler{
Client: c,
Client: c,
ClusterCache: clustercache.NewFakeClusterCache(remoteClient, client.ObjectKeyFromObject(tc.cluster)),
}

err := mr.isDeleteNodeAllowed(ctx, tc.cluster, tc.machine)
err := mr.isDeleteNodeAllowed(ctx, tc.cluster, tc.machine, tc.infraMachine)
if tc.expectedError == nil {
g.Expect(err).ToNot(HaveOccurred())
} else {
Expand Down