CCXDEV-12875: Enable Insight Operator entitlements for multi arch clusters #1066

opokornyy · 2025-01-21T12:43:55Z

This PR implements the gathering of architectures used by nodes and the retrieval of entitlement certificates for each architecture in use. These certificates are then stored in secrets.

If only one architecture is present, the secret is named etc-pki-entitlement.
If multiple architectures are present, secrets are created with names like etc-pki-entitlement-ARCH, where ARCH represents the specific architecture.

Sample Archive

The archive won't change with this feature

Documentation

No documentation update

Unit Tests

pkg/ocm/sca/architectures_gather_test.go
pkg/ocm/sca/sca_test.go

Privacy

Yes. There are no sensitive data in the newly collected information.

Changelog

No

Breaking Changes

Yes/No

References

https://issues.redhat.com/browse/CCXDEV-12875

openshift-ci-robot · 2025-01-21T12:44:00Z

@opokornyy: This pull request references CCXDEV-12875 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.19.0" version, but no target version was set.

In response to this:

This PR implements the gathering of architectures used by nodes and the retrieval of entitlement certificates for each architecture in use. These certificates are then stored in secrets.

If only one architecture is present, the secret is named etc-pki-entitlement.
If multiple architectures are present, secrets are created with names like etc-pki-entitlement-ARCH, where ARCH represents the specific architecture.

Categories

Bugfix

Data Enhancement

Feature

Backporting

Others (CI, Infrastructure, Documentation)

Sample Archive

The archive won't change with this feature

Documentation

No documentation update

Unit Tests

pkg/ocm/sca/architectures_gather_test.go

pkg/ocm/sca/sca_test.go

Privacy

Yes. There are no sensitive data in the newly collected information.

Changelog

No

Breaking Changes

Yes/No

References

https://issues.redhat.com/browse/CCXDEV-12875

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 openshift-eng/jira-lifecycle-plugin repository.

opokornyy · 2025-01-21T12:46:03Z

pkg/ocm/sca/architectures_gather.go

+)
+
+// Mapping of kubernetes architecture labels to the format used by SCA API
+var kubernetesArchMapping = map[string]string{


I am not sure if we can use only the map or if we need a function to return a default value when no value is found.

Good question. I think it would make sense to use some default value - e.g the arch of node the operator is running on

Totally agree. Let's add a little function to return the arch from operator's node as default value. That could even help with testing.

opokornyy · 2025-01-21T12:50:18Z

There is one case I am not exactly sure how to handle: if a secret is already created and a node with a different architecture is added to the cluster, we will have one secret named etc-pki-entitlement and another with an architecture suffix, both containing the same information. I am not sure if we should, clean up the old secret or take some other action. WDYT?

tremes · 2025-01-21T13:05:36Z

pkg/ocm/sca/sca.go

 		return
 	}

