Restrict checks for image existence #734

qbarrand · 2024-02-08T11:40:59Z

Before populating the NodeModulesConfig object, only check if built or signed image exist on the registry.
Add a log message when the NodeModulesConfig is not populated for that reason.

/cc @yevgeny-shnaidman @mresvanis

netlify · 2024-02-08T11:41:04Z

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name	Link
🔨 Latest commit	`5f3fbde`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/65ca409678aa6a000845edeb
😎 Deploy Preview	https://deploy-preview-734--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.

k8s-ci-robot · 2024-02-08T11:41:05Z

[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:

~~OWNERS~~ [qbarrand]

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

codecov-commenter · 2024-02-08T11:43:39Z

Codecov Report

Attention: 64 lines in your changes are missing coverage. Please review.

Comparison is base (fa23a9b) 79.09% compared to head (5f3fbde) 78.40%.
Report is 10 commits behind head on main.

Files	Patch %	Lines
internal/controllers/module_nmc_reconciler.go	66.07%	13 Missing and 6 partials ⚠️
internal/webhook/module.go	70.96%	6 Missing and 3 partials ⚠️
...ternal/controllers/build_sign_events_reconciler.go	36.36%	5 Missing and 2 partials ⚠️
...ontrollers/node_label_module_version_reconciler.go	12.50%	7 Missing ⚠️
internal/webhook/namespace.go	61.53%	5 Missing ⚠️
...ternal/controllers/device_plugin_pod_reconciler.go	42.85%	3 Missing and 1 partial ⚠️
internal/controllers/device_plugin_reconciler.go	20.00%	4 Missing ⚠️
internal/controllers/build_sign_reconciler.go	25.00%	3 Missing ⚠️
...rnal/controllers/preflightvalidation_reconciler.go	40.00%	3 Missing ⚠️
internal/meta/labels.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #734      +/-   ##
==========================================
- Coverage   79.09%   78.40%   -0.69%     
==========================================
  Files          51       49       -2     
  Lines        5109     3960    -1149     
==========================================
- Hits         4041     3105     -936     
+ Misses        882      663     -219     
- Partials      186      192       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

internal/controllers/module_nmc_reconciler.go

internal/controllers/module_nmc_reconciler_test.go

Before populating the NodeModulesConfig object, only check if built or signed image exist on the registry. Add a log message when the NodeModulesConfig is not populated for that reason.

mresvanis · 2024-02-12T16:00:51Z

/lgtm

yevgeny-shnaidman · 2024-02-08T11:46:31Z

internal/controllers/module_nmc_reconciler.go

-		// skip updating NMC, reconciliation will kick in once the build pod is completed
-		return nil
+
+	if module.ShouldBeBuilt(mld) || module.ShouldBeSigned(mld) {


In case Module is using pre-built image, we are not checking it existence here, so we might end up spawning the Worker pod that will fail on pulling an image. How about we change this code here, so that we always check image existence, and in case prebuilt image should exist, we log it and return an error? WDYT?

@qbarrand i see that the PR was merged, but have not received answer for the comment

A problem with doing that is that we are going to query the registry a lot. That is likely why we did not do it in the first place.

yevgeny-shnaidman · 2024-02-08T11:49:15Z

internal/controllers/module_nmc_reconciler.go

@@ -357,7 +362,7 @@ func (mnrh *moduleNMCReconcilerHelper) enableModuleOnNode(ctx context.Context, m
 	}

 	opRes, err := controllerutil.CreateOrPatch(ctx, mnrh.client, nmcObj, func() error {
-		err = mnrh.nmcHelper.SetModuleConfig(nmcObj, mld, &moduleConfig)
+		err := mnrh.nmcHelper.SetModuleConfig(nmcObj, mld, &moduleConfig)


why do we need a new variable here? if SetModuleConfig fail, err will be update with its value, and if it is ok then err will be updated anyway with the value of CreateOrPatch. What am i missing?

@qbarrand i see that the PR was merged, but have not received answer the comment

err inside the callback called CreateOrPatch and outside that callback are two different variables.
If SetModuleConfig returns an error, then CreateOrPatch likely returns that error as well. But the line

if err := mnrh.nmcHelper.SetModuleConfig(nmcObj, mld, &moduleConfig); err != nil {

does not affect the outside err.

and why do we care if it affects? eventually outside err will hold the return value of CreateAndPatch, so even if it holds the error code of SeModuleConfig, it will be for a short time, and it won't affect the correctness of the code

Oh, I see what you mean now. Indeed we could have skipped the semicolon here - I added it out of habit because I rewrote the if statement to hold on one line. At best, this adds one pointer on the stack - I don't think it warrants a reverting PR 🙂

sure , no need for additional PR. Just wanted to make sure that i read the code correctly, that there is no hidden issue that i missed that required a new err variable

Before populating the NodeModulesConfig object, only check if built or signed image exist on the registry. Add a log message when the NodeModulesConfig is not populated for that reason.

* Add a webhook for namespace deletion (#719) This commit makes KMM set the kmm.node.k8s.io/contains-modules label on all namespaces that contain at least one Module. It also adds a new webhook to the bundle and the corresponding handler in the manager. The new webhook rejects namespace deletions if the kmm.node.k8s.io/contains-modules label is present on the namespace. This avoids entering situations where the namespace is being deleted and KMM cannot create unloading Pods to honor Module deletion. * Restrict checks for image existence (#734) Before populating the NodeModulesConfig object, only check if built or signed image exist on the registry. Add a log message when the NodeModulesConfig is not populated for that reason. * Make slight changes to the CRDs (#736) Module: make moduleName an optional field. ManagedClusterModule: make spokeNamespace a required field.

k8s-ci-robot requested review from mresvanis and yevgeny-shnaidman February 8, 2024 11:41

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 8, 2024

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2024

mresvanis reviewed Feb 8, 2024

View reviewed changes

internal/controllers/module_nmc_reconciler.go Outdated Show resolved Hide resolved

mresvanis reviewed Feb 8, 2024

View reviewed changes

internal/controllers/module_nmc_reconciler.go Outdated Show resolved Hide resolved

mresvanis reviewed Feb 8, 2024

View reviewed changes

internal/controllers/module_nmc_reconciler_test.go Outdated Show resolved Hide resolved

mresvanis reviewed Feb 8, 2024

View reviewed changes

internal/controllers/module_nmc_reconciler_test.go Outdated Show resolved Hide resolved

Restrict checks for image existence

5f3fbde

Before populating the NodeModulesConfig object, only check if built or signed image exist on the registry. Add a log message when the NodeModulesConfig is not populated for that reason.

qbarrand force-pushed the fix-image-check branch from cf21cc4 to 5f3fbde Compare February 12, 2024 16:00

k8s-ci-robot assigned mresvanis Feb 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2024

k8s-ci-robot merged commit a7709ac into kubernetes-sigs:main Feb 12, 2024

qbarrand deleted the fix-image-check branch February 12, 2024 17:03

yevgeny-shnaidman reviewed Feb 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict checks for image existence #734

Restrict checks for image existence #734

qbarrand commented Feb 8, 2024

netlify bot commented Feb 8, 2024 •

edited

Loading

k8s-ci-robot commented Feb 8, 2024

codecov-commenter commented Feb 8, 2024 •

edited

Loading

mresvanis commented Feb 12, 2024

yevgeny-shnaidman Feb 8, 2024

yevgeny-shnaidman Feb 12, 2024

qbarrand Feb 13, 2024

yevgeny-shnaidman Feb 8, 2024

yevgeny-shnaidman Feb 12, 2024

qbarrand Feb 13, 2024

yevgeny-shnaidman Feb 13, 2024

qbarrand Feb 13, 2024

yevgeny-shnaidman Feb 14, 2024

Restrict checks for image existence #734

Restrict checks for image existence #734

Conversation

qbarrand commented Feb 8, 2024

netlify bot commented Feb 8, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kmm ready!

k8s-ci-robot commented Feb 8, 2024

codecov-commenter commented Feb 8, 2024 • edited Loading

Codecov Report

mresvanis commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Feb 8, 2024 •

edited

Loading

codecov-commenter commented Feb 8, 2024 •

edited

Loading