Skip to content

Commit

Permalink
Removing labels without tolerations
Browse files Browse the repository at this point in the history 
This change addresses #940.
Kmod labels are now deleted only if the kernel
module does not have the appropriate tolerations for the taints on the node.
  • Loading branch information
@TomerNewman
TomerNewman committed Dec 8, 2024
1 parent 29b6433 commit 5bf1f5e
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 37 deletions.
2 changes: 1 addition & 1 deletion internal/controllers/nmc_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r

// removing label of loaded kmods
if !r.nodeAPI.IsNodeSchedulable(&node, nil) {
if err := r.nodeAPI.RemoveNodeReadyLabels(ctx, &node); err != nil {
if err := r.nodeAPI.RemoveNodeReadyLabels(ctx, &node, &nmcObj); err != nil {
return ctrl.Result{}, fmt.Errorf("could remove node %s labels: %v", node.Name, err)
}
return ctrl.Result{}, nil
Expand Down
8 changes: 4 additions & 4 deletions internal/controllers/nmc_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,8 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
),
nm.EXPECT().IsNodeSchedulable(&node, nil).Return(false),
nm.EXPECT().IsNodeSchedulable(&node, nil).Return(false),
nm.EXPECT().RemoveNodeReadyLabels(ctx, &node).DoAndReturn(
func(_ context.Context, obj ctrlclient.Object) error {
nm.EXPECT().RemoveNodeReadyLabels(ctx, &node, nmc).DoAndReturn(
func(_ context.Context, obj ctrlclient.Object, _ *kmmv1beta1.NodeModulesConfig) error {
delete(node.ObjectMeta.Labels, kmodName)
return nil
},
Expand Down Expand Up @@ -219,8 +219,8 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
},
),
nm.EXPECT().IsNodeSchedulable(&node, nil).Return(false),
nm.EXPECT().RemoveNodeReadyLabels(ctx, &node).DoAndReturn(
func(_ context.Context, obj ctrlclient.Object) error {
nm.EXPECT().RemoveNodeReadyLabels(ctx, &node, nmc).DoAndReturn(
func(_ context.Context, obj ctrlclient.Object, _ *kmmv1beta1.NodeModulesConfig) error {
return fmt.Errorf("some error")
},
),
Expand Down
9 changes: 5 additions & 4 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.

37 changes: 28 additions & 9 deletions internal/node/node.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package node
import (
"context"
"fmt"
"github.com/kubernetes-sigs/kernel-module-management/api/v1beta1"
"github.com/kubernetes-sigs/kernel-module-management/internal/meta"
"github.com/kubernetes-sigs/kernel-module-management/internal/utils"
v1 "k8s.io/api/core/v1"
Expand All @@ -19,7 +20,7 @@ type Node interface {
GetNumTargetedNodes(ctx context.Context, selector map[string]string, tolerations []v1.Toleration) (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
RemoveNodeReadyLabels(ctx context.Context, node *v1.Node, nmcObj *v1beta1.NodeModulesConfig) error
}

type node struct {
Expand Down Expand Up @@ -87,14 +88,8 @@ 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)
}
}
func (n *node) RemoveNodeReadyLabels(ctx context.Context, node *v1.Node, nmcObj *v1beta1.NodeModulesConfig) error {
labelsToRemove := getLabelsToRemove(node, nmcObj)
if err := n.UpdateLabels(ctx, node, []string{}, labelsToRemove); err != nil {
return fmt.Errorf("could update node %s labels: %v", node.Name, err)
}
Expand Down Expand Up @@ -130,3 +125,27 @@ func removeLabels(node *v1.Node, labels []string) {
)
}
}

func getLabelsToRemove(node *v1.Node, nmcObj *v1beta1.NodeModulesConfig) []string {
var labelsToRemove []string
for _, module := range nmcObj.Spec.Modules {
shouldRemoveLabel := false
for _, taint := range node.Spec.Taints {
isTolerated := false
for _, toleration := range module.Config.Tolerations {
if toleration.ToleratesTaint(&taint) {
isTolerated = true
break
}
}
if !isTolerated {
shouldRemoveLabel = true
break
}
}
if shouldRemoveLabel {
labelsToRemove = append(labelsToRemove, utils.GetKernelModuleReadyNodeLabel(module.Namespace, module.Name))
}
}
return labelsToRemove
}
135 changes: 116 additions & 19 deletions internal/node/node_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package node
import (
"context"
"fmt"
"github.com/kubernetes-sigs/kernel-module-management/api/v1beta1"
"github.com/kubernetes-sigs/kernel-module-management/internal/client"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -160,9 +161,10 @@ var _ = Describe("GetNodesListBySelector", func() {
})

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"
firstloadedKernelModuleReadyNodeLabel = "kmm.node.kubernetes.io/loaded1-ns.loaded1-n.ready"
secondloadedKernelModuleReadyNodeLabel = "kmm.node.kubernetes.io/loaded2-ns.loaded2-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 All @@ -186,14 +188,14 @@ var _ = Describe("UpdateLabels", func() {
Labels: map[string]string{},
},
}
loaded := []string{loadedKernelModuleReadyNodeLabel}
loaded := []string{firstloadedKernelModuleReadyNodeLabel}
unloaded := []string{unloadedKernelModuleReadyNodeLabel}

clnt.EXPECT().Patch(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)

err := n.UpdateLabels(ctx, &node, loaded, unloaded)
Expect(err).ToNot(HaveOccurred())
Expect(node.Labels).To(HaveKey(loadedKernelModuleReadyNodeLabel))
Expect(node.Labels).To(HaveKey(firstloadedKernelModuleReadyNodeLabel))
})

It("Should fail to patch node", func() {
Expand All @@ -202,7 +204,7 @@ var _ = Describe("UpdateLabels", func() {
Labels: map[string]string{},
},
}
loaded := []string{loadedKernelModuleReadyNodeLabel}
loaded := []string{firstloadedKernelModuleReadyNodeLabel}
unloaded := []string{unloadedKernelModuleReadyNodeLabel}

clnt.EXPECT().Patch(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error"))
Expand Down Expand Up @@ -325,33 +327,72 @@ var _ = Describe("RemoveNodeReadyLabels", func() {
node *v1.Node
ctx context.Context
clnt *client.MockClient
nmc *v1beta1.NodeModulesConfig
)

BeforeEach(func() {
ctrl = gomock.NewController(GinkgoT())
clnt = client.NewMockClient(ctrl)
ctx = context.TODO()
n = NewNode(clnt)
nmc = &v1beta1.NodeModulesConfig{
Spec: v1beta1.NodeModulesConfigSpec{
Modules: []v1beta1.NodeModuleSpec{
{
ModuleItem: v1beta1.ModuleItem{
Name: "loaded1-n",
Namespace: "loaded1-ns",
},
Config: v1beta1.ModuleConfig{
Tolerations: []v1.Toleration{
{Key: "TestKey", Value: "TestValue", Effect: v1.TaintEffectNoSchedule},
},
},
},
{
ModuleItem: v1beta1.ModuleItem{
Name: "loaded2-n",
Namespace: "loaded2-ns",
},
},
},
},
}
node = &v1.Node{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
loadedKernelModuleReadyNodeLabel: "",
notKernelModuleReadyNodeLabel: "",
firstloadedKernelModuleReadyNodeLabel: "",
secondloadedKernelModuleReadyNodeLabel: "",
notKernelModuleReadyNodeLabel: "",
},
},
}
})

It("Should remove all kmod labels", func() {
clnt.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
err := n.RemoveNodeReadyLabels(ctx, node)
It("Should not remove any kmod labels from a node", func() {
clnt.EXPECT().Patch(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
err := n.RemoveNodeReadyLabels(ctx, node, nmc)
Expect(err).To(BeNil())
Expect(node.Labels).ToNot(HaveKey(loadedKernelModuleReadyNodeLabel))
Expect(node.Labels).To(HaveKey(firstloadedKernelModuleReadyNodeLabel))
Expect(node.Labels).To(HaveKey(secondloadedKernelModuleReadyNodeLabel))
Expect(node.Labels).To(HaveKey(notKernelModuleReadyNodeLabel))
})
It("Should remove only kmod labels with tolerations to the taints", func() {
node.Spec = v1.NodeSpec{
Taints: []v1.Taint{
{Key: "TestKey", Value: "TestValue", Effect: v1.TaintEffectNoSchedule},
},
}
clnt.EXPECT().Patch(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
err := n.RemoveNodeReadyLabels(ctx, node, nmc)
Expect(err).To(BeNil())
Expect(node.Labels).To(HaveKey(firstloadedKernelModuleReadyNodeLabel))
Expect(node.Labels).ToNot(HaveKey(secondloadedKernelModuleReadyNodeLabel))
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)
clnt.EXPECT().Patch(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error"))
err := n.RemoveNodeReadyLabels(ctx, node, nmc)
Expect(err).ToNot(BeNil())
})
})
Expand All @@ -364,9 +405,9 @@ var _ = Describe("addLabels", func() {
})

It("Should add labels", func() {
labels := []string{loadedKernelModuleReadyNodeLabel}
labels := []string{firstloadedKernelModuleReadyNodeLabel}
addLabels(&node, labels)
Expect(node.Labels).To(HaveKey(loadedKernelModuleReadyNodeLabel))
Expect(node.Labels).To(HaveKey(firstloadedKernelModuleReadyNodeLabel))
})
})

Expand All @@ -377,15 +418,71 @@ var _ = Describe("removeLabels", func() {
node = v1.Node{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
loadedKernelModuleReadyNodeLabel: "",
firstloadedKernelModuleReadyNodeLabel: "",
},
},
}
})

