AROSLSRE-687: propagate HCP Azure resource ID annotation to control plane namespace - #8312
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the exported constant HCPAzureResourceIDAnnotation with value "azure.microsoft.com/hcp-cluster-azure-resource-id". The HostedCluster controller reconciliation propagates that annotation from an Azure-platform HostedCluster CR to the hosted control plane Namespace: it initializes namespace.Annotations if nil, writes the annotation value when present on the HostedCluster, and removes the annotation from the Namespace when the HostedCluster no longer has it. Non-Azure platforms are not affected; other Namespace annotations are preserved. Sequence Diagram(s)sequenceDiagram
participant HC as HostedCluster CR
participant Ctrl as HostedCluster Controller
participant API as Kubernetes API (ControlPlane Namespace)
HC->>Ctrl: Reconcile event / Read HostedCluster
Ctrl->>Ctrl: Check platform == Azure?
alt Azure platform
alt annotation present on HC
Ctrl->>API: GET ControlPlane Namespace
API-->>Ctrl: Namespace object
Ctrl->>Ctrl: Ensure namespace.Annotations initialized
Ctrl->>API: PATCH/UPDATE Namespace with HCPAzureResourceIDAnnotation
API-->>Ctrl: 200 OK
else annotation absent on HC
Ctrl->>API: GET ControlPlane Namespace
API-->>Ctrl: Namespace object
Ctrl->>Ctrl: Remove HCPAzureResourceIDAnnotation from Namespace.Annotations (if present)
Ctrl->>API: PATCH/UPDATE Namespace
API-->>Ctrl: 200 OK
end
else Non-Azure platform
Ctrl-->>API: No annotation written or removed
end
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
openshift-ci
Bot
added
area/api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 6583-6590: The test currently duplicates the Azure resourceID
annotation propagation logic inline; instead invoke the real reconciliation path
(call HostedClusterReconciler.Reconcile or the production helper used by the
controller) using the fake client with the test's HostedCluster (hcluster) and
Namespace (ns) fixtures, then fetch the Namespace from the fake client and
assert that ns.Annotations[hyperv1.HCPAzureResourceIDAnnotation] matches the
HostedCluster annotation; update the test to remove the inline propagation block
and reference HostedClusterReconciler.Reconcile (or the controller helper
function) and the hcluster/ns fixtures so the assertion verifies controller
behavior end-to-end.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 1413-1421: The reconcile currently only copies
hyperv1.HCPAzureResourceIDAnnotation from the HostedCluster to the
controlPlaneNamespace when present, but never deletes it when the source
annotation is removed; update the logic in the reconcile block handling
hcluster.Spec.Platform == hyperv1.AzurePlatform (around the
controlPlaneNamespace update) to check for the annotation on the HostedCluster
and if missing remove hyperv1.HCPAzureResourceIDAnnotation from
controlPlaneNamespace (ensure controlPlaneNamespace.Annotations is non-nil
before modifying), then persist the change (e.g., via the existing client
update/patch call used for controlPlaneNamespace) so stale values are cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a1bf189c-2644-4dbd-8566-929d11783745
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (3)
api/hypershift/v1beta1/hostedcluster_types.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8312 +/- ##
==========================================
+ Coverage 41.75% 41.80% +0.05%
==========================================
Files 758 759 +1
Lines 93981 94049 +68
==========================================
+ Hits 39240 39316 +76
+ Misses 51988 51983 -5
+ Partials 2753 2750 -3
... and 5 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/approve For API |
621ee27 to
468c3dc
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
e5be0cf to
4088e5f
Compare
|
/retitle AROSLSRE-687: propagate HCP Azure resource ID annotation to control plane namespace |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ashishmax31: This pull request references AROSLSRE-687 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 story 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. |
|
/approve |
|
You will need to force push in order to retest the env test for KAS. |
openshift-ci
Bot
added
the
approved
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks
|
Scheduling tests matching the |
|
/test e2e-aks-4-22 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest-required |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/override "ci/prow/e2e-kubevirt-aws-ovn-reduced" This job is permafailing and I pinged the kubevirt folks earlier here - https://redhat-internal.slack.com/archives/C0B71V5TDMW/p1781616231915999 |
|
@bryan-cox: Overrode contexts on behalf of bryan-cox: ci/prow/e2e-kubevirt-aws-ovn-reduced 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 kubernetes-sigs/prow repository. |
|
/verified |
|
@ashishmax31: The 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. |
|
/hold putting a hold for @enxebre API approval |
openshift-ci
Bot
added
the
do-not-merge/hold
…ane namespace Add a new annotation constant HCPAzureResourceIDAnnotation that carries the Azure resource ID set by Cluster Service on the HostedCluster CR. The hostedcluster controller now propagates this annotation to the hosted control plane namespace for Azure platform clusters. Refer https://redhat.atlassian.net/browse/AROSLSRE-687 and https://issues.redhat.com/browse/AROSLSRE-447 for more details. Signed-off-by: Ashish <asnaraya@redhat.com>
ba9711a to
651fed3
Compare
|
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashishmax31, bryan-cox, JoelSpeed, jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by @ashishmax31 |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by 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. |
|
/retest |
|
All four agents confirm the same root cause. All four jobs failed identically — pod scheduling timeouts on the Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll four Prow jobs failed due to a transient CI infrastructure capacity issue on the Root CauseThe root cause is CI infrastructure resource exhaustion on the All four jobs were created between
Preemption was also ineffective: 107–109 nodes had no helpful preemption candidates, 2 still had insufficient memory even after preemption, and 4 had no viable preemption victims. Since the pods never started, no Recommendations
Evidence
|
4f6659c
into
openshift:main
|
@ashishmax31: all tests passed! 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. |
7 participants
What this PR does / why we need it:
Add a new annotation constant HCPAzureResourceIDAnnotation that carries
the Azure resource ID set by Cluster Service on the HostedCluster CR.
The hostedcluster controller now propagates this annotation to the
hosted control plane namespace for Azure platform clusters.
Fixes
https://redhat.atlassian.net/browse/AROSLSRE-687
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
Tests