-	klog.Infof("%s secret successfully updated", secretName)
-	c.StatusController.UpdateStatus(controllerstatus.Summary{
+	klog.Info("sca secret successfully updated")


It would be good to keep the secretName in the log message.

I have moved this log into the createSecret and updateSecret where I can log the secret name easily

tremes · 2025-01-21T13:06:47Z

pkg/ocm/sca/sca.go

-	// check & update the secret here
-	err = c.checkSecret(ctx, &ocmRes)
-	if err != nil {
-		klog.Errorf("Error when checking the %s secret: %v", secretName, err)


Same here. There's no sca secret right.

tremes · 2025-01-21T13:11:24Z

pkg/ocm/sca/sca.go

+		if err != nil {
+			klog.Errorf("Unable to decode response: %v", err)
+			return true, err
+		}


I think it would better to move this out of the exponential backoff function call

tremes · 2025-01-21T13:14:08Z

pkg/ocm/sca/sca.go

+	architectures, err := c.gatherArchitectures(ctx)
+	if err != nil {
+		klog.Warningf("Gathering nodes architectures failed: %s", err.Error())
+	}


Do we want to continue in case of this error? It seems the architecture will be nil so I am wondering what will happen with the request.

It will most likely fail on a request to the api with a 400 http return code, so at least it would save one request if we return early here

opokornyy · 2025-01-23T12:11:40Z

/test e2e-gcp-ovn-techpreview

opokornyy · 2025-01-23T12:24:37Z

/test okd-scos-e2e-aws-ovn

prb112 · 2025-01-23T12:40:09Z

pkg/ocm/sca/architectures_gather.go

+	"ppc":     "ppc",
+	"ppc64":   "ppc64",


Suggested change

"ppc": "ppc",

"ppc64": "ppc64",

"ppc": "ppc",

"ppc64": "ppc64",

only ppc64le is supported. There are no plans to support Big Endian

I have used all architectures that are supported by AMS api. But you are right that OCM right now supports only 4 architectures.
It was also discussed here: https://docs.google.com/document/d/1kT8uzjbmTTN2Zyhfo1jhMNR8q3PhkNJ9yGx83FOFL8k/edit?pli=1&disco=AAABZjOhM1A

prb112 · 2025-01-23T12:41:04Z

pkg/ocm/sca/sca.go

+	"ppc":     "ppc",
+	"ppc64":   "ppc64",


Same comment on ppc/ppc64 support
We only support ppc64le

prb112 · 2025-01-23T12:42:47Z

pkg/ocm/sca/architectures_gather.go

+
+	architectures := make(map[string]struct{})
+	for i := range nodes.Items {
+		nodeArch := nodes.Items[i].Status.NodeInfo.Architecture


There are some situations where AutoScale MachineSet from zero where the secondary architecture is not present when this code 'could' run.

@aleskandro you might be interested in this and have some insights.

Autoscale from zero should work and be architecture-aware with AWS, Azure and GCP IPI clusters.

It is not supported on BM infrastructures (IPI included).. I've never looked into the IBM Machine API providers though.

I'll get a better look at this PR asap

Signed-off-by: Ondrej Pokorny <[email protected]>

opokornyy · 2025-01-28T07:24:17Z

/test e2e-gcp-ovn-techpreview

opokornyy · 2025-01-28T07:24:39Z

/test insights-operator-e2e-tests

openshift-ci · 2025-01-28T09:57:56Z

@opokornyy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-scos-e2e-aws-ovn	`0e67f85`	link	false	`/test okd-scos-e2e-aws-ovn`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

tremes · 2025-01-29T12:44:21Z

I didn't try this, but the changes look good. Thank you!
/approve
/lgtm

openshift-ci · 2025-01-29T12:44:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: opokornyy, tremes

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~~ [opokornyy,tremes]

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

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 21, 2025

openshift-ci bot requested review from ncaak and tremes January 21, 2025 12:44

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2025

opokornyy commented Jan 21, 2025

View reviewed changes

tremes reviewed Jan 21, 2025

View reviewed changes

opokornyy mentioned this pull request Jan 23, 2025

OCPBUGS-38727: Enable Insight Operator entitlements for multi arch clusters #979

Closed

5 tasks

prb112 reviewed Jan 23, 2025

View reviewed changes

pkg/ocm/sca/sca.go

Comment on lines +38 to +40

"ppc": "ppc",

"ppc64": "ppc64",

Copy link

prb112 Jan 23, 2025

Choose a reason for hiding this comment

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

Same comment on ppc/ppc64 support
We only support ppc64le

prb112 reviewed Jan 23, 2025

View reviewed changes

joselsegura and others added 10 commits January 27, 2025 15:09

Gather architectures from k8s API

d9ffd6b

feat: get certificates for multi arch clusters

ec764aa

Signed-off-by: Ondrej Pokorny <[email protected]>

feat: test getting sca certificates

fe11ac5

Signed-off-by: Ondrej Pokorny <[email protected]>

feat: change sca endpoint in config

e699f72

Signed-off-by: Ondrej Pokorny <[email protected]>

chore: fix linter errors

9bee173

Signed-off-by: Ondrej Pokorny <[email protected]>

feat: improve logging

800bc39

Signed-off-by: Ondrej Pokorny <[email protected]>

feat: add default value for node arch mapping

02188fd

Signed-off-by: Ondrej Pokorny <[email protected]>

feat: add permissions to list nodes

79f5e10

Signed-off-by: Ondrej Pokorny <[email protected]>

feat: improve error handlling

bcbbd4f

Signed-off-by: Ondrej Pokorny <[email protected]>

refactor: sca response processing tests

0e67f85

Signed-off-by: Ondrej Pokorny <[email protected]>

opokornyy force-pushed the CCX12875-multi-arch branch from ee43090 to 0e67f85 Compare January 27, 2025 14:09

openshift-ci bot assigned tremes Jan 29, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCXDEV-12875: Enable Insight Operator entitlements for multi arch clusters #1066

CCXDEV-12875: Enable Insight Operator entitlements for multi arch clusters #1066

opokornyy commented Jan 21, 2025

openshift-ci-robot commented Jan 21, 2025 •

edited by openshift-ci bot

Loading

Categories

Sample Archive

Documentation

Unit Tests

Privacy

Changelog

Breaking Changes

References

opokornyy Jan 21, 2025

tremes Jan 21, 2025

ncaak Jan 22, 2025

opokornyy commented Jan 21, 2025

tremes Jan 21, 2025

opokornyy Jan 23, 2025

tremes Jan 21, 2025

tremes Jan 21, 2025

tremes Jan 21, 2025

opokornyy Jan 21, 2025

opokornyy commented Jan 23, 2025

opokornyy commented Jan 23, 2025

prb112 Jan 23, 2025

opokornyy Jan 23, 2025

prb112 Jan 23, 2025

prb112 Jan 23, 2025

aleskandro Jan 23, 2025 •

edited

Loading

opokornyy commented Jan 28, 2025

opokornyy commented Jan 28, 2025

openshift-ci bot commented Jan 28, 2025

tremes commented Jan 29, 2025

openshift-ci bot commented Jan 29, 2025

CCXDEV-12875: Enable Insight Operator entitlements for multi arch clusters #1066

Are you sure you want to change the base?

CCXDEV-12875: Enable Insight Operator entitlements for multi arch clusters #1066

Conversation

opokornyy commented Jan 21, 2025

Categories

Sample Archive

Documentation

Unit Tests

Privacy

Changelog

Breaking Changes

References

openshift-ci-robot commented Jan 21, 2025 • edited by openshift-ci bot Loading

Categories

Sample Archive

Documentation

Unit Tests

Privacy

Changelog

Breaking Changes

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opokornyy commented Jan 21, 2025

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

opokornyy commented Jan 23, 2025

opokornyy commented Jan 23, 2025

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

aleskandro Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

opokornyy commented Jan 28, 2025

opokornyy commented Jan 28, 2025

openshift-ci bot commented Jan 28, 2025

tremes commented Jan 29, 2025

openshift-ci bot commented Jan 29, 2025

openshift-ci-robot commented Jan 21, 2025 •

edited by openshift-ci bot

Loading

aleskandro Jan 23, 2025 •

edited

Loading