Use controllerutil for create or update #121
Conversation
When updating this causes an error like:
```
2025-04-02T18:57:32Z ERROR failed creating ClusterImageSet {"controller": "clusterdeployment", "controllerGroup": "hive.openshift.io", "controllerKind": "ClusterDeployment", "ClusterDeployment": {"name":"test-multinode","namespace":"test-capi"}, "namespace": "test-capi", "name": "test-multinode", "reconcileID": "18ea3061-27ad-4efb-ab87-f9f859a301d9", "error": "clusterimagesets.hive.openshift.io \"test-multinode\" is invalid: metadata.resourceVersion: Invalid value: 0x0: must be specified for an update"}
github.com/openshift-assisted/cluster-api-agent/controlplane/internal/controller.(*ClusterDeploymentReconciler).ensureAgentClusterInstall
/workspace/controlplane/internal/controller/clusterdeployment_controller.go:114
github.com/openshift-assisted/cluster-api-agent/controlplane/internal/controller.(*ClusterDeploymentReconciler).Reconcile
/workspace/controlplane/internal/controller/clusterdeployment_controller.go:91
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Reconcile
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.6/pkg/internal/controller/controller.go:116
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).reconcileHandler
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.6/pkg/internal/controller/controller.go:303
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).processNextWorkItem
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.6/pkg/internal/controller/controller.go:263
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2.2
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.6/pkg/internal/controller/controller.go:224
```
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughThis update refactors parts of the control plane by modifying key reconciliation functions and renaming several image registry functions. In the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar." ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
openshift-ci
bot
added
approved
|
/lgtm |
|
I finally understand why controlleruitls' CreateOrUpdate it's so inconvenient to use.. 😓 We should probably remove our CreateOrUpdate within this PR too |
|
Yeah, there's still one more caller I need to change, but that was my goal. |
openshift-ci
bot
removed
lgtm
carbonin
changed the title
openshift-ci
bot
added
size/XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controlplane/internal/controller/openshiftassistedcontrolplane_controller.go (1)
191-194: Typographical fix needed in log message.
The word "ClusterDeploment" is misspelled on line 192. Consider correcting it to "ClusterDeployment".- log.Error(err, "failed to set OACP ClusterDeploment reference") + log.Error(err, "failed to set OACP ClusterDeployment reference")controlplane/internal/controller/clusterdeployment_controller.go (1)
101-107: Image registry creation logic is triggered conditionally.
Consider naming the helper asensureImageRegistryfor clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
assistedinstaller/clusterdeployment.go(0 hunks)controlplane/internal/controller/clusterdeployment_controller.go(5 hunks)controlplane/internal/controller/openshiftassistedcontrolplane_controller.go(4 hunks)util/suite_test.go(0 hunks)util/util.go(0 hunks)util/util_test.go(0 hunks)
💤 Files with no reviewable changes (4)
- util/suite_test.go
- util/util.go
- assistedinstaller/clusterdeployment.go
- util/util_test.go
🧰 Additional context used
🧬 Code Definitions (1)
controlplane/internal/controller/openshiftassistedcontrolplane_controller.go (2)
util/util.go (1)
ControlPlaneMachineLabelsForCluster(42-61)controlplane/api/v1alpha2/openshiftassistedcontrolplane_types.go (1)
OpenshiftAssistedControlPlane(239-245)
🔇 Additional comments (39)
controlplane/internal/controller/openshiftassistedcontrolplane_controller.go (12)
28-28: No concerns regarding this new import.
38-38: Import looks correct.
40-40: No issues with this added import.
512-513: Confirm necessity of skipping updates.
If theClusterDeploymentRefis already set, the remaining logic is bypassed. Verify that this behavior is intended if changes toacp.Spec.Configoccur after the reference is initially established.
516-517: Override of cluster name is clear and logical.
No issues identified.
520-520: Constructing a new ClusterDeployment object.
No concerns here.
521-525: Metadata assignment looks good.
No issues identified.
527-548: CreateOrPatch usage is appropriate.
The reference tocontrollerutil.SetOwnerReferenceensures correct ownership. Spec fields appear consistent with HPC logic.
549-550: Returning the CreateOrPatch error is clean.
No issues identified.
552-560: Introducing setClusterDeploymentRef helper.
Implementation approach is clear for setting the status reference.
563-563: Logging the Get operation outcome is handled up the call chain.
No issues here.
572-577: Reference extraction ensures accurate status.
Storing a reference to the retrievedClusterDeploymentobject is correct.controlplane/internal/controller/clusterdeployment_controller.go (27)
46-46: Added controllerutil for resource patching/creation.
No issues here.
92-95: Architecture retrieval is straightforward.
Error handling is appropriate.
96-100: Potential naming conflict for ClusterImageSet creation.
UsingclusterDeployment.Nameas theClusterImageSetname might cause collisions if multipleClusterDeploymentobjects share the same name across namespaces. Verify uniqueness or consider a namespaced or hashed scheme.
108-110: EnsureAgentClusterInstall invocation is consistent with the new flow.
No issues identified.
119-120: Signature returns only an error now.
This simplifies error flow.
125-125: Returning early on error.
Implementation is aligned with newly simplified logic.
128-131: Gathering worker node count and cluster config references.
Variables are neatly assembled.
132-136: Instantiating AgentClusterInstall CR.
Construct is straightforward and consistent with hive extension.
139-144: Setting labels for ACI object.
The usage ofutil.ControlPlaneMachineLabelsForClusteris consistent with labeling strategy.
146-146: OwnerReference is properly set with OACP.
This ensures correct ownership in the cluster's resource graph.
148-169: AgentClusterInstall spec definition.
Fields correctly map to relevant fields inOpenshiftAssistedConfig. The logic for forcing baremetal platform when VIPs are set is coherent.
170-170: ACICapabilities call is properly invoked.
No issues identified.
171-173: Error returned if capabilities setting fails.
The fallback logic looks clean.
175-175: CreateOrPatch usage for AgentClusterInstall.
Practice aligns with recommended controller patterns.
176-178: Error handling logs a clear message.
Implementation is consistent with the rest of the file.
227-228: New ensureClusterImageSet function.
Naming conveys intention clearly. No issues identified.
234-238: CreateOrPatch with a function closure for imageSet.
Pattern is appropriate for reconciling cluster-wide resources.
239-239: Returning the final error.
Logic is clean and consistent.
242-242: Utility for retrieving cluster networks.
Function signature correctly returns a pair of networks.
255-256: Returning computed cluster networks.
No issues identified.
258-258: Function retrieving additional manifests references.
Naming is clear; content is consistent with the rest of the code.
268-268: Returning aggregated manifest references.
No issues identified.
271-271: createImageRegistry method introduced.
Implements retrieval of an existing configmap and generating new image registry config if needed.
277-278: Delegates data generation to imageregistry package.
Error handling is appropriate.
282-286: Initialize the new ConfigMap object.
Approach is consistent with the standard K8s resource creation patterns.
288-292: Setting ConfigMap data via CreateOrPatch.
Implementation ensures idempotent creation or update.
293-293: Return the final error.
Conclusion of the creation logic. Looks good.
| if acp.Status.ClusterDeploymentRef != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Wouldn't we still want to go through the create or patch function below even if the cluster deployment ref is set in the ACP's status? Like in the event the ACP's pull secret ref change?
There was a problem hiding this comment.
I agree generally, but this was the behavior before so I just kept it as is for this PR.
There was a problem hiding this comment.
Gotcha, sorry didn't catch that it was prior behavior.
|
One question, but looks great otherwise! |
|
/hold Want to fix one more thing |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
…il should be used in the future
This avoids the case where the controller attempts to update and already altered object which results in an error, it also issues fewer changes to the server as only the fields we want to change are considered an update.
This was attempting to set the `UserManagedNetworking` field to nil
when it had previously been defaulted to some value by the
assisted-service controllers.
This error can be seen in logs like:
```
2025-04-03T16:17:55Z ERROR Reconciler error {"controller": "clusterdeployment", "controllerGroup": "hive.openshift.io", "controllerKind": "ClusterDeployment", "ClusterDeployment": {"name":"test-multinode","namespace":"test-capi"}, "namespace": "test-capi", "name": "test-multinode", "reconcileID": "316eee13-3248-4919-9569-f40026a9a95f", "error": "admission webhook \"agentclusterinstallvalidators.admission.agentinstall.openshift.io\" denied the request: Attempted to change AgentClusterInstall.Spec which is immutable after install started, except for ClusterMetadata,IgnitionEndpoint fields. Unsupported change: \n\tNetworking.UserManagedNetworking: (0xc000988c4c => <nil>)"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).reconcileHandler
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.6/pkg/internal/controller/controller.go:316
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).processNextWorkItem
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.6/pkg/internal/controller/controller.go:263
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2.2
/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.19.6/pkg/internal/controller/controller.go:224
```
carbonin
force-pushed
the
fix-create-or-update
branch
from
96e0af3 to
8a0e537
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controlplane/internal/controller/openshiftassistedcontrolplane_controller.go (2)
511-513: Consider updating on existing references as well.
Returning early if the cluster deployment reference is already set means changes won't propagate if updates are needed (e.g., re-synchronizing with new specs).if acp.Status.ClusterDeploymentRef != nil { - return nil + // Optionally check if the existing reference needs to be updated or re-synced }
519-525: Potential logging enhancement.
Creating a newClusterDeploymentobject is beneficial for visibility. Consider adding a debug/info log before the CreateOrPatch call.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
assistedinstaller/clusterdeployment.go(0 hunks)controlplane/internal/controller/clusterdeployment_controller.go(5 hunks)controlplane/internal/controller/openshiftassistedcontrolplane_controller.go(4 hunks)util/suite_test.go(0 hunks)util/util.go(0 hunks)util/util_test.go(0 hunks)
💤 Files with no reviewable changes (4)
- util/suite_test.go
- assistedinstaller/clusterdeployment.go
- util/util_test.go
- util/util.go
🧰 Additional context used
🧬 Code Definitions (1)
controlplane/internal/controller/clusterdeployment_controller.go (2)
util/util.go (1)
ControlPlaneMachineLabelsForCluster(42-61)controlplane/internal/imageregistry/image_registry.go (2)
ImageConfigMapName(20-20)GenerateImageRegistryData(38-90)
🔇 Additional comments (29)
controlplane/internal/controller/openshiftassistedcontrolplane_controller.go (5)
28-40: New imports appear consistent with the API usage.
Imports forv1alpha2,hiveextension/v1beta1, and the Hiveagentpackage correctly align with the references in this file. No concerns.
190-195: Proper integration withsetClusterDeploymentRef.
This invocation appropriately handles error logging and returning, neatly tying in the newly introduced function that sets the reference in the ACP status.
515-517: Clear, concise cluster name override.
Ifacp.Spec.Config.ClusterNameis specified, it correctly takes precedence. Implementation is straightforward.
527-550: Safe creation or patch forClusterDeployment.
UtilizingCreateOrPatchand setting necessary fields (owner reference, labels, platform) aligns well with best practices for reconcilers.
552-579: Robust function for setting the ACP status reference.
Good handling of a non-existentClusterDeploymentand clearing the reference if not found. Error handling is clear.controlplane/internal/controller/clusterdeployment_controller.go (24)
46-46: Import forcontrollerutil
This import is necessary for theCreateOrPatchusage throughout the controller.
92-107: Ensuring the cluster image set and optional image registry creation.
• Fetching the architecture before callingensureClusterImageSetis logical.
• The conditional creation of image registry resources based onacp.Spec.Config.ImageRegistryRefis nicely structured.
No critical issues found.
108-112: Final steps in Reconcile flow.
Cleanly ensures the AgentClusterInstall and updates the ClusterDeploymentRef. This clear ordering helps maintainers.
119-119: Refactored signature to return onlyerror.
This simplifies error handling, reducing confusion about separatectrl.Result.
129-129: Determining worker node count.
Extraction intogetWorkerNodesCountis a neat separation of concerns, enhancing readability.
130-131: Retrieving cluster networks.
Storing the networks at this stage supports building the ACI spec properly.
132-138: InitializingAgentClusterInstallresource.
Good approach: using the ClusterDeployment name and namespace for consistency.
139-144: Setting labels & owner references.
Leverages utility functions to keep label logic consistent across the codebase.
148-149: ClusterDeploymentRef assignment in the ACI spec.
Accurately references the parent cluster deployment. Straightforward approach.
150-153: Provision requirements.
Assigning control plane and worker agent counts addresses the bare-metal provisioning logic effectively.
154-157: Configuration assignments.
Applying the user-specified DiskEncryption, MastersSchedulable, Proxy, SSHAuthorizedKey ensures consistent configurations.
158-161: Cluster networking.
PopulatingClusterNetworkandServiceNetworkfrom the retrieved data. No issues.
162-162: Appending manifest config references.
Allows additional cluster manifests to be deployed as part of the installation.
163-167: Bare metal VIP handling.
Manages custom VIP values for APIVIPs and IngressVIPs, switching the platform type toBareMetal.
168-168: Setting capabilities.
The call tosetACICapabilitiesis consistent with the rest of the configuration logic.
174-174:CreateOrPatchfor ACI
Ensures the resource is created or updated in a single transaction. Proper usage.
175-176: Error logging
Logs any failure from the patch operation. Sufficient detail is provided.
220-220: AssigningClusterInstallRef
Points the cluster deployment to theAgentClusterInstallobject, aligning with Hive's relationships.
225-238:ensureClusterImageSet
Creates or patches theClusterImageSetwith the correct release image. Straightforward approach.
240-254: Gathering cluster network CIDR blocks.
Using theCluster.Spec.ClusterNetworkfields for building the ACI's networking arrays.
256-256: Manifest references aggregator function.
Combining user-specified config map references with the registry config map if present.
269-269: Creating image registry.
Fetches the registry configmap and reuses its data to generate a new config for the spoke cluster.
275-275: Generating image registry data.
Leverages the extracted logic inimageregistry.GenerateImageRegistryData, centralizing registry setup.
280-291: Constructing or patching the final ConfigMap.
Populatingcm.Datawith the generated config ensures that the image registry info is correctly conveyed to the cluster.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, CrystalChun, rccrdpccl 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 |
openshift-merge-bot
bot
merged commit 52da144
into
openshift-assisted:master
The current create or update function causes problems when called for an update with a resource that was freshly created in code (not queried from the existing cluster).
This is solved by using
controllerutil.CreateOrPatchwhich handles doing this query, applying changes, and only updating if changes were made.This PR updates all our uses of the existing util function to the
controllerutilversion as well as cleans up some existing functions to ensure 😉 they're only doing one thing.WIP until I take care of the last caller and remove the function.
Summary by CodeRabbit