CNF-14679: Adapt to single H/W manager plugin #220

tliu2021 · 2024-09-26T14:50:07Z

This update introduces the following changes:

Added an environment variable HWMGR_PLUGIN_NAMESPACE to the deployment to specify the namespace for the single plugin.
Introduced a new field hwMgrId in the NodePool spec to identify the plugin adapter.
Added ObservedGeneration fields for both the cloud manager and the hardware manager plugin in the NodePool status.

Note: To continue using the test plugin, use the following commands:
make run HWMGR_PLUGIN_NAMESPACE=oran-hwmgr-plugin-test
make deploy HWMGR_PLUGIN_NAMESPACE=oran-hwmgr-plugin-test

openshift-ci-robot · 2024-09-26T14:50:11Z

@tliu2021: This pull request references CNF-14679 which is a valid jira issue.

In response to this:

This update introduces the following changes:

Added an environment variable HWMGR_PLUGIN_NAMESPACE to the deployment to specify the namespace for the single plugin.

Introduced a new field hwMgrId in the NodePool spec to identify the plugin adapter.

Added ObservedGeneration fields for both the cloud manager and the hardware manager plugin in the NodePool status.

Note: To continue using the test plugin, use the following commands:
make run HWMGR_PLUGIN_NAMESPACE=oran-hwmgr-plugin-test
make deploy HWMGR_PLUGIN_NAMESPACE=oran-hwmgr-plugin-test

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.

openshift-ci · 2024-09-26T14:50:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from tliu2021. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

donpenney · 2024-09-26T17:40:23Z

Makefile

@@ -170,7 +173,7 @@ uninstall: manifests kustomize ## Uninstall CRDs from the K8s cluster specified
 .PHONY: deploy
 deploy: manifests kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config.
 	cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
-	$(KUSTOMIZE) build config/default | $(KUBECTL) apply -f -
+	$(KUSTOMIZE) build config/default | sed 's|\plugin-namespace-placeholder|$(HWMGR_PLUGIN_NAMESPACE)|g' | $(KUBECTL) apply -f -


Is this a common method for doing such customizations? TALM sets a number of env variables by creating a patch.yaml that gets applied by the kustomization.yaml:
https://github.com/openshift-kni/cluster-group-upgrades-operator/blob/main/Makefile#L268

I am not sure if the TALM approach is the best option. I'll investigate further to see if there's a better way to handle this.

Kustomize in FluxCD (kustomize.toolkit.fluxcd.io) supports post-build variable substitution, but I don't think it's necessary to introduce that feature just for this. Instead, I'm using a ConfigMap to pass the environment variable to the deployment.

It just seemed like TALM's method of creating a patch with env vars is a little more formal than doing a sed to modify the output of the kustomize build while piping to the apply

I agree, but I believe using a ConfigMap with Kustomize Replacements is also a solid solution for this use case.

donpenney · 2024-09-26T17:44:33Z

internal/controllers/utils/utils.go

+// GetHwMgrPluginNS returns the value of environment variable HWMGR_PLUGIN_NAMESPACE
+func GetHwMgrPluginNS() string {
+	// Ensure that this code only runs once
+	once.Do(func() {


This once.Do is new to me... Very cool!

donpenney · 2024-09-26T18:23:02Z

api/hardwaremanagement/v1alpha1/node_pools.go

@@ -42,6 +42,9 @@ type NodePoolSpec struct {
 	//+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Location Spec",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:text"}
 	LocationSpec `json:",inline"`

+	// HwMgrId is the identifier for the hardware manager plugin adaptor.
+	HwMgrId string `json:"hwMgrId" yaml:"hwMgrId"`


This breaks the current plugin code, as it is adding a new required field. I think add omitempty here may avoid that. We can likely drop the omitempty later once the plugin is updated to know about the new field.

Verified that adding omitempty allows the current plugin code to work, which is referencing the previous version of the CRD, which it causes a failure to update the CR without it, as it would be missing this new field.

Suggested change

HwMgrId string `json:"hwMgrId" yaml:"hwMgrId"`

HwMgrId string `json:"hwMgrId,omitempty"`

Right, need to support the current plugin code.

donpenney · 2024-09-26T18:27:06Z

api/hardwaremanagement/v1alpha1/node_pools.go

@@ -42,6 +42,9 @@ type NodePoolSpec struct {
 	//+operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Location Spec",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:text"}
 	LocationSpec `json:",inline"`

+	// HwMgrId is the identifier for the hardware manager plugin adaptor.
+	HwMgrId string `json:"hwMgrId" yaml:"hwMgrId"`


Is the yaml field necessary? The code seems inconsistent about using it. It's there for some fields, but not all. If it's not needed, if the yaml would default to the json field, we should drop it altogether

The yaml field is not needed and it will be removed.

donpenney · 2024-09-27T18:46:13Z

/retest

donpenney · 2024-09-27T18:47:53Z

/lgtm

donpenney · 2024-09-27T19:21:47Z

/retest

openshift-ci · 2024-09-27T19:28:22Z

@tliu2021: 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/ci-job	`95b09fe`	link	true	`/test ci-job`

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.

Signed-off-by: Tao Liu <tali@redhat.com>

openshift-ci bot requested review from browsell and jhernand September 26, 2024 14:50

donpenney requested review from alegacy, donpenney and rauhersu September 26, 2024 15:31

donpenney reviewed Sep 26, 2024

View reviewed changes

tliu2021 force-pushed the hwMgrId branch from 7c5776c to 95b09fe Compare September 27, 2024 17:59

openshift-ci bot assigned donpenney Sep 27, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2024

donpenney removed the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2024

CNF-14679: Adapt to single H/W manager plugin

7a34823

Signed-off-by: Tao Liu <tali@redhat.com>

tliu2021 force-pushed the hwMgrId branch from 95b09fe to 7a34823 Compare September 30, 2024 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNF-14679: Adapt to single H/W manager plugin #220

CNF-14679: Adapt to single H/W manager plugin #220

tliu2021 commented Sep 26, 2024

openshift-ci-robot commented Sep 26, 2024 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Sep 26, 2024

donpenney Sep 26, 2024

tliu2021 Sep 26, 2024

tliu2021 Sep 27, 2024

donpenney Sep 27, 2024

tliu2021 Sep 27, 2024

donpenney Sep 26, 2024

donpenney Sep 26, 2024

donpenney Sep 26, 2024

tliu2021 Sep 26, 2024

tliu2021 Sep 27, 2024

donpenney Sep 26, 2024

tliu2021 Sep 27, 2024

donpenney commented Sep 27, 2024

donpenney commented Sep 27, 2024

donpenney commented Sep 27, 2024

openshift-ci bot commented Sep 27, 2024

	HwMgrId string `json:"hwMgrId" yaml:"hwMgrId"`
	HwMgrId string `json:"hwMgrId,omitempty"`

CNF-14679: Adapt to single H/W manager plugin #220

Are you sure you want to change the base?

CNF-14679: Adapt to single H/W manager plugin #220

Conversation

tliu2021 commented Sep 26, 2024

openshift-ci-robot commented Sep 26, 2024 • edited by openshift-ci bot Loading

openshift-ci bot commented Sep 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donpenney commented Sep 27, 2024

donpenney commented Sep 27, 2024

donpenney commented Sep 27, 2024

openshift-ci bot commented Sep 27, 2024

openshift-ci-robot commented Sep 26, 2024 •

edited by openshift-ci bot

Loading