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 @k8s-ci-robot
TomerNewman authored and k8s-ci-robot committed Dec 9, 2024
1 parent 3848e68 commit e63298a
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 103 deletions.
7 changes: 4 additions & 3 deletions internal/controllers/nmc_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,15 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r
}

errs := make([]error, 0, len(nmcObj.Spec.Modules)+len(nmcObj.Status.Modules))

var readyLabelsToRemove []string
for _, mod := range nmcObj.Spec.Modules {
moduleNameKey := mod.Namespace + "/" + mod.Name

logger := logger.WithValues("module", moduleNameKey)

// skipping handling NMC spec module until node is ready
if !r.nodeAPI.IsNodeSchedulable(&node, mod.Config.Tolerations) {
readyLabelsToRemove = append(readyLabelsToRemove, utils.GetKernelModuleReadyNodeLabel(mod.Namespace, mod.Name))
delete(statusMap, moduleNameKey)
continue
}
Expand Down Expand Up @@ -166,8 +167,8 @@ 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 readyLabelsToRemove != nil {
if err := r.nodeAPI.UpdateLabels(ctx, &node, nil, readyLabelsToRemove); err != nil {
return ctrl.Result{}, fmt.Errorf("could remove node %s labels: %v", node.Name, err)
}
return ctrl.Result{}, nil
Expand Down
24 changes: 15 additions & 9 deletions internal/controllers/nmc_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,8 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
It("should remove kmod labels and not continue if node is not schedulable", func() {
spec0 := kmmv1beta1.NodeModuleSpec{
ModuleItem: kmmv1beta1.ModuleItem{
Namespace: namespace,
Name: "mod0",
Namespace: "test-ns",
Name: "test-module",
},
}
nmc := &kmmv1beta1.NodeModulesConfig{
Expand Down Expand Up @@ -171,9 +171,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().UpdateLabels(ctx, &node, nil, []string{kmodName}).DoAndReturn(
func(_ context.Context, obj ctrlclient.Object, _, _ []string) error {
delete(node.ObjectMeta.Labels, kmodName)
return nil
},
Expand All @@ -186,8 +185,17 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
})

It("should fail to remove kmod labels and not continue if node is not schedulable", func() {
spec0 := kmmv1beta1.NodeModuleSpec{
ModuleItem: kmmv1beta1.ModuleItem{
Namespace: "test-ns",
Name: "test-module",
},
}
nmc := &kmmv1beta1.NodeModulesConfig{
ObjectMeta: metav1.ObjectMeta{Name: nmcName},
Spec: kmmv1beta1.NodeModulesConfigSpec{
Modules: []kmmv1beta1.NodeModuleSpec{spec0},
},
}
node := v1.Node{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -219,8 +227,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().UpdateLabels(ctx, &node, nil, []string{kmodName}).DoAndReturn(
func(_ context.Context, obj ctrlclient.Object, _, _ []string) error {
return fmt.Errorf("some error")
},
),
Expand Down Expand Up @@ -299,7 +307,6 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
nm.EXPECT().IsNodeSchedulable(&node, nil).Return(true),
wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec1, nil, &node),
wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2, &node),
nm.EXPECT().IsNodeSchedulable(&node, nil).Return(true),
wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc),
wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(loaded, unloaded, err),
wh.EXPECT().RecordEvents(&node, loaded, unloaded),
Expand Down Expand Up @@ -378,7 +385,6 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
nm.EXPECT().IsNodeSchedulable(&node, nil).Return(true),
wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec0, &status0, &node).Return(fmt.Errorf(errorMeassge)),
wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2, &node).Return(fmt.Errorf(errorMeassge)),
nm.EXPECT().IsNodeSchedulable(&node, nil).Return(true),
wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc).Return(fmt.Errorf(errorMeassge)),
wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(nil, nil, fmt.Errorf(errorMeassge)),
)
Expand Down
14 changes: 0 additions & 14 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: 0 additions & 16 deletions internal/node/node.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/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 @@ -19,7 +18,6 @@ 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
}

type node struct {
Expand Down Expand Up @@ -87,20 +85,6 @@ 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
71 changes: 10 additions & 61 deletions internal/node/node_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,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 +187,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 +203,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 @@ -318,74 +319,22 @@ 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

BeforeEach(func() {
node = v1.Node{}
})

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

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

BeforeEach(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))
})
})

0 comments on commit e63298a

Please sign in to comment.