Skip to content

Commit

Permalink
Gate nodes associated with a machine based on resolved provider id
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathan-innis committed May 23, 2023
1 parent 8caf21a commit 7782457
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 6 deletions.
2 changes: 0 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -837,8 +837,6 @@ k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280 h1:+70TFaan3hfJzs+7VK2o+O
k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280/go.mod h1:+Axhij7bCpeqhklhUTe3xmOn6bWxolyZEeyaFpjGtl4=
k8s.io/utils v0.0.0-20221108210102-8e77b1f39fe2 h1:GfD9OzL11kvZN5iArC6oTS7RTj7oJOIfnislxYlqTj8=
k8s.io/utils v0.0.0-20221108210102-8e77b1f39fe2/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
knative.dev/pkg v0.0.0-20221123154742-05b694ec4d3a h1:mTDxXL+zRBMz7BcdM3WOgw9FVbmkIN/3cvEj4MeS8zI=
knative.dev/pkg v0.0.0-20221123154742-05b694ec4d3a/go.mod h1:fckNBPf9bu5/p1RbnOhEauX7r+kfN1zSQupEVtkaYBs=
knative.dev/pkg v0.0.0-20230502134655-db8a35330281 h1:9mN8O5XO68DKlkzEhFAShUx+O/I+TQR71vmTvYt8oF4=
knative.dev/pkg v0.0.0-20230502134655-db8a35330281/go.mod h1:2qWPP9Gjh9Q7ETti+WRHnBnGCSCq+6q7m3p/nmUQviE=
rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
Expand Down
5 changes: 4 additions & 1 deletion pkg/controllers/machine/termination/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"time"

