Introducing Module-NMC controller #503

yevgeny-shnaidman · 2023-08-03T13:56:57Z

This PR introduces a new controller, that is responsible for reconciliation of NMC based on the data in Module and Node The following are the main points:

It watches for changes in Modules, Nodes (labels, becoming
schedulable) and Jobs belonging owned by Modules (in order to
reschedule once Build/Sign job are finished)
Per reconciled Module, go over all the nodes in the cluster
and reconcile their NMCs based on the Module/Node state

Changes in the PR:

adding functions to filter package that are resposible for filtering
the events (nodes. modules, jobs) and finding out which modules
should be reconciled
changing the NMC handler logic a little, adding verification for
container image presence in case module should be enable.
Module-NMC Reconciliation is being done both in case Kernel Mapping
exists and in case it does not exists ( solves the garbage
collection flow)

This PR does not include Module status updates based on NMC updates

k8s-ci-robot · 2023-08-03T13:57:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yevgeny-shnaidman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yevgeny-shnaidman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2023-08-03T13:57:08Z

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name	Link
🔨 Latest commit	`70b755e`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/64d0eab8559ebb0008d58675
😎 Deploy Preview	https://deploy-preview-503--kubernetes-sigs-kmm.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

yevgeny-shnaidman · 2023-08-03T13:57:14Z

/assign @ybettan

yevgeny-shnaidman · 2023-08-03T13:57:31Z

/assign @qbarrand

ybettan · 2023-08-06T07:51:13Z

cmd/manager/main.go

+	mnc := controllers.NewModuleNMCReconciler(
+		client,
+		kernelAPI,
+		nmcHandler,


Should this one be renamed to nmcAPI to be consistent with the rest of the API passed to it (or to the Module reconciler)?

This is a controller, we are not creating/returning an Interface ( API), but the struct itself, as the rest of the reconcilers/controllers, that's why there is no API in the name

nmcHandler is a public interface. Why don't we call it nmcAPI instead?

moved the NMC-Handler API into the controller, since it implements controller logic

ybettan · 2023-08-06T07:53:26Z

controllers/module_nmc_reconciler.go

+	"sigs.k8s.io/controller-runtime/pkg/log"
+)
+
+//+kubebuilder:rbac:groups="core",resources=nodes,verbs=get;watch;patch


Why do we need to patch nodes?

no need, removing

ybettan · 2023-08-06T07:53:44Z

controllers/module_nmc_reconciler.go

+)
+
+//+kubebuilder:rbac:groups="core",resources=nodes,verbs=get;watch;patch
+//+kubebuilder:rbac:groups=kmm.sigs.x-k8s.io,resources=nodemodulesconfigs,verbs=get;list;watch;update;patch;create


What is the difference between update and patch?

that depends on the action that you execute on the object. I prefer to mention both of them, so that we can execute them in the code without a need to update the rules

What's the problem with updating the rules? A new version of the code will come with a new bundle anyway. I would restrict the verbs here to the bare minimum.

removed the "update"

ybettan · 2023-08-06T07:58:03Z

controllers/module_nmc_reconciler.go

+	// get the reconciled module
+	logger := log.FromContext(ctx)
+
+	logger.Info("Starting Module-NMS reconcilation", "module name and namespace", req.NamespacedName)


Who says that the reconciled object is a Module? Can't it be a Node? a Job?

because this is how this reconciler is defined. It is defined For Module, and for every "watch" it finds the appropriate Module to reconcile

ybettan · 2023-08-06T08:00:38Z

controllers/module_nmc_reconciler.go

+			logger.Info("Module deleted")
+			return ctrl.Result{}, nil
+		}
+		return ctrl.Result{}, fmt.Errorf("failed to get the requested %s KMMO CR: %w", req.NamespacedName, err)


Suggested change

return ctrl.Result{}, fmt.Errorf("failed to get the requested %s KMMO CR: %w", req.NamespacedName, err)

return ctrl.Result{}, fmt.Errorf("failed to get the requested %s KMMO CR: %v", req.NamespacedName, err)

Are we using the wrapped error anywhere?

Fixed. We do use wrapped errors , when needed. For example in the KernelMapper, to get the appropriate MLD, if exists

I meant, "do we use the wrapped error in this case". If you fixed it I assume we were not using it :)

yes, in this case it was a mistake

ybettan · 2023-08-06T08:04:11Z

controllers/module_nmc_reconciler.go

+	errs := make([]error, 0, len(nodes))
+	for _, node := range nodes {
+		kernelVersion := strings.TrimSuffix(node.Status.NodeInfo.KernelVersion, "+")
+		mld, err := mnr.kernelAPI.GetModuleLoaderDataForKernel(mod, kernelVersion)


Why do we need to do that for every node? Can we extract it outside of the loop?

because the kernel version of the nodes may differ

But I don't see node being passed to this function or the implementation depending on the nodes in any way. Am I missing something?

the node is used to determine the kernelVersion , and to determine kernel mapping (using kernel version). Then both kernel mapping(mld) and node are passed to the function that determines whether the module needs to run on the node or not

ybettan · 2023-08-06T08:05:13Z

controllers/module_nmc_reconciler.go

+		}
+
+		err = mnr.nmcHandler.HandleModuleNMC(ctx, mod, mld, node)
+		errs = append(errs, err)


Aren't you appending nil errors here as well?

yes, but errors.Join knows how to handle them and ignore

Won't it be clearer to only append real errors instead of delegating it to Join?

why to add another "if", if there is already code in the Join that takes care of it? This is the purpose of Join function: decide of there are errors in the array of errors , and create the appropriate error

why to add another "if", if there is already code in the Join that takes care of it?

Because it is not trivial for someone who didn't read the Join doc to understand those are ignored. Everywhere else in the code we are checking if an error is != nil before doing something with it except here.

If you think it is better this way, let's keep it, I just though it is a bit confusing.

i am keeping it: if we have a Golang library function in the code, i would expect that developer to read about it and understand

ybettan · 2023-08-06T08:10:26Z

controllers/module_nmc_reconciler.go

+	getRequestedModule(ctx context.Context, namespacedName types.NamespacedName) (*kmmv1beta1.Module, error)
+	getNodesList(ctx context.Context) ([]v1.Node, error)


I am wondering why getRequestedModule isn't a method in the module package and why we don't have a node package that will hold getNodeList as well.

As discussed in Slack, i am leaving it here and we can decide on it and the rest of functions in packages in v2.1

ybettan · 2023-08-06T09:03:47Z

controllers/module_nmc_reconciler.go

+	}
+}
+
+func (mnrh *moduleNMCReconcilerHelper) getRequestedModule(ctx context.Context, namespacedName types.NamespacedName) (*kmmv1beta1.Module, error) {


Isn't this function a dup of getRequestedModule in the module_reconciler.go file?

As discussed in Slack, i am leaving it here and we can decide on it and the rest of functions in packages in v2.1

ybettan · 2023-08-06T09:05:18Z

controllers/module_nmc_reconciler.go

+	return &mod, nil
+}
+
+func (mnrh *moduleNMCReconcilerHelper) getNodesList(ctx context.Context) ([]v1.Node, error) {


Same here, this function exist (almost) in module_reconciler.go

As discussed in Slack, i am leaving it here and we can decide on it and the rest of functions in packages in v2.1

Wondering if we need this function at all? In tests, we replace one EXPECT() with another - not sure what is the benefit. I guess that remark is also cvalid for getRequestedModule above.

I prefer to set such function in a helper interface, makes it easier to write the unit-tests for the main reconciliation loop, and to check all the corner case in a dedicated unit-test for that specific function.

cmd/manager/main.go

qbarrand · 2023-08-07T06:40:08Z

controllers/module_nmc_reconciler.go

+)
+
+//+kubebuilder:rbac:groups="core",resources=nodes,verbs=get;watch;patch
+//+kubebuilder:rbac:groups=kmm.sigs.x-k8s.io,resources=nodemodulesconfigs,verbs=get;list;watch;update;patch;create


What's the problem with updating the rules? A new version of the code will come with a new bundle anyway. I would restrict the verbs here to the bare minimum.

controllers/module_nmc_reconciler.go

qbarrand · 2023-08-07T06:45:26Z

controllers/module_nmc_reconciler.go

+	return &mod, nil
+}
+
+func (mnrh *moduleNMCReconcilerHelper) getNodesList(ctx context.Context) ([]v1.Node, error) {


Wondering if we need this function at all? In tests, we replace one EXPECT() with another - not sure what is the benefit. I guess that remark is also cvalid for getRequestedModule above.

internal/filter/filter.go

qbarrand · 2023-08-07T09:52:45Z

/test pull-kernel-module-management-unit-tests

internal/filter/filter.go

yevgeny-shnaidman · 2023-08-07T10:48:32Z

/test pull-kernel-module-management-unit-tests

internal/filter/filter.go

qbarrand · 2023-08-07T12:13:07Z

controllers/module_nmc_reconciler.go

+		Watches(
+			&batchv1.Job{},
+			handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &kmmv1beta1.Module{}, handler.OnlyControllerOwner()),
+			builder.WithPredicates(
+				filter.ModuleNMCReconcileJobPredicate(),
+			),
+		).


Can we use Owns instead?

Suggested change

Watches(

&batchv1.Job{},

handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &kmmv1beta1.Module{}, handler.OnlyControllerOwner()),

builder.WithPredicates(

filter.ModuleNMCReconcileJobPredicate(),

),

).

Owns(

&batchv1.Job{},

builder.WithPredicates(

filter.ModuleNMCReconcileJobPredicate(),

),

).

No, Owns defines it own filter and handler. Since i would like to keep the filter, i am still using Watches

qbarrand · 2023-08-07T12:43:28Z

/lgtm

This PR introduces a new controller, that is responsible for reconciliation of NMC based on the data in Module and Node The following are the main points: 1) It watches for changes in Modules, Nodes (labels, becoming schedulable) and Jobs belonging owned by Modules (in order to reschedule once Build/Sign job are finished) 2) Per reconciled Module, go over all the nodes in the cluster and reconcile their NMCs based on the Module/Node state Changes in the PR: 1) adding functions to filter package that are resposible for filtering the events (nodes. modules, jobs) and finding out which modules should be reconciled 2) changing the NMC handler logic a little, adding verification for container image presence in case module should be enable. 3) Module-NMC Reconciliation is being done both in case Kernel Mapping exists and in case it does not exists ( solves the garbage collection flow) This PR does not include Module status updates based on NMC updates

qbarrand · 2023-08-07T13:02:33Z

/lgtm

Build using Go 1.18. Upstream-Commit: 4c76f4d

…igs#694) This PR introduces a new controller, that is responsible for reconciliation of NMC based on the data in Module and Node The following are the main points: 1) It watches for changes in Modules, Nodes (labels, becoming schedulable) and Jobs belonging owned by Modules (in order to reschedule once Build/Sign job are finished) 2) Per reconciled Module, go over all the nodes in the cluster and reconcile their NMCs based on the Module/Node state Changes in the PR: 1) adding functions to filter package that are resposible for filtering the events (nodes. modules, jobs) and finding out which modules should be reconciled 2) changing the NMC handler logic a little, adding verification for container image presence in case module should be enable. 3) Module-NMC Reconciliation is being done both in case Kernel Mapping exists and in case it does not exists ( solves the garbage collection flow) This PR does not include Module status updates based on NMC updates

k8s-ci-robot requested review from mresvanis and ybettan August 3, 2023 13:57

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 3, 2023

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 3, 2023

k8s-ci-robot assigned ybettan Aug 3, 2023

k8s-ci-robot assigned qbarrand Aug 3, 2023

ybettan reviewed Aug 6, 2023

View reviewed changes

yevgeny-shnaidman force-pushed the yevgeny/module-nmc-controller branch from d65402f to b1c21c6 Compare August 6, 2023 12:33

qbarrand reviewed Aug 7, 2023

View reviewed changes

yevgeny-shnaidman force-pushed the yevgeny/module-nmc-controller branch from b1c21c6 to 2b37582 Compare August 7, 2023 09:43

qbarrand reviewed Aug 7, 2023

View reviewed changes

internal/filter/filter.go Show resolved Hide resolved

internal/filter/filter.go Show resolved Hide resolved

yevgeny-shnaidman force-pushed the yevgeny/module-nmc-controller branch 2 times, most recently from c6e518a to 09f9ade Compare August 7, 2023 11:37

qbarrand reviewed Aug 7, 2023

View reviewed changes

internal/filter/filter.go Show resolved Hide resolved

qbarrand reviewed Aug 7, 2023

View reviewed changes

yevgeny-shnaidman force-pushed the yevgeny/module-nmc-controller branch 3 times, most recently from ebc5e47 to fcb00c9 Compare August 7, 2023 12:40

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2023

yevgeny-shnaidman force-pushed the yevgeny/module-nmc-controller branch from fcb00c9 to 70b755e Compare August 7, 2023 12:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2023

k8s-ci-robot merged commit 9bd2ba4 into kubernetes-sigs:main Aug 7, 2023

qbarrand added a commit to qbarrand/kernel-module-management that referenced this pull request Nov 8, 2023

Add a CodeQL GitHub Action (kubernetes-sigs#503)

aa32b29

Build using Go 1.18. Upstream-Commit: 4c76f4d

	return ctrl.Result{}, fmt.Errorf("failed to get the requested %s KMMO CR: %w", req.NamespacedName, err)
	return ctrl.Result{}, fmt.Errorf("failed to get the requested %s KMMO CR: %v", req.NamespacedName, err)

		getRequestedModule(ctx context.Context, namespacedName types.NamespacedName) (*kmmv1beta1.Module, error)
		getNodesList(ctx context.Context) ([]v1.Node, error)

Introducing Module-NMC controller #503

Introducing Module-NMC controller #503

Uh oh!

Conversation

yevgeny-shnaidman commented Aug 3, 2023

Uh oh!

k8s-ci-robot commented Aug 3, 2023

Uh oh!

netlify bot commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Uh oh!

yevgeny-shnaidman commented Aug 3, 2023

Uh oh!

yevgeny-shnaidman commented Aug 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

netlify bot commented Aug 3, 2023 •

edited

Loading