AROSLSRE-687: propagate HCP Azure resource ID annotation to control plane namespace#8312
AROSLSRE-687: propagate HCP Azure resource ID annotation to control plane namespace#8312ashishmax31 wants to merge 1 commit into
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8312 +/- ##
==========================================
+ Coverage 41.27% 41.45% +0.18%
==========================================
Files 755 756 +1
Lines 93446 93660 +214
==========================================
+ Hits 38566 38823 +257
+ Misses 52148 52114 -34
+ Partials 2732 2723 -9
... and 11 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 |
ashishmax31
force-pushed
the
AROSLSRE-687
branch
from
621ee27 to
468c3dc
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
ashishmax31
force-pushed
the
AROSLSRE-687
branch
2 times, most recently
from
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashishmax31, 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 |
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
|
@jparrill Addressed the review comments. PTAL |
|
/test images |
|
/retest |
| // This annotation is consumed by ARO-HCP logging and observability components to correlate the | ||
| // HostedCluster with the corresponding Azure resources. | ||
| func propagateAzureResourceIDAnnotation(hcluster *hyperv1.HostedCluster, ns *corev1.Namespace) { | ||
| if hcluster.Spec.Platform.Type != hyperv1.AzurePlatform { |
There was a problem hiding this comment.
We should use the IsAroHcp check here or whatever is replacing it. Actually why not just move that check up on L1400?
cc @muraee
There was a problem hiding this comment.
@jparrill asked to handle this case too: #8312 (comment)
Tiny thing: if a namespace somehow already has this annotation but the cluster isn't Azure, the early return leaves it sitting there forever. Platform type is immutable in practice so it's not really a bug, but a
deletebefore the return would be strictly cleaner. Up to you.
If we move the check up then it'll be ugly to add this check
There was a problem hiding this comment.
There is no IsAroHcp method on the HostedCluster
There was a problem hiding this comment.
There is. We use it all over the place. Here's an example -
ashishmax31
force-pushed
the
AROSLSRE-687
branch
4 times, most recently
from
bc21498 to
c98e0a5
Compare
…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>
|
Now I can see the full picture. The alignment issues are in multiple places:
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Verify workflow failed because the test file Root CauseThe newly added Specifically, the struct type definition uses space-padded alignment: tests := []struct {
name string // 15 spaces between name and type
isAroHCP bool // 11 spaces
hcluster *hyperv1.HostedCluster // 11 spaces
nsAnnotations map[string]string // 6 spaces
expectedAnnotation string // 1 space
}And the struct literal initializations also use space-based alignment: name: "..." // 5 spaces after name:
nsAnnotations: nil // 6 spaces after nsAnnotations:
The author likely used an editor that inserted spaces for alignment instead of tabs, or manually padded fields for visual alignment without running Recommendations
Evidence
|
|
@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. |
| // on the HostedCluster CR to the control plane namespace, or removes it if no longer present. | ||
| // This annotation is consumed by ARO-HCP logging and observability components to correlate the | ||
| // HostedCluster with the corresponding Azure resources. | ||
| func propagateAzureResourceIDAnnotation(hcluster *hyperv1.HostedCluster, ns *corev1.Namespace) { |
There was a problem hiding this comment.
Can't this be simplified to just see if it is ARO HCP and then either set it or delete the annotation?
5 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