It("Should remove labels", func() {
labels := []string{loadedKernelModuleReadyNodeLabel}
labels := []string{firstloadedKernelModuleReadyNodeLabel}
removeLabels(&node, labels)
Expect(node.Labels).ToNot(HaveKey(loadedKernelModuleReadyNodeLabel))
Expect(node.Labels).ToNot(HaveKey(firstloadedKernelModuleReadyNodeLabel))
})
})

var _ = Describe("getLabelsToRemove", func() {
var (
node *v1.Node
nmc *v1beta1.NodeModulesConfig
)

BeforeEach(func() {
nmc = &v1beta1.NodeModulesConfig{
Spec: v1beta1.NodeModulesConfigSpec{
Modules: []v1beta1.NodeModuleSpec{
{
ModuleItem: v1beta1.ModuleItem{
Name: "loaded1-n",
Namespace: "loaded1-ns",
},
Config: v1beta1.ModuleConfig{
Tolerations: []v1.Toleration{
{Key: "TestKey", Value: "TestValue", Effect: v1.TaintEffectNoSchedule},
},
},
},
{
ModuleItem: v1beta1.ModuleItem{
Name: "loaded2-n",
Namespace: "loaded2-ns",
},
},
},
},
}
node = &v1.Node{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
firstloadedKernelModuleReadyNodeLabel: "",
secondloadedKernelModuleReadyNodeLabel: "",
notKernelModuleReadyNodeLabel: "",
},
},
}
})

It("Should not get any kmod labels from a taintless node", func() {
labels := getLabelsToRemove(node, nmc)
Expect(labels).To(BeNil())
})
It("Should get only kmod labels without tolerations to the taints", func() {
node.Spec = v1.NodeSpec{
Taints: []v1.Taint{
{Key: "TestKey", Value: "TestValue", Effect: v1.TaintEffectNoSchedule},
},
}
labels := getLabelsToRemove(node, nmc)
Expect(labels).To(Equal([]string{secondloadedKernelModuleReadyNodeLabel}))
})
})

0 comments on commit 5bf1f5e

Please sign in to comment.