Skip to content

Commit

Permalink
Removing labels when node not ready
Browse files Browse the repository at this point in the history 
Until now nodes kept their kmod labels when became not ready.
Today we are deleting them when they are not ready due to potential race condition.
  • Loading branch information
@TomerNewman @k8s-ci-robot
TomerNewman authored and k8s-ci-robot committed Nov 25, 2024
1 parent fc26e4a commit 3596563
Show file tree
Hide file tree
Showing 5 changed files with 161 additions and 15 deletions.
6 changes: 5 additions & 1 deletion internal/controllers/nmc_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,12 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r
return ctrl.Result{}, fmt.Errorf("could not get node %s: %v", nmcObj.Name, err)
}

// skipping handling NMC spec, labelling, events until node becomes ready
// skipping handling NMC spec, events until node becomes ready
// removing label of loaded kmods
if !r.nodeAPI.IsNodeSchedulable(&node) {
if err := r.nodeAPI.RemoveNodeReadyLabels(ctx, &node); err != nil {
return ctrl.Result{}, fmt.Errorf("could remove node %s labels: %v", node.Name, err)
}
return ctrl.Result{}, nil
}

Expand Down
94 changes: 84 additions & 10 deletions internal/controllers/nmc_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,15 @@ import (
)

const (
nmcName = "nmc"
nsFirst = "example-ns-1"
nsSecond = "example-ns-2"
nameFirst = "example-name-1"
nameSecond = "example-name-2"
imageFirst = "example-image-1"
imageSecond = "example-image-2"
nmcName = "nmc"
nsFirst = "example-ns-1"
nsSecond = "example-ns-2"
nameFirst = "example-name-1"
nameSecond = "example-name-2"
imageFirst = "example-image-1"
imageSecond = "example-image-2"
kmodName = "kmm.node.kubernetes.io/test-ns.test-module.ready"
labelNotToRemove = "example.node.kubernetes.io/label-not-to-be-removed"
)

var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
Expand Down Expand Up @@ -126,11 +128,25 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
Expect(err).To(HaveOccurred())
})

It("should not continue if node is not schedulable", func() {
It("should remove kmod labels and not continue if node is not schedulable", func() {
nmc := &kmmv1beta1.NodeModulesConfig{
ObjectMeta: metav1.ObjectMeta{Name: nmcName},
}
node := v1.Node{}
node := v1.Node{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
kmodName: "",
labelNotToRemove: "",
},
},
}
expectedNode := v1.Node{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
labelNotToRemove: "",
},
},
}
gomock.InOrder(
kubeClient.
EXPECT().
Expand All @@ -139,12 +155,70 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
*kubeNmc.(*kmmv1beta1.NodeModulesConfig) = *nmc
}),
wh.EXPECT().SyncStatus(ctx, nmc),
kubeClient.EXPECT().Get(ctx, types.NamespacedName{Name: nmc.Name}, &node).Return(nil),
kubeClient.EXPECT().Get(ctx, types.NamespacedName{Name: nmc.Name}, &v1.Node{}).DoAndReturn(
func(_ context.Context, _ types.NamespacedName, fetchedNode *v1.Node, _ ...ctrlclient.Options) error {
*fetchedNode = node
return nil
},
),
nm.EXPECT().IsNodeSchedulable(&node).Return(false),
nm.EXPECT().RemoveNodeReadyLabels(ctx, &node).DoAndReturn(
func(_ context.Context, obj ctrlclient.Object) error {
delete(node.ObjectMeta.Labels, kmodName)
return nil
},
),
)

_, err := r.Reconcile(ctx, req)
Expect(err).To(BeNil())
Expect(node).To(Equal(expectedNode))
})

It("should fail to remove kmod labels and not continue if node is not schedulable", func() {
nmc := &kmmv1beta1.NodeModulesConfig{
ObjectMeta: metav1.ObjectMeta{Name: nmcName},
}
node := v1.Node{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
kmodName: "",
labelNotToRemove: "",
},
},
}
expectedNode := v1.Node{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
labelNotToRemove: "",
},
},
}
gomock.InOrder(
kubeClient.
EXPECT().
Get(ctx, nmcNsn, &kmmv1beta1.NodeModulesConfig{}).
Do(func(_ context.Context, _ types.NamespacedName, kubeNmc ctrlclient.Object, _ ...ctrlclient.Options) {
*kubeNmc.(*kmmv1beta1.NodeModulesConfig) = *nmc
}),
wh.EXPECT().SyncStatus(ctx, nmc),
kubeClient.EXPECT().Get(ctx, types.NamespacedName{Name: nmc.Name}, &v1.Node{}).DoAndReturn(
func(_ context.Context, _ types.NamespacedName, fetchedNode *v1.Node, _ ...ctrlclient.Options) error {
*fetchedNode = node
return nil
},
),
nm.EXPECT().IsNodeSchedulable(&node).Return(false),
nm.EXPECT().RemoveNodeReadyLabels(ctx, &node).DoAndReturn(
func(_ context.Context, obj ctrlclient.Object) error {
return fmt.Errorf("some error")
},
),
)

