Skip to content

Commit

Permalink
Changing Node interface and updating NMC controller (#1220)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@yevgeny-shnaidman
yevgeny-shnaidman authored Sep 25, 2024
1 parent 6bda6ab commit aed4d51
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 125 deletions.
11 changes: 2 additions & 9 deletions internal/controllers/nmc_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
75 changes: 16 additions & 59 deletions internal/controllers/nmc_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})

Expand Down Expand Up @@ -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},
}
Expand All @@ -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(
Expand All @@ -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() {
Expand Down Expand Up @@ -1356,16 +1315,14 @@ var _ = Describe("nmcReconcilerHelperImpl_RecordEvents", func() {
var (
client *testclient.MockClient
fakeRecorder *record.FakeRecorder
n node.Node
wh nmcReconcilerHelper
)

BeforeEach(func() {
ctrl := gomock.NewController(GinkgoT())
client = testclient.NewMockClient(ctrl)
n = node.NewNode(client)
fakeRecorder = record.NewFakeRecorder(10)
wh = newNMCReconcilerHelper(client, nil, fakeRecorder, n)
wh = newNMCReconcilerHelper(client, nil, fakeRecorder, nil)
})

closeAndGetAllEvents := func(events chan string) []string {
Expand Down
29 changes: 15 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.

28 changes: 15 additions & 13 deletions internal/node/node.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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())

Expand All @@ -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(
Expand Down
63 changes: 33 additions & 30 deletions internal/node/node_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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())
})
})

Expand Down

0 comments on commit aed4d51

Please sign in to comment.