MGMT-24834: Standalone cluster import fails — assisted-service fails to set ClusterDeployment.spec.installed after Day1 completion - #10693
Conversation
…to set ClusterDeployment.spec.installed after Day1 completion
openshift-ci-robot
added
the
jira/valid-reference
|
@rivkyrizel: This pull request references MGMT-24557 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 bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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. |
WalkthroughThe controller now marks a cluster deployment installed only after metadata updates succeed, returns persistence errors through status updates, and adds reconciliation coverage for Day1 completion. ChangesCluster installation completion
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rivkyrizel The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
rivkyrizel
changed the title
|
@rivkyrizel: This pull request references MGMT-24834 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 bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/controllers/clusterdeployments_controller_test.go (1)
2388-2413: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a failure-path regression test for the installation gate.
This test covers only successful metadata updates, so it would still pass if
Spec.Installedwere persisted before metadata failure. Add a case that makes credential/secret metadata persistence fail and assert thatClusterDeployment.Spec.Installedremains false and the expected status error is recorded.Based on the supplied PR objective to gate installed-state persistence on successful metadata updates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/controllers/clusterdeployments_controller_test.go` around lines 2388 - 2413, Add a separate failure-path test alongside the successful “should set ClusterDeployment.Spec.Installed” case that makes credential or secret metadata persistence fail, then reconcile and assert Spec.Installed remains false and the expected status error is recorded. Configure the relevant mock expectation to return the failure while preserving the installation-gate setup and verify the resulting ClusterDeployment status condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/controller/controllers/clusterdeployments_controller_test.go`:
- Around line 2388-2413: Add a separate failure-path test alongside the
successful “should set ClusterDeployment.Spec.Installed” case that makes
credential or secret metadata persistence fail, then reconcile and assert
Spec.Installed remains false and the expected status error is recorded.
Configure the relevant mock expectation to return the failure while preserving
the installation-gate setup and verify the resulting ClusterDeployment status
condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7562b0fe-9abe-4d5b-89c7-cccb1862cb79
📒 Files selected for processing (2)
internal/controller/controllers/clusterdeployments_controller.gointernal/controller/controllers/clusterdeployments_controller_test.go
|
/retest-required |
|
@rivkyrizel: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
You can review https://github.com/openshift/hive/blob/master/docs/enhancements/cluster-install-apis.md to understand the integration a bit better. If there are issues with this flag or moving conditions to the clusterdeployment from the cluster install, check the hive logs or ask that team for help.
| if err != nil { | ||
| log.WithError(err).Error("failed to update cluster metadata") | ||
| } else { | ||
| clusterDeployment.Spec.Installed = true |
There was a problem hiding this comment.
We should not do this. This is hive's job based on the conditions.
3 participants
When a standalone cluster completes Day1 installation,
handleClusterInstalled()inclusterdeployments_controller.goupdates theAgentClusterInstallmetadata and status conditions but never setsClusterDeployment.Spec.Installed = true.The assisted-service reconciler doesn't notice because
isInstalled()has a fallback that checks the AgentClusterInstall condition, but external components like Hive and MCE check Spec.Installed directly - since it remains false, standalone cluster import fails.The fix sets
clusterDeployment.Spec.Installed = trueand persists the ClusterDeployment after successful metadata update, matching the existing pattern inlocal_cluster_import_controller.go. A unit test was added to verify Spec.Installed is set to true after Day1 completion.List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
Bug Fixes
Tests