_, err := r.Reconcile(ctx, req)
Expect(err).To(HaveOccurred())
Expect(node).ToNot(Equal(expectedNode))
})

It("should process spec entries and orphan statuses", func() {
Expand Down
14 changes: 14 additions & 0 deletions internal/node/mock_node.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions internal/node/node.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"github.com/kubernetes-sigs/kernel-module-management/internal/meta"
"github.com/kubernetes-sigs/kernel-module-management/internal/utils"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -18,6 +19,7 @@ type Node interface {
GetNumTargetedNodes(ctx context.Context, selector map[string]string) (int, error)
UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeRemoved []string) error
NodeBecomeReadyAfter(node *v1.Node, checkTime metav1.Time) bool
RemoveNodeReadyLabels(ctx context.Context, node *v1.Node) error
}

type node struct {
Expand Down Expand Up @@ -78,6 +80,20 @@ func (n *node) UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeR
return nil
}

func (n *node) RemoveNodeReadyLabels(ctx context.Context, node *v1.Node) error {
var labelsToRemove []string
for label := range node.GetLabels() {
if ok, _, _ := utils.IsKernelModuleReadyNodeLabel(label); ok ||
utils.IsDeprecatedKernelModuleReadyNodeLabel(label) {
labelsToRemove = append(labelsToRemove, label)
}
}
if err := n.UpdateLabels(ctx, node, []string{}, labelsToRemove); err != nil {
return fmt.Errorf("could update node %s labels: %v", node.Name, err)
}
return nil
}

func (n *node) NodeBecomeReadyAfter(node *v1.Node, timestamp metav1.Time) bool {
conds := node.Status.Conditions
for i := 0; i < len(conds); i++ {
Expand Down
46 changes: 42 additions & 4 deletions internal/node/node_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"fmt"
"github.com/kubernetes-sigs/kernel-module-management/internal/client"
"github.com/kubernetes-sigs/kernel-module-management/internal/utils"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"go.uber.org/mock/gomock"
Expand Down Expand Up @@ -121,9 +120,10 @@ var _ = Describe("GetNodesListBySelector", func() {
})
})

var (
loadedKernelModuleReadyNodeLabel = utils.GetKernelModuleReadyNodeLabel("loaded-ns", "loaded-n")
unloadedKernelModuleReadyNodeLabel = utils.GetKernelModuleReadyNodeLabel("unloaded-ns", "unloaded-n")
const (
loadedKernelModuleReadyNodeLabel = "kmm.node.kubernetes.io/loaded-ns.loaded-n.ready"
unloadedKernelModuleReadyNodeLabel = "kmm.node.kubernetes.io/unloaded-ns.unloaded-n.ready"
notKernelModuleReadyNodeLabel = "example.node.kubernetes.io/label-not-to-be-removed"
)

var _ = Describe("UpdateLabels", func() {
Expand Down Expand Up @@ -279,6 +279,44 @@ var _ = Describe("NodeBecomeReadyAfter", func() {
})
})

var _ = Describe("RemoveNodeReadyLabels", func() {
var (
ctrl *gomock.Controller
n Node
node *v1.Node
ctx context.Context
clnt *client.MockClient
)

BeforeEach(func() {
ctrl = gomock.NewController(GinkgoT())
clnt = client.NewMockClient(ctrl)
ctx = context.TODO()
n = NewNode(clnt)
node = &v1.Node{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
loadedKernelModuleReadyNodeLabel: "",
notKernelModuleReadyNodeLabel: "",
},
},
}
})

It("Should remove all kmod labels", func() {
clnt.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
err := n.RemoveNodeReadyLabels(ctx, node)
Expect(err).To(BeNil())
Expect(node.Labels).ToNot(HaveKey(loadedKernelModuleReadyNodeLabel))
Expect(node.Labels).To(HaveKey(notKernelModuleReadyNodeLabel))
})
It("Should fail", func() {
clnt.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error"))
err := n.RemoveNodeReadyLabels(ctx, node)
Expect(err).ToNot(BeNil())
})
})

var _ = Describe("addLabels", func() {
var node v1.Node

Expand Down

0 comments on commit 3596563

Please sign in to comment.