Repository navigation
[kubernetes] Fix patches for kubevirt-cloud-provider - #735
Conversation
Signed-off-by: Andrei Kvapil <[email protected]>
WalkthroughThis pull request updates the image tag for the kubevirt-cloud-provider by replacing an old digest with a new one. It also removes the old Changes
Sequence Diagram(s)sequenceDiagram
participant C as Controller
participant K8s as Kubernetes API
C->>C: Initialize controller with clusterName
C->>C: Start controller (startKubevirtCloudController)
C->>C: Check for clusterName
alt clusterName missing
C->>C: Log fatal error and exit
else clusterName present
C->>K8s: Retrieve services
K8s-->>C: Return list of services
C->>C: Filter services by clusterName
C->>K8s: Create/Update EndpointSlice for valid services
K8s-->>C: Acknowledge update
end
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Andrei Kvapil (kvaps)
changed the title
![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
🧹 Nitpick comments (2)
packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/341.diff (2)
24-35: Consider gracefully handling missing clusterName.
klog.Fatalfabruptly terminates the process if--cluster-nameis empty. If that's not strictly necessary, consider returning an error or setting a default instead.
309-436: Comprehensive slice update logic.Merging "delete + create" into "update" is an efficient approach. Consider testing corner cases (mismatched address types, partial merges) to ensure robust behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/apps/kubernetes/images/kubevirt-cloud-provider.tag(1 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/335.diff(0 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/341.diff(4 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/999.diff(0 hunks)
💤 Files with no reviewable changes (2)
- packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/999.diff
- packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/335.diff
🔇 Additional comments (9)
packages/apps/kubernetes/images/kubevirt-cloud-provider.tag (1)
1-1:Details
❓ Verification inconclusive
Confirm the updated image digest.
It's recommended to verify the new digest points to a trusted image.
Run the following command to confirm the image is recognized in the registry:
🏁 Script executed:
#!/bin/bash docker pull ghcr.io/cozystack/cozystack/kubevirt-cloud-provider:latest@sha256:47ad85a2bb2b11818df85e80cbc6e07021e97e429d5bb020ce8db002b37a77f1Length of output: 241
Image Digest Verification Required
The updated image digest in
packages/apps/kubernetes/images/kubevirt-cloud-provider.tagappears updated, but the automated verification failed because the Docker CLI wasn't available in the execution environment. Please manually verify that the image digest points to a trusted image using Docker in a local or properly configured environment. For example, run the following command where Docker is installed:docker pull ghcr.io/cozystack/cozystack/kubevirt-cloud-provider:latest@sha256:47ad85a2bb2b11818df85e80cbc6e07021e97e429d5bb020ce8db002b37a77f1Once confirmed, you can safely proceed.
packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/341.diff (8)
51-55: NewclusterNamefield in the Controller.Adding
clusterNameclarifies multi-cluster usage. Ensure single-cluster scenarios also account for a non-empty clusterName.
82-117: Refined label selection withclusterName.This ensures EndpointSlices are fetched only for the correct cluster. Verify that all relevant resources set the essential labels.
127-138: Updated EndpointSlice retrieval with clusterName.Applying tighter scoping avoids cross-cluster collisions. Confirm that EndpointSlices are labeled properly.
Do you want a script to check for missing
clusterNamelabels in EndpointSlices?
155-167: Skipping services from other clusters.Properly filters out services that belong to different clusters. This helps maintain isolation in multi-cluster setups.
293-303: Resolved closure capture pitfalls.Switching to an indexed loop prevents subtle bugs caused by capturing the loop variable. Good fix.
455-456: Test constructor now requiresclusterName.Good to see test coverage for new multi-cluster logic.
465-690: Extended test coverage.These tests thoroughly validate multi-port endpoints, no-selector usage, and cross-cluster scenarios. Excellent improvement in reliability.
58-66:Details
✅ Verification successful
Ensure consistent usage of the updated constructor.
Every call to
NewKubevirtEPSControllermust now supplyclusterName. Double-check references throughout the codebase.
🏁 Script executed:
#!/bin/bash rg -A 5 'NewKubevirtEPSController'Length of output: 3546
Updated Constructor Usage Verified
The changes correctly update the
NewKubevirtEPSControllersignature to include theclusterNameparameter, and all call sites in the patch now supply this additional argument. Specifically:
- The constructor in the patch file now takes
infraNamespace string, clusterName stringand returns*Controller.- Existing references, including the test call, have been updated (e.g.,
"test"becomes"test", "test-cluster"), ensuring consistency with the new signature.Please double-check any external usages outside of this patch to confirm they match the updated constructor pattern.
| +issues: | ||
| + exclude-rules: | ||
| + - filename: "kubevirteps_controller_test.go" | ||
| + linters: | ||
| + - govet | ||
| + text: "declaration of \"err\" shadows" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix err shadowing instead of ignoring the warning.
Excluding lint rules can hide real issues. It's often better to address the root cause by refactoring the code to avoid redeclaring err.
1 participant
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit