Fix EndpointSlice reconciliation #518
Conversation
dosubot
bot
added
the
size:XXL
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several updates related to the Kubernetes application. The Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/336.diff (2)
58-58: Pass appropriate context instead ofcontext.TODO()When calling
getInfraServiceForEPS, you're usingcontext.TODO(). It's better practice to pass a propercontext.Contextfrom a higher level to allow for cancellation and timeout handling as needed.Consider passing the context from the
Initfunction or using a context tied to the lifecycle of the controller.Also applies to: 72-72, 86-86
55-97: Reduce code duplication in event handlersThe
AddFunc,UpdateFunc, andDeleteFunchandlers for theEndpointSlicesinformer contain similar logic. Refactoring the common code into a separate function can improve maintainability and readability.Example refactor:
func (c *Controller) handleInfraEndpointSliceEvent(obj interface{}, reqType ReqType) { var eps *discovery.EndpointSlice if reqType == DeleteReq { // Handle possible tombstone objects if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok { var ok bool eps, ok = tombstone.Obj.(*discovery.EndpointSlice) if !ok { klog.Errorf("Failed to convert tombstone object to EndpointSlice: %v", tombstone.Obj) return } } else { eps = obj.(*discovery.EndpointSlice) } } else { eps = obj.(*discovery.EndpointSlice) } if c.managedByController(eps) { svc, svcErr := c.getInfraServiceForEPS(context.TODO(), eps) if svcErr != nil { klog.Errorf("Failed to get infra Service for EndpointSlice %s/%s: %v", eps.Namespace, eps.Name, svcErr) return } if svc != nil { klog.Infof("Infra EndpointSlice %s: %v/%v, requeuing Service: %v/%v", reqType, eps.Namespace, eps.Name, svc.Namespace, svc.Name) c.queue.Add(newRequest(reqType, svc, nil)) } } }Update the event handlers:
AddFunc: func(obj interface{}) { c.handleInfraEndpointSliceEvent(obj, AddReq) }, UpdateFunc: func(oldObj, newObj interface{}) { c.handleInfraEndpointSliceEvent(newObj, UpdateReq) }, DeleteFunc: func(obj interface{}) { c.handleInfraEndpointSliceEvent(obj, DeleteReq) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/apps/kubernetes/Chart.yaml(1 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider.tag(1 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider/Dockerfile(1 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/335.diff(1 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/336.diff(1 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/external-traffic-policy-local.diff(0 hunks)packages/apps/versions_map(1 hunks)
💤 Files with no reviewable changes (1)
- packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/external-traffic-policy-local.diff
✅ Files skipped from review due to trivial changes (2)
- packages/apps/kubernetes/Chart.yaml
- packages/apps/kubernetes/images/kubevirt-cloud-provider.tag
🧰 Additional context used
🪛 Hadolint (2.12.0)
packages/apps/kubernetes/images/kubevirt-cloud-provider/Dockerfile
[error] 12-12: Use COPY instead of ADD for files and folders
(DL3020)
🔇 Additional comments (4)
packages/apps/versions_map (2)
45-45: LGTM: Version bump for EndpointSlice fixes
The addition of version 0.14.1 aligns with the EndpointSlice reconciliation fixes and corresponds to the Chart.yaml version bump.
44-44: Verify the commit hash for kubernetes 0.14.0
The version 0.14.0 is now pinned to commit bfbde07. Let's verify this commit contains the stable state of version 0.14.0.
✅ Verification successful
Commit hash bfbde07 correctly maps to kubernetes version 0.14.0
The verification confirms that commit bfbde07 contains version 0.14.0 in its Chart.yaml, which matches the version mapping in the versions_map file. This is the correct stable point for version 0.14.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the commit hash contains the correct version in Chart.yaml
# Check the Chart.yaml version at this commit
git show bfbde07c:packages/apps/kubernetes/Chart.yaml | grep -A1 "version:"
Length of output: 92
packages/apps/kubernetes/images/kubevirt-cloud-provider/Dockerfile (1)
10-13: 🛠️ Refactor suggestion
Improve patch management and follow Dockerfile best practices.
- Using wildcards (
*.diff) for patch application makes the build less deterministic and could lead to unintended patches being applied if new files are accidentally added. - Using
ADDinstead ofCOPYis discouraged asADDhas some magic behaviors that can make builds less predictable.
Consider these improvements:
- # see: https://github.com/kubevirt/cloud-provider-kubevirt/pull/335
- # see: https://github.com/kubevirt/cloud-provider-kubevirt/pull/336
- ADD patches /patches
- RUN git apply /patches/*.diff
+ # Patches from:
+ # - https://github.com/kubevirt/cloud-provider-kubevirt/pull/335
+ # - https://github.com/kubevirt/cloud-provider-kubevirt/pull/336
+ COPY patches/ /patches/
+ # List patches explicitly for better build determinism
+ RUN git apply /patches/endpointslice.diff /patches/reconciliation.diffLet's verify the patch files that should be listed:
✅ Verification successful
Update patch file names in the Dockerfile for better clarity and determinism.
- # see: https://github.com/kubevirt/cloud-provider-kubevirt/pull/335
- # see: https://github.com/kubevirt/cloud-provider-kubevirt/pull/336
- ADD patches /patches
- RUN git apply /patches/*.diff
+ # Patches from:
+ # - https://github.com/kubevirt/cloud-provider-kubevirt/pull/335
+ # - https://github.com/kubevirt/cloud-provider-kubevirt/pull/336
+ COPY patches/ /patches/
+ # List patches explicitly for better build determinism
+ RUN git apply /patches/335.diff /patches/336.diff🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# List all patch files to ensure we explicitly name them in the Dockerfile
find packages/apps/kubernetes/images/kubevirt-cloud-provider/patches -name "*.diff"
Length of output: 231
🧰 Tools
🪛 Hadolint (2.12.0)
[error] 12-12: Use COPY instead of ADD for files and folders
(DL3020)
packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/335.diff (1)
9-20: Fix address-of-loop-variable issue by using local variables
Assigning port fields to local variables before taking their addresses prevents the common pitfall where all pointers end up referencing the same memory address due to the loop variable being reused. This change ensures that each EndpointPort correctly references its intended values.
| @@ -3,13 +3,14 @@ FROM --platform=linux/amd64 golang:1.20.6 AS builder | |||
|
|
|||
| RUN git clone https://github.com/kubevirt/cloud-provider-kubevirt /go/src/kubevirt.io/cloud-provider-kubevirt \ | |||
| && cd /go/src/kubevirt.io/cloud-provider-kubevirt \ | |||
| && git checkout adbd6c27468b86b020cf38490e84f124ef24ab62 | |||
| && git checkout da9e0cf | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use full commit hash for better security and traceability.
Using a shortened commit hash (da9e0cf) reduces traceability and could potentially lead to ambiguity if hash collisions occur in the future.
- && git checkout da9e0cf
+ && git checkout da9e0cf2a4b89d7b78436e7c6d2e2c4f68d7c8a9Committable suggestion skipped: line range outside the PR's diff.
| RUN go get 'k8s.io/endpointslice/[email protected]' 'k8s.io/[email protected]' | ||
| RUN go mod tidy |
There was a problem hiding this comment.
💡 Codebase verification
Version mismatch detected between new dependencies and existing k8s.io packages
The go.mod shows that the existing k8s.io dependencies are using version v0.31.2, but the new dependencies in the Dockerfile are being added with version v0.28. This version mismatch could lead to compatibility issues.
packages/apps/kubernetes/images/kubevirt-cloud-provider/Dockerfile: Update the version of new dependencies from v0.28 to v0.31.2:RUN go get 'k8s.io/endpointslice/[email protected]' 'k8s.io/[email protected]'
🔗 Analysis chain
Verify compatibility of new dependencies.
The addition of EndpointSlice-related dependencies (k8s.io/endpointslice/[email protected], k8s.io/[email protected]) should be compatible with the existing codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential version conflicts in go.mod
rg -A 5 "k8s.io/(endpointslice|apiserver)" | grep "v0.28"
Length of output: 194
Script:
#!/bin/bash
# Check go.mod for k8s.io dependencies and their versions
fd go.mod --exec cat {} | grep -A 10 "require ("
# Check for imports of endpointslice and apiserver packages
rg "import.*k8s.io/(endpointslice|apiserver)" -A 5
Length of output: 1008
| + svcName := eps.Labels[discovery.LabelServiceName] | ||
| + if svcName == "" { | ||
| + // No service name label found, can't determine infra service. | ||
| + return nil, nil |
There was a problem hiding this comment.
Check for nil labels in getInfraServiceForEPS to prevent nil pointer dereference
In getInfraServiceForEPS, if eps.Labels is nil, accessing eps.Labels[discovery.LabelServiceName] may cause a nil pointer dereference. Ensure that eps.Labels is not nil before accessing it.
Add a nil check for eps.Labels:
if eps.Labels == nil {
// No labels present; cannot determine infra service.
return nil, nil
}
svcName := eps.Labels[discovery.LabelServiceName]| + } | ||
| + }, | ||
| + DeleteFunc: func(obj interface{}) { | ||
| + eps := obj.(*discovery.EndpointSlice) |
There was a problem hiding this comment.
Ensure safe type assertion in DeleteFunc to handle tombstone objects
In the DeleteFunc, when an object is deleted, it may sometimes be a cache.DeletedFinalStateUnknown due to delayed deletion in the Kubernetes cache. Directly casting obj to *discovery.EndpointSlice without checking can lead to a runtime panic. Please handle this case to ensure robustness.
Consider modifying the DeleteFunc as follows:
DeleteFunc: func(obj interface{}) {
- eps := obj.(*discovery.EndpointSlice)
+ var eps *discovery.EndpointSlice
+ if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok {
+ var ok bool
+ eps, ok = tombstone.Obj.(*discovery.EndpointSlice)
+ if !ok {
+ klog.Errorf("Failed to convert tombstone object to EndpointSlice: %v", tombstone.Obj)
+ return
+ }
+ } else {
+ eps = obj.(*discovery.EndpointSlice)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + eps := obj.(*discovery.EndpointSlice) | |
| var eps *discovery.EndpointSlice | |
| if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok { | |
| var ok bool | |
| eps, ok = tombstone.Obj.(*discovery.EndpointSlice) | |
| if !ok { | |
| klog.Errorf("Failed to convert tombstone object to EndpointSlice: %v", tombstone.Obj) | |
| return | |
| } | |
| } else { | |
| eps = obj.(*discovery.EndpointSlice) | |
| } |
| for _, port := range service.Spec.Ports { | ||
| + pName := port.Name | ||
| + pProtocol := port.Protocol | ||
| + pVal := port.TargetPort.IntVal |
There was a problem hiding this comment.
Handle TargetPort of type string to support named ports
The current implementation assumes that port.TargetPort is an integer value (IntVal). However, TargetPort can also be a string (StrVal), representing a named port. This could lead to incorrect behavior if the service uses named ports.
Consider handling both integer and string TargetPort types:
pName := port.Name
pProtocol := port.Protocol
var pPort int32
if port.TargetPort.Type == intstr.Int {
pPort = port.TargetPort.IntVal
} else {
// Resolve the named port to an integer port number as needed
// This may require additional logic to map the named port to its value
// For now, you might log a warning or handle it accordingly
klog.Warningf("Named TargetPort %s is not supported", port.TargetPort.StrVal)
}
desiredPorts = append(desiredPorts, discovery.EndpointPort{
Port: &pPort,
Protocol: &pProtocol,
Name: &pName,
})Also applies to: 17-19
Signed-off-by: Andrei Kvapil <[email protected]>
kvaps
force-pushed
the
fix-ep-reconicle
branch
from
cc87e41 to
958a0d6
Compare
Upstream fixes:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation