fix(mgmt-agent): watch acn.azure.com CRDs and Nodes (MTPNC, NNC) - #6665
fix(mgmt-agent): watch acn.azure.com CRDs and Nodes (MTPNC, NNC)#6665Andrew Denton (ventifus) wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ventifus 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 |
|
Hi Andrew Denton (@ventifus). Thanks for your PR. I'm waiting for a Azure member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
There was a problem hiding this comment.
Pull request overview
This PR updates the mgmt-agent resource watcher’s API group suffix filter to include the broader acn.azure.com domain, so additional ACN networking CRDs (notably MTPNC and NNC) will be discovered and snapshotted while preserving coverage of existing multitenancy.acn.azure.com resources via suffix matching.
Changes:
- Replace
multitenancy.acn.azure.comwithacn.azure.comin the watcher’swatchedGroupSuffixes. - Update the
matchesGroupSuffixunit test to reflect the new suffix list entry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| mgmt-agent/pkg/controller/resourcewatcher.go | Broadens the watched API group suffix from multitenancy.acn.azure.com to acn.azure.com so both the apex group and its subdomains are included. |
| mgmt-agent/pkg/controller/resourcewatcher_test.go | Adjusts TestMatchesGroupSuffix to validate matching for acn.azure.com. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9f56eca to
2fd199c
Compare
| {"multicluster.openshift.io", true}, | ||
| {"multitenancy.acn.azure.com", true}, | ||
| {"acn.azure.com", true}, | ||
| {"velero.io", true}, |
There was a problem hiding this comment.
Good catch, restored - multitenancy.acn.azure.com is back in as a subdomain-match test case for acn.azure.com.
2fd199c to
509028e
Compare
509028e to
959ea5f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
mgmt-agent/pkg/controller/resourcewatcher_test.go:40
- TestMatchesGroupSuffix no longer exercises the important subdomain case for the new suffix (e.g.
multitenancy.acn.azure.comshould still matchacn.azure.com). Without this, a regression in thestrings.HasSuffix(group, "."+suffix)logic foracn.azure.comwouldn’t be caught.
{"multicluster.openshift.io", true},
{"acn.azure.com", true},
{"multitenancy.acn.azure.com", true},
| gvrs = append(gvrs, | ||
| schema.GroupVersionResource{Group: "", Version: "v1", Resource: "namespaces"}, | ||
| schema.GroupVersionResource{Group: "", Version: "v1", Resource: "nodes"}, | ||
| ) |
There was a problem hiding this comment.
I think this is based on old kube (?). It was true at some point in the past that node status was incredibly noisy, but they fixed it AFAICT - https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/589-efficient-node-heartbeats
IMO - if a Node status changes (regardless of frequency), SRE will really care about that, especially since it's expected that Node outages or remediations will be our primary course of action. This data is likely the most valuable thing for us to ship to logs - highlighting issues with capacity and connectivity.
In any case, I'm convinced Nodes will generate way less traffic than Events or Pods, which we already track.
What
Expands
watchedGroupSuffixesfrommultitenancy.acn.azure.comto the entireacn.azure.comgroup, to pick up MultiTenantPodNetworkConfig and NodeNetworkConfig. Also addscore/v1/nodesto the hardcoded resources watched alongsidecore/v1/namespaces.Why
The SWIFT networking TSG (ARO-25382) needs MTPNC, NNC, and Node state for triage, but none of it was being snapshotted into
kubernetesResourceSnapshots.