Skip to content

Add the NodeModulesConfig controller #513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 10, 2023
Merged

Add the NodeModulesConfig controller #513

merged 1 commit into from
Aug 10, 2023

Conversation

qbarrand
Copy link
Contributor

@qbarrand qbarrand commented Aug 8, 2023 •
edited
Loading

Reconcile NodeModulesConfig resources by creating and monitoring worker Pods.
Use the NodeModulesConfig status to maintain the state of modules on nodes.
Use the plural form for the NodeModulesConfig CRD, per convention.

/cc @yevgeny-shnaidman @ybettan

Split into several PRs:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 8, 2023
@netlify
Copy link

netlify bot commented Aug 8, 2023 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name Link
🔨 Latest commit c6d15bf
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/64d4f0b7d1947c0008c2d1e2
😎 Deploy Preview https://deploy-preview-513--kubernetes-sigs-kmm.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qbarrand

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:

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 8, 2023
@qbarrand
Copy link
Contributor Author

qbarrand commented Aug 8, 2023

/cc @mresvanis

@k8s-ci-robot k8s-ci-robot requested a review from mresvanis August 8, 2023 13:15
@qbarrand qbarrand linked an issue Aug 8, 2023 that may be closed by this pull request
Add the NodeModulesConfig controller #466
Closed
@qbarrand
Copy link
Contributor Author

qbarrand commented Aug 8, 2023

/hold
Extracting some files from this PR to make it easier to review.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2023
@qbarrand qbarrand force-pushed the add-nmc-reconciler branch 3 times, most recently from 667432e to 02ab0bd Compare August 8, 2023 14:42
@qbarrand
Copy link
Contributor Author

qbarrand commented Aug 8, 2023

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

@qbarrand
Copy link
Contributor Author

qbarrand commented Aug 8, 2023

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2023
yevgeny-shnaidman
yevgeny-shnaidman reviewed Aug 9, 2023
View reviewed changes
cmd/manager/main.go

ctx := ctrl.SetupSignalHandler()

if err = controllers.NewNodeModulesConfigReconciler(client, workerHelper).SetupWithManager(ctx, mgr); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename it to NewNMCReconciler? we already have NMC package, and ModuleNMC controller, it seems that it is a clearer name then the whole shabang...

controllers/nodemodulesconfig_reconciler.go

//go:generate mockgen -source=nodemodulesconfig_reconciler.go -package=controllers -destination=mock_nodemodulesconfig_reconciler.go WorkerHelper

type WorkerHelper interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it a public interface? Why not make it private? you can create in NewNodeModulesConfigReconciler and also in unit test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, unit tests would belong to a different package...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it is easier and more convenient to have unit-test in the same package, and all our "helper" interfaces already implemented as private

cmd/manager/main.go
@@ -163,6 +164,17 @@ func main() {
cmd.FatalError(setupLogger, err, "unable to create controller", "name", controllers.ModuleNMCReconcilerName)
}

workerHelper := controllers.NewWorkerHelper(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to create it in main if you make it a private interface

controllers/nodemodulesconfig_reconciler.go Outdated
return fmt.Errorf("node %s has no Ready condition", nmc.Name)
}

if status.LastTransitionTime.Before(&readyCondition.LastTransitionTime) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment describing the scenarion (why we need to compare times) would be beneficial here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you can add that the reason to compare the times is in order to catch the node reboot scenario, it would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved the comment. Does it look OK to you?

@qbarrand qbarrand force-pushed the add-nmc-reconciler branch 3 times, most recently from d31e620 to d23a95a Compare August 10, 2023 14:10
@qbarrand
Add the NodeModulesConfig controller
c6d15bf 
Reconcile NodeModulesConfig resources by creating and monitoring worker
Pods.
Use the NodeModulesConfig status to maintain the state of modules on
nodes.
@qbarrand qbarrand force-pushed the add-nmc-reconciler branch from d23a95a to c6d15bf Compare August 10, 2023 14:14
@yevgeny-shnaidman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit 04d0c1c into kubernetes-sigs:main Aug 10, 2023
@qbarrand qbarrand deleted the add-nmc-reconciler branch August 10, 2023 15:25
qbarrand pushed a commit to qbarrand/kernel-module-management that referenced this pull request Nov 8, 2023
@enriquebelarte
Fix set (kubernetes-sigs#513)
549004a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yevgeny-shnaidman yevgeny-shnaidman yevgeny-shnaidman left review comments

@ybettan ybettan Awaiting requested review from ybettan

@mresvanis mresvanis Awaiting requested review from mresvanis

Assignees

@yevgeny-shnaidman yevgeny-shnaidman

Labels
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the NodeModulesConfig controller
3 participants
@qbarrand @k8s-ci-robot @yevgeny-shnaidman