"github.com/samber/lo"
"golang.org/x/time/rate"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
Expand Down Expand Up @@ -86,7 +87,9 @@ func (c *Controller) Finalize(ctx context.Context, machine *v1alpha5.Machine) (r
if len(nodes) > 0 {
return reconcile.Result{}, nil
}
if machine.Status.ProviderID != "" {
// If the providerID is defined, use that value; else, use the machine linked annotation if it's on the machine
providerID := lo.Ternary(machine.Status.ProviderID != "", machine.Status.ProviderID, machine.Annotations[v1alpha5.MachineLinkedAnnotationKey])
if providerID != "" {
if err := c.cloudProvider.Delete(ctx, machine); cloudprovider.IgnoreMachineNotFoundError(err) != nil {
return reconcile.Result{}, fmt.Errorf("terminating cloudprovider instance, %w", err)
}
Expand Down
44 changes: 44 additions & 0 deletions pkg/controllers/machine/termination/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/samber/lo"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -90,6 +91,9 @@ var _ = Describe("Termination", func() {
Labels: map[string]string{
v1alpha5.ProvisionerNameLabelKey: provisioner.Name,
},
Finalizers: []string{
v1alpha5.TerminationFinalizer
},
},
Spec: v1alpha5.MachineSpec{
Resources: v1alpha5.ResourceRequirements{
Expand Down Expand Up @@ -153,6 +157,26 @@ var _ = Describe("Termination", func() {
_, err = cloudProvider.Get(ctx, machine.Status.ProviderID)
Expect(cloudprovider.IsMachineNotFoundError(err)).To(BeTrue())
})
It("should delete the Node if the Machine is linked but doesn't have its providerID resolved yet", func() {
node := test.MachineLinkedNode(machine)

machine.Annotations = lo.Assign(machine.Annotations, map[string]string{v1alpha5.MachineLinkedAnnotationKey: machine.Status.ProviderID})
machine.Status.ProviderID = ""
ExpectApplied(ctx, env.Client, provisioner, machine, node)

// Expect the machine to be gone
Expect(env.Client.Delete(ctx, machine)).To(Succeed())
ExpectReconcileSucceeded(ctx, terminationController, client.ObjectKeyFromObject(machine)) // triggers the node deletion
ExpectFinalizersRemoved(ctx, env.Client, node)
ExpectNotFound(ctx, env.Client, node)

ExpectReconcileSucceeded(ctx, terminationController, client.ObjectKeyFromObject(machine)) // now all nodes are gone so machine deletion continues
ExpectNotFound(ctx, env.Client, machine, node)

// Expect the machine to be gone from the cloudprovider
_, err := cloudProvider.Get(ctx, machine.Annotations[v1alpha5.MachineLinkedAnnotationKey])
Expect(cloudprovider.IsMachineNotFoundError(err)).To(BeTrue())
})
It("should not delete the Machine until all the Nodes are removed", func() {
ExpectApplied(ctx, env.Client, provisioner, machine)
ExpectReconcileSucceeded(ctx, machineController, client.ObjectKeyFromObject(machine))
Expand All @@ -178,6 +202,7 @@ var _ = Describe("Termination", func() {
ExpectNotFound(ctx, env.Client, machine)
})
It("should not call Delete() on the CloudProvider if the machine hasn't been launched yet", func() {
machine.Status.ProviderID = ""
ExpectApplied(ctx, env.Client, provisioner, machine)

// Expect the machine to be gone
Expand All @@ -187,4 +212,23 @@ var _ = Describe("Termination", func() {
Expect(cloudProvider.DeleteCalls).To(HaveLen(0))
ExpectNotFound(ctx, env.Client, machine)
})
It("should not delete nodes without provider ids if the machine hasn't been launched yet", func() {
// Generate 10 nodes, none of which have a provider id
var nodes []*v1.Node
for i := 0; i < 10; i++ {
nodes = append(nodes, test.Node())
}
ExpectApplied(ctx, env.Client, lo.Map(nodes, func(n *v1.Node, _ int) client.Object { return n })...)

ExpectApplied(ctx, env.Client, provisioner, machine)

// Expect the machine to be gone
Expect(env.Client.Delete(ctx, machine)).To(Succeed())
ExpectReconcileSucceeded(ctx, terminationController, client.ObjectKeyFromObject(machine))

ExpectNotFound(ctx, env.Client, machine)
for _, node := range nodes {
ExpectExists(ctx, env.Client, node)
}
})
})
15 changes: 12 additions & 3 deletions pkg/utils/machine/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,19 +125,28 @@ func NodeForMachine(ctx context.Context, c client.Client, machine *v1alpha5.Mach
if err != nil {
return nil, err
}
// If the providerID is defined, use that value; else, use the machine linked annotation if it's on the machine
providerID := lo.Ternary(machine.Status.ProviderID != "", machine.Status.ProviderID, machine.Annotations[v1alpha5.MachineLinkedAnnotationKey])
if len(nodes) > 1 {
return nil, &DuplicateNodeError{ProviderID: machine.Status.ProviderID}
return nil, &DuplicateNodeError{ProviderID: providerID}
}
if len(nodes) == 0 {
return nil, &NodeNotFoundError{ProviderID: machine.Status.ProviderID}
return nil, &NodeNotFoundError{ProviderID: providerID}
}
return nodes[0], nil
}

// AllNodesForMachine is a helper function that takes a v1alpha5.Machine and finds ALL matching v1.Nodes by their providerID
// If the providerID is not resolved for a Machine, then no Nodes will map to it
func AllNodesForMachine(ctx context.Context, c client.Client, machine *v1alpha5.Machine) ([]*v1.Node, error) {
// If the providerID is defined, use that value; else, use the machine linked annotation if it's on the machine
providerID := lo.Ternary(machine.Status.ProviderID != "", machine.Status.ProviderID, machine.Annotations[v1alpha5.MachineLinkedAnnotationKey])
// Machines that have no resolved providerID have no nodes mapped to them
if providerID == "" {
return nil, nil
}
nodeList := v1.NodeList{}
if err := c.List(ctx, &nodeList, client.MatchingFields{"spec.providerID": machine.Status.ProviderID}); err != nil {
if err := c.List(ctx, &nodeList, client.MatchingFields{"spec.providerID": providerID}); err != nil {
return nil, fmt.Errorf("listing nodes, %w", err)
}
return lo.ToSlicePtr(nodeList.Items), nil
Expand Down

0 comments on commit 7782457

Please sign in to comment.