From 6a736c4112f5abba8fa4fb618e636ad34dc6e8bf Mon Sep 17 00:00:00 2001 From: Yevgeny Shnaidman Date: Tue, 24 Sep 2024 16:03:33 +0300 Subject: [PATCH] Changing Node interface and updating NMC controller This PR is a preparation for the fixes needed for cluster upgrade KMM implementation. We are adding a new function to the node interface, that determines whether a node has been rebooted while a kernel module was loaded. This PR also updates NMC controller to use this API and refactors the unit-test --- internal/controllers/nmc_reconciler.go | 11 +-- internal/controllers/nmc_reconciler_test.go | 79 +++++---------------- internal/node/mock_node.go | 29 ++++---- internal/node/node.go | 28 ++++---- internal/node/node_test.go | 63 ++++++++-------- 5 files changed, 84 insertions(+), 126 deletions(-) diff --git a/internal/controllers/nmc_reconciler.go b/internal/controllers/nmc_reconciler.go index 4cce8386d..4603b4b14 100644 --- a/internal/controllers/nmc_reconciler.go +++ b/internal/controllers/nmc_reconciler.go @@ -350,15 +350,8 @@ func (h *nmcReconcilerHelperImpl) ProcessModuleSpec( return fmt.Errorf("could not get node %s: %v", nmcObj.Name, err) } - readyCondition := h.nodeAPI.FindNodeCondition(node.Status.Conditions, v1.NodeReady) - if readyCondition == nil { - return fmt.Errorf("node %s has no Ready condition", nmcObj.Name) - } - - // node has been rebooted, load the module using the spec - if readyCondition.Status == v1.ConditionTrue && status.LastTransitionTime.Before(&readyCondition.LastTransitionTime) { - logger.Info("Outdated last transition time status; creating loader Pod") - + if h.nodeAPI.NodeBecomeReadyAfter(&node, status.LastTransitionTime) { + logger.Info("node has been rebooted and become ready after kernel module was loaded; creating loader Pod") return h.pm.CreateLoaderPod(ctx, nmcObj, spec) } diff --git a/internal/controllers/nmc_reconciler_test.go b/internal/controllers/nmc_reconciler_test.go index e19a6e789..292c0ea70 100644 --- a/internal/controllers/nmc_reconciler_test.go +++ b/internal/controllers/nmc_reconciler_test.go @@ -397,14 +397,14 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { client *testclient.MockClient pm *MockpodManager wh nmcReconcilerHelper - nm node.Node + nm *node.MockNode ) BeforeEach(func() { ctrl := gomock.NewController(GinkgoT()) client = testclient.NewMockClient(ctrl) pm = NewMockpodManager(ctrl) - nm = node.NewNode(client) + nm = node.NewMockNode(ctrl) wh = newNMCReconcilerHelper(client, pm, nil, nm) }) @@ -535,40 +535,6 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { ) }) - It("should return an error if the node has no ready condition", func() { - nmc := &kmmv1beta1.NodeModulesConfig{ - ObjectMeta: metav1.ObjectMeta{Name: nmcName}, - } - - spec := &kmmv1beta1.NodeModuleSpec{ - ModuleItem: kmmv1beta1.ModuleItem{ - Name: name, - Namespace: namespace, - }, - Config: moduleConfig, - } - - status := &kmmv1beta1.NodeModuleStatus{ - ModuleItem: kmmv1beta1.ModuleItem{ - Name: name, - Namespace: namespace, - }, - Config: moduleConfig, - LastTransitionTime: metav1.Now(), - } - - gomock.InOrder( - pm.EXPECT().GetWorkerPod(ctx, podName, namespace), - client.EXPECT().Get(ctx, types.NamespacedName{Name: nmcName}, &v1.Node{}), - ) - - Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, status), - ).To( - HaveOccurred(), - ) - }) - nmc := &kmmv1beta1.NodeModulesConfig{ ObjectMeta: metav1.ObjectMeta{Name: nmcName}, } @@ -594,28 +560,21 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { DescribeTable( "should create a loader Pod if status is older than the Ready condition", - func(cs v1.ConditionStatus, shouldCreate bool) { + func(shouldCreate bool) { - getPod := pm. - EXPECT(). - GetWorkerPod(ctx, podName, namespace) + returnValue := false + if shouldCreate { + returnValue = true + } - getNode := client. - EXPECT(). - Get(ctx, types.NamespacedName{Name: nmcName}, &v1.Node{}). - Do(func(_ context.Context, _ types.NamespacedName, node *v1.Node, _ ...ctrl.Options) { - node.Status.Conditions = []v1.NodeCondition{ - { - Type: v1.NodeReady, - Status: cs, - LastTransitionTime: now, - }, - } - }). - After(getPod) + gomock.InOrder( + pm.EXPECT().GetWorkerPod(ctx, podName, namespace), + client.EXPECT().Get(ctx, types.NamespacedName{Name: nmcName}, &v1.Node{}), + nm.EXPECT().NodeBecomeReadyAfter(gomock.Any(), status.LastTransitionTime).Return(returnValue), + ) if shouldCreate { - pm.EXPECT().CreateLoaderPod(ctx, nmc, spec).After(getNode) + pm.EXPECT().CreateLoaderPod(ctx, nmc, spec) } Expect( @@ -624,8 +583,8 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { HaveOccurred(), ) }, - Entry(nil, v1.ConditionFalse, false), - Entry(nil, v1.ConditionTrue, true), + Entry("pod status is newer then node's Ready condition, worker pod should not be created", false), + Entry("pod status is older then node's Ready condition, worker pod should be created", true), ) It("should do nothing if the pod is not loading a kmod", func() { @@ -1356,16 +1315,16 @@ var _ = Describe("nmcReconcilerHelperImpl_RecordEvents", func() { var ( client *testclient.MockClient fakeRecorder *record.FakeRecorder - n node.Node - wh nmcReconcilerHelper + //nm *node.MockNode + wh nmcReconcilerHelper ) BeforeEach(func() { ctrl := gomock.NewController(GinkgoT()) client = testclient.NewMockClient(ctrl) - n = node.NewNode(client) + //nm = node.NewMockNode(ctrl) fakeRecorder = record.NewFakeRecorder(10) - wh = newNMCReconcilerHelper(client, nil, fakeRecorder, n) + wh = newNMCReconcilerHelper(client, nil, fakeRecorder, nil) }) closeAndGetAllEvents := func(events chan string) []string { diff --git a/internal/node/mock_node.go b/internal/node/mock_node.go index 74308443e..0d0556d40 100644 --- a/internal/node/mock_node.go +++ b/internal/node/mock_node.go @@ -14,6 +14,7 @@ import ( gomock "go.uber.org/mock/gomock" v1 "k8s.io/api/core/v1" + v10 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // MockNode is a mock of Node interface. @@ -39,20 +40,6 @@ func (m *MockNode) EXPECT() *MockNodeMockRecorder { return m.recorder } -// FindNodeCondition mocks base method. -func (m *MockNode) FindNodeCondition(cond []v1.NodeCondition, conditionType v1.NodeConditionType) *v1.NodeCondition { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "FindNodeCondition", cond, conditionType) - ret0, _ := ret[0].(*v1.NodeCondition) - return ret0 -} - -// FindNodeCondition indicates an expected call of FindNodeCondition. -func (mr *MockNodeMockRecorder) FindNodeCondition(cond, conditionType any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindNodeCondition", reflect.TypeOf((*MockNode)(nil).FindNodeCondition), cond, conditionType) -} - // GetNodesListBySelector mocks base method. func (m *MockNode) GetNodesListBySelector(ctx context.Context, selector map[string]string) ([]v1.Node, error) { m.ctrl.T.Helper() @@ -97,6 +84,20 @@ func (mr *MockNodeMockRecorder) IsNodeSchedulable(node any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsNodeSchedulable", reflect.TypeOf((*MockNode)(nil).IsNodeSchedulable), node) } +// NodeBecomeReadyAfter mocks base method. +func (m *MockNode) NodeBecomeReadyAfter(node *v1.Node, checkTime v10.Time) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "NodeBecomeReadyAfter", node, checkTime) + ret0, _ := ret[0].(bool) + return ret0 +} + +// NodeBecomeReadyAfter indicates an expected call of NodeBecomeReadyAfter. +func (mr *MockNodeMockRecorder) NodeBecomeReadyAfter(node, checkTime any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NodeBecomeReadyAfter", reflect.TypeOf((*MockNode)(nil).NodeBecomeReadyAfter), node, checkTime) +} + // UpdateLabels mocks base method. func (m *MockNode) UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeRemoved []string) error { m.ctrl.T.Helper() diff --git a/internal/node/node.go b/internal/node/node.go index 8e7f6d54e..f30ec897d 100644 --- a/internal/node/node.go +++ b/internal/node/node.go @@ -5,16 +5,19 @@ import ( "fmt" "github.com/rh-ecosystem-edge/kernel-module-management/internal/meta" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" ) +//go:generate mockgen -source=node.go -package=node -destination=mock_node.go + type Node interface { IsNodeSchedulable(node *v1.Node) bool GetNodesListBySelector(ctx context.Context, selector map[string]string) ([]v1.Node, error) GetNumTargetedNodes(ctx context.Context, selector map[string]string) (int, error) - FindNodeCondition(cond []v1.NodeCondition, conditionType v1.NodeConditionType) *v1.NodeCondition UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeRemoved []string) error + NodeBecomeReadyAfter(node *v1.Node, checkTime metav1.Time) bool } type node struct { @@ -63,18 +66,6 @@ func (n *node) GetNumTargetedNodes(ctx context.Context, selector map[string]stri return len(targetedNode), nil } -func (n *node) FindNodeCondition(cond []v1.NodeCondition, conditionType v1.NodeConditionType) *v1.NodeCondition { - for i := 0; i < len(cond); i++ { - c := cond[i] - - if c.Type == conditionType { - return &c - } - } - - return nil -} - func (n *node) UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeRemoved []string) error { patchFrom := client.MergeFrom(node.DeepCopy()) @@ -87,6 +78,17 @@ func (n *node) UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeR return nil } +func (n *node) NodeBecomeReadyAfter(node *v1.Node, timestamp metav1.Time) bool { + conds := node.Status.Conditions + for i := 0; i < len(conds); i++ { + c := conds[i] + if c.Type == v1.NodeReady && c.Status == v1.ConditionTrue && timestamp.Before(&c.LastTransitionTime) { + return true + } + } + return false +} + func addLabels(node *v1.Node, labels []string) { for _, label := range labels { meta.SetLabel( diff --git a/internal/node/node_test.go b/internal/node/node_test.go index cd141a44e..a492d6b9c 100644 --- a/internal/node/node_test.go +++ b/internal/node/node_test.go @@ -10,6 +10,7 @@ import ( "go.uber.org/mock/gomock" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "time" ) var _ = Describe("IsNodeSchedulable", func() { @@ -229,50 +230,52 @@ var _ = Describe("GetNumTargetedNodes", func() { }) }) -var _ = Describe("FindNodeCondition", func() { +var _ = Describe("NodeBecomeReadyAfter", func() { var ( - ctrl *gomock.Controller - clnt *client.MockClient - mn Node - conditionFound *v1.NodeCondition + n Node + testNode v1.Node + timestamp time.Time ) BeforeEach(func() { - ctrl = gomock.NewController(GinkgoT()) - clnt = client.NewMockClient(ctrl) - mn = NewNode(clnt) - }) - - It("Finds condition", func() { - condition := v1.NodeCondition{ - Type: v1.NodeReady, - } - node := v1.Node{ - Status: v1.NodeStatus{ - Conditions: []v1.NodeCondition{ - condition, - }, - }, - } - conditionFound = mn.FindNodeCondition(node.Status.Conditions, v1.NodeReady) - Expect(conditionFound).To(Equal(&condition)) - }) - - It("Does not find condition", func() { - node := v1.Node{ + n = NewNode(nil) + timestamp = time.Now() + testNode = v1.Node{ Status: v1.NodeStatus{ Conditions: []v1.NodeCondition{ + { + Type: v1.NodeMemoryPressure, + }, { Type: v1.NodeDiskPressure, }, { - Type: v1.NodeMemoryPressure, + Type: v1.NodeReady, + }, + { + Type: v1.NodeNetworkUnavailable, }, }, }, } - conditionFound = mn.FindNodeCondition(node.Status.Conditions, v1.NodeReady) - Expect(conditionFound).To(BeNil()) + }) + + It("ready condition is true, its timestamp is after the test timestamp", func() { + testTimestamp := metav1.NewTime(timestamp) + testNode.Status.Conditions[2].Status = v1.ConditionTrue + testNode.Status.Conditions[2].LastTransitionTime = metav1.NewTime(timestamp.Add(2 * time.Minute)) + + res := n.NodeBecomeReadyAfter(&testNode, testTimestamp) + Expect(res).To(BeTrue()) + }) + + It("ready condition is true, its timestamp is before the test timestamp", func() { + testTimestamp := metav1.NewTime(timestamp.Add(2 * time.Minute)) + testNode.Status.Conditions[2].Status = v1.ConditionTrue + testNode.Status.Conditions[2].LastTransitionTime = metav1.NewTime(timestamp) + + res := n.NodeBecomeReadyAfter(&testNode, testTimestamp) + Expect(res).To(BeFalse()) }) })