Update multi-arch validator to check node affinity for multi-platform environments #276

jaypoulz · 2023-01-28T00:10:35Z

Adding a validator to image references for nodeAffinity specifications.
Will check for the following for the CSV images:

Missing node affinity boundaries
Incomplete node affinity boundaries (image supports more than node affinity)
Extra node affinity settings that are not supported by the image data

In doing so I've built upon the work done by @camilamacedo86.

jaypoulz · 2023-01-31T20:25:05Z

/assign awgreene
I believe that an extension to this which also scans for node affinity discrepancies in the rest of the bundle is desirable, but I wanted to get this patch up for early feedback.

jaypoulz · 2023-01-31T20:25:19Z

/unhold

gallettilance

Looks good! Don't have much to say - everything is clear and tests are extensive.

awgreene

Looks good, but I'd like to confirm that warnings do not prevent an operator from being published. Thanks for the contribution.

awgreene · 2023-02-03T19:23:09Z

pkg/validation/internal/multiarch.go

+		var os = make([]string, 0)
+
+		for _, e := range t.MatchExpressions {
+			if e.Operator != "In" {


This won't support anti-affinity, which I think is fine given the goal of this change.

The multiarch validator is used in the operator-framework/audit tool to produce a report on the health of operators and is also available for self-validation for operator-sdk authors. It will not block operator publishing. :)

awgreene · 2023-02-03T19:27:30Z

pkg/validation/internal/multiarch.go

@@ -297,12 +375,12 @@ func (data *multiArchValidator) inspectImages(images map[string][]platform) map[
 			if err != nil {
 				data.warns = append(data.warns, fmt.Errorf("unable to inspect the image (%s) : %s", k, err))

-				// We set the Arch and SO as error for we are able to deal witth these cases further
+				// We set the Arch and OS as error for we are able to deal witth these cases further


Suggested change

// We set the Arch and OS as error for we are able to deal witth these cases further

// We set the Arch and OS as error for we are able to deal with these cases further

awgreene · 2023-02-03T19:30:17Z

pkg/validation/internal/multiarch.go

 // checkMissingSupportForOtherImages checks if any image is missing some arch or so found
-// (probably the image should support the arch or SO )
-func (data *multiArchValidator) checkMissingSupportForOtherImages() {
-	for image, plaformFromImage := range data.allOtherImages {
+// (probably the image should support the arch or OS )
+func (data *multiArchValidator) checkMissingSupportForOtherImages(images map[string][]platform) {
+	for image, platformFromImage := range images {


Note: No functional changes were made in this function other than the new argument.

Yes - the function is now called twice (once for the non-manager containers, and then again for the relatedImage containers).
So it seemed simpler to me to parameterize the image list.

awgreene · 2023-02-03T19:32:29Z

pkg/validation/internal/multiarch.go

+			// Ignore the case when the Platform.Architecture == "error" since that means
+			// that was not possible to inspect the image
+			if imageData.Architecture == "error" {
+				imagePlatformDataValid = false
+				break
+			}


Could you explain why it is okay not to log an error here? Is it logged elsewhere when the image fails to be inspected?

It is - specifically it is logged at inspection time in line 382.
https://github.com/operator-framework/api/pull/276/files#diff-20ba073b3d34ec5ee84564e23371a9a0653ca0528bd97fe76837efa1289d8c88R382

jaypoulz · 2023-02-17T22:11:52Z

Some notes on this PR since I've been evaluating it via the operator-sdk instructions as well as trying to run it through the operator-framework/audit tool:

The audit report is going to flag all operators as those with warnings since by default, the operator scaffolding does not provide a kube-rbac-proxy node affinity specification. I think this may need to be updated as part of operator-framework/operator-sdk#5983 (or another PR focused on providing more opinionated scaffolding). While this report is technically accurate, it does muddy the signal for the 40 or so operators in the redhat-operator-index that have otherwise been compliant as per Camila's original report. I don't think we should remove it or skip that image, but I do think that it would be good to add nodeAffinity for this image to the scaffolding so future operators don't hit this.

jaypoulz · 2023-04-05T14:47:45Z

/retest

openshift-ci · 2023-04-05T14:48:01Z

@jaypoulz: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

awgreene · 2023-04-10T20:22:23Z

/approve

openshift-ci · 2023-04-10T20:22:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, jaypoulz

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~~ [awgreene]

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

dtfranz · 2023-04-10T22:08:11Z

/lgtm
Thanks for taking this on!

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2023

openshift-ci bot requested review from anik120 and ecordell January 28, 2023 00:10

jaypoulz marked this pull request as draft January 28, 2023 00:10

jaypoulz force-pushed the multi-arch-validator branch 5 times, most recently from 2653a75 to ada51af Compare January 31, 2023 20:15

jaypoulz marked this pull request as ready for review January 31, 2023 20:16

openshift-ci bot requested a review from joelanford January 31, 2023 20:16

openshift-ci bot assigned awgreene Jan 31, 2023

jaypoulz changed the title ~~WIP: Began writing validator for nodeAffinity~~ Update multi-arch validator with node affinity validation for multi-platform environments Jan 31, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2023

jaypoulz changed the title ~~Update multi-arch validator with node affinity validation for multi-platform environments~~ Update multi-arch validator to check node affinity for multi-platform environments Jan 31, 2023

jaypoulz force-pushed the multi-arch-validator branch from ada51af to 0345a47 Compare February 1, 2023 15:33

gallettilance approved these changes Feb 3, 2023

View reviewed changes

awgreene reviewed Feb 3, 2023

View reviewed changes

jaypoulz mentioned this pull request Feb 23, 2023

✨MIXEDARCH-242 - Updated multi-arch report to use the operator-framework/api multi-arch validator. operator-framework/audit#144

Merged

Updated .gitignore to ignore vscode files.

4a73cc9

jaypoulz force-pushed the multi-arch-validator branch 3 times, most recently from d64584e to 228bde0 Compare March 14, 2023 21:20

Added validation for node affinity boundaries in the CSV.

d1bfcb6

jaypoulz force-pushed the multi-arch-validator branch from 228bde0 to d1bfcb6 Compare March 16, 2023 19:08

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2023

openshift-ci bot assigned dtfranz Apr 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2023

openshift-merge-robot merged commit 0ab3deb into operator-framework:master Apr 10, 2023

jaypoulz mentioned this pull request Apr 12, 2023

MIXEDARCH-80: Guide on using node affinity to safely schedule pods in heterogeneous clusters. operator-framework/operator-sdk#5983

Merged

jaypoulz deleted the multi-arch-validator branch April 12, 2023 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update multi-arch validator to check node affinity for multi-platform environments #276

Update multi-arch validator to check node affinity for multi-platform environments #276

jaypoulz commented Jan 28, 2023 •

edited

Loading

jaypoulz commented Jan 31, 2023

jaypoulz commented Jan 31, 2023

gallettilance left a comment

awgreene left a comment

awgreene Feb 3, 2023

jaypoulz Feb 17, 2023

awgreene Feb 3, 2023

awgreene Feb 3, 2023

jaypoulz Mar 16, 2023

awgreene Feb 3, 2023

jaypoulz Mar 14, 2023

jaypoulz commented Feb 17, 2023

jaypoulz commented Apr 5, 2023

openshift-ci bot commented Apr 5, 2023

awgreene commented Apr 10, 2023

openshift-ci bot commented Apr 10, 2023

dtfranz commented Apr 10, 2023

	// We set the Arch and OS as error for we are able to deal witth these cases further
	// We set the Arch and OS as error for we are able to deal with these cases further

Update multi-arch validator to check node affinity for multi-platform environments #276

Update multi-arch validator to check node affinity for multi-platform environments #276

Conversation

jaypoulz commented Jan 28, 2023 • edited Loading

jaypoulz commented Jan 31, 2023

jaypoulz commented Jan 31, 2023

gallettilance left a comment

Choose a reason for hiding this comment

awgreene left a comment

Choose a reason for hiding this comment

awgreene Feb 3, 2023

Choose a reason for hiding this comment

jaypoulz Feb 17, 2023

Choose a reason for hiding this comment

awgreene Feb 3, 2023

Choose a reason for hiding this comment

awgreene Feb 3, 2023

Choose a reason for hiding this comment

jaypoulz Mar 16, 2023

Choose a reason for hiding this comment

awgreene Feb 3, 2023

Choose a reason for hiding this comment

jaypoulz Mar 14, 2023

Choose a reason for hiding this comment

jaypoulz commented Feb 17, 2023

jaypoulz commented Apr 5, 2023

openshift-ci bot commented Apr 5, 2023

awgreene commented Apr 10, 2023

openshift-ci bot commented Apr 10, 2023

dtfranz commented Apr 10, 2023

jaypoulz commented Jan 28, 2023 •

edited

Loading