RFE-9138: feat: label some hosted control plane services#8298
RFE-9138: feat: label some hosted control plane services#8298ingvagabund wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ingvagabund: This pull request references RFE-9138 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 feature request 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. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (16)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe changes add metadata labels to three Kubernetes Service resources. In control-plane-operator/manifests/infra.go, OAuth and Konnectivity Services now include labels 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
|
/retest-required |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@control-plane-operator/controllers/hostedcontrolplane/manifests/infra.go`:
- Around line 144-146: The KonnectivityServerService resource's metadata Labels
map currently uses "app: konnectivity-agent" which mismatches the service
selector produced by kasLabels(); update the Labels in the
KonnectivityServerService definition (infra.go, the Labels map in the service
metadata) to use "app: kube-apiserver" so the resource labels reflect the actual
selector used by kasLabels() and remain consistent at reconciliation time.
🪄 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: bab30a8d-90ad-4b47-8043-248f6a5a7bfc
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/manifests/infra.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/ignition-server/service.yaml
| Labels: map[string]string{ | ||
| "app": "konnectivity-agent", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify konnectivity service selector and deployment labels
echo "=== Searching for konnectivity-server service full specification ==="
rg -n -A 20 "func.*KonnectivityServerService" --type=go
echo -e "\n=== Searching for konnectivity service selector configuration ==="
rg -n "konnectivity.*selector" --type=go --type=yaml -C 3
echo -e "\n=== Searching for konnectivity-server deployment/daemonset ==="
fd -e yaml -e go "konnectivity-server" -x echo "File: {}" \; -x head -30 {}
echo -e "\n=== Searching for konnectivity-agent deployment/daemonset ==="
fd -e yaml -e go "konnectivity-agent" -x echo "File: {}" \; -x head -30 {}
echo -e "\n=== Checking if service selector gets populated elsewhere ==="
rg -n "KonnectivityServerService.*Spec.*Selector" --type=go -C 5Repository: openshift/hypershift
Length of output: 10600
🏁 Script executed:
#!/bin/bash
# Find the kasLabels() function definition and context
echo "=== Searching for kasLabels() function definition ==="
rg -n "func kasLabels" --type=go -A 5
echo -e "\n=== Checking kas/service.go for context around selector assignment ==="
rg -n "svc.Spec.Selector = kasLabels" --type=go -B 3 -A 5
echo -e "\n=== Searching for kasLabels usage in other services ==="
rg -n "kasLabels()" --type=go -B 2 -A 2
echo -e "\n=== Checking if metadata labels are used elsewhere ==="
rg -n "konnectivity-agent" --type=go -C 3Repository: openshift/hypershift
Length of output: 47414
Change metadata labels to match the actual service selector.
The KonnectivityServerService has metadata labels set to app: konnectivity-agent, but the service selector is set to app: kube-apiserver (via kasLabels() in the reconciler at kas/service.go:281). This mismatch is misleading—the metadata labels should reflect what the selector actually targets. Update the labels in infra.go lines 144-145 to app: kube-apiserver for consistency with the selector set at reconciliation time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@control-plane-operator/controllers/hostedcontrolplane/manifests/infra.go`
around lines 144 - 146, The KonnectivityServerService resource's metadata Labels
map currently uses "app: konnectivity-agent" which mismatches the service
selector produced by kasLabels(); update the Labels in the
KonnectivityServerService definition (infra.go, the Labels map in the service
metadata) to use "app: kube-apiserver" so the resource labels reflect the actual
selector used by kasLabels() and remain consistent at reconciliation time.
There was a problem hiding this comment.
This is invalid. The label used to identify the service is different from the selector it has to select specific pods.
It does make sense to have a label to find the service and it does not change its current behavior when routing to pods with label app: kube-apiserver.
There was a problem hiding this comment.
@clebs, you're absolutely right, and I apologize for the confusion. I incorrectly conflated two distinct Kubernetes concepts:
metadata.labels— labels on the Service object itself, used by external selectors (like a Metal LBIPAddressPool) to find the Service.spec.selector— used by the Service to route traffic to matching pods.
The label app: konnectivity-agent on the Service metadata is entirely independent of the spec.selector (set to app: kube-apiserver via kasLabels()). Adding the metadata label does not affect pod routing at all. The PR's intent is valid and the implementation is correct.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
ingvagabund
force-pushed
the
label-some-services
branch
2 times, most recently
from
3e8a572 to
80c768b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8298 +/- ##
==========================================
+ Coverage 35.70% 39.39% +3.69%
==========================================
Files 767 773 +6
Lines 93401 94678 +1277
==========================================
+ Hits 33353 37303 +3950
+ Misses 57346 54676 -2670
+ Partials 2702 2699 -3
... and 165 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:
|
|
/retest-required |
|
/lgtm |
|
Scheduling tests matching the |
|
/unassign |
ingvagabund
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
Test Resultse2e-aks
Failed TestsTotal failed tests: 18
... and 13 more failed tests e2e-aws
|
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
@clebs thank you for the review. Is there a CI job from which we can validate the service labels are present in the rendered objects? |
Hi @ingvagabund , I think this is already checked by unit tests given the fixtures you updated with the labels. If the resulting manifests differ from the fixtures unit tests would fail. |
|
/retest |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/retest-required |
| return &corev1.Service{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: konnectivityServerServiceName, | ||
| Namespace: hostedClusterNamespace, |
There was a problem hiding this comment.
Nit: This Service (konnectivity-server) selects KAS pods via kasLabels() — the konnectivity server runs as a sidecar in the KAS pod. Labeling it app: konnectivity-agent is misleading since the agent is the client that connects to this Service, not the backend it exposes. Should this be app: konnectivity-server instead?
There was a problem hiding this comment.
No objection here. I will update the PR.
To allow the services to be listed through a label selector. The service labeling is aligned with the corresponding component labels: - oauth-openshift: app: oauth-openshift (from the component's Deployment spec) - konnectivity-server: app: konnectivity-server - ignition-server: app: ignition-server (from the component's Deployment spec)
ingvagabund
force-pushed
the
label-some-services
branch
from
80c768b to
a692ea4
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ingvagabund 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 |
|
@clebs anyone who can approve the PR? |
|
/lgtm |
|
@ingvagabund according to @openshift-ci-robot, the chosen one to approve is @devguyio |
|
Scheduling tests matching the |
|
/retest-required |
1 similar comment
|
/retest-required |
|
@ingvagabund: The following test 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. |
|
Now I have a complete picture of all four failures. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root Causee2e-aws (Prow): Flaky AWS infrastructure teardown timeout The test framework's
The CLI-side Cross-run evidence confirms flakiness: This PR has had 4 e2e-aws runs — Run 2 passed fully; Runs 1, 3, and 4 each failed on different teardown tests ( codecov/patch: Pre-existing coverage gap The Konflux Enterprise Contract (both checks): Image policy failure Both Konflux checks ran against the same container image snapshot ( Recommendations
Evidence
|
6 participants
What this PR does / why we need it:
Not every hosted control plane service is labeled. Which makes it impossible to list a service or a list of service using a label selector. E.g. when a Metal load balancer needs to select the Hosted Clusters Services for the IPAddressPool.
The service labeling is aligned with the corresponding component labels:
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit