packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/379.diff (2)

99-155: Consider extracting shared endpoint creation logic.

The endpoint creation logic (NodeName validation, phase checks, interface iteration, IP validation, Endpoint construction) is duplicated between getAllVMIEndpoints and the main loop in getDesiredEndpoints.

Extracting a helper would reduce duplication and ensure consistent behavior:

// createEndpointFromVMI creates a discovery.Endpoint from a VMI, or returns nil if the VMI is not ready
func (c *Controller) createEndpointFromVMI(vmi *kubevirtv1.VirtualMachineInstance) *discovery.Endpoint {
    if vmi.Status.NodeName == "" {
        klog.Warningf("Skipping VMI %s/%s: NodeName is empty", vmi.Namespace, vmi.Name)
        return nil
    }
    
    ready := vmi.Status.Phase == kubevirtv1.Running
    serving := vmi.Status.Phase == kubevirtv1.Running
    terminating := vmi.Status.Phase == kubevirtv1.Failed || vmi.Status.Phase == kubevirtv1.Succeeded
    
    for _, i := range vmi.Status.Interfaces {
        if i.Name == "default" {
            if i.IP == "" {
                klog.Warningf("VMI %s/%s interface %q has no IP, skipping", vmi.Namespace, vmi.Name, i.Name)
                return nil
            }
            return &discovery.Endpoint{
                Addresses:  []string{i.IP},
                Conditions: discovery.EndpointConditions{
                    Ready:       &ready,
                    Serving:     &serving,
                    Terminating: &terminating,
                },
                NodeName: &vmi.Status.NodeName,
            }
        }
    }
    return nil
}

---

169-219: Good test coverage for the skip behavior.

The test correctly validates that endpoints and VMIs without NodeName are skipped. Consider adding an additional test case for the fallback path (getAllVMIEndpoints) to ensure full coverage of the new functionality - specifically testing that when hasEndpointsWithoutNodeName is true and nodeSet is empty, the fallback returns endpoints from all valid VMIs.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code

hack/e2e-apps/gateway-api.bats (2)

159-165: Consider increasing request sample size for load balancing verification.

With only 10 requests, there's a small probability that both backends might not be hit, especially if the load balancer has warm-up behavior or sticky sessions. Consider increasing to 20-30 requests to improve test reliability.

🔎 Proposed enhancement

  # Test load balancing
  responses=""
- for i in {1..10}; do
+ for i in {1..20}; do
    response=$(curl -s --max-time 5 $test_url)
    responses="$responses$response\n"
  done
  # Check that both backends are hit
  echo -e "$responses" | grep -q "Hello from backend1"
  echo -e "$responses" | grep -q "Hello from backend2"

---

167-173: Consider trap-based cleanup for robustness.

The current cleanup only runs if the test completes successfully. If the test fails earlier, resources may be leaked. Consider using a trap to ensure cleanup runs even on test failure.

🔎 Example trap-based cleanup pattern

@test "test gateway api tcp load balancer in tenant cluster" {
  name=test
  
  # Setup trap for cleanup
  cleanup() {
    if [ -n "$PF_PID" ]; then kill $PF_PID 2>/dev/null || true; fi
    kubectl delete deployment backend1 backend2 --namespace tenant-test --ignore-not-found
    kubectl delete configmap backend1-config backend2-config --namespace tenant-test --ignore-not-found
    kubectl delete gateway ${name}-gateway --namespace tenant-test --ignore-not-found
    kubectl delete tcproute ${name}-tcproute --namespace tenant-test --ignore-not-found
    kubectl delete service backend-svc --namespace tenant-test --ignore-not-found
  }
  trap cleanup EXIT
  
  # ... rest of test ...
}

Configuration used: defaults

Review profile: CHILL

Plan: Pro

hack/e2e-install-cozystack.bats (1)

127-129: LGTM! Gateway API enablement is correctly placed.

The ConfigMap patch to enable Gateway API is syntactically correct and appropriately positioned before the tenant patch. The change aligns with the PR objective to fix cilium-gateway endpoints and supports the new gateway API tests.

Optional: Add verification step

While the current pattern is consistent with other ConfigMap patches in this file (lines 148, 175), you could optionally add a verification step to ensure the Gateway API feature is fully reconciled before proceeding:

 # Enable Gateway API for testing
 kubectl patch configmap/cozystack -n cozy-system --type merge -p '{"data":{"addons.gatewayAPI.enabled":"true"}}'
+
+# Wait for Gateway API resources to be available
+timeout 60 sh -ec 'until kubectl get crd gateways.gateway.networking.k8s.io >/dev/null 2>&1; do sleep 1; done'

This would guard against potential race conditions, though the current implementation may be sufficient if the Gateway API controller reconciles quickly.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

hack/e2e-apps/loadbalancer-service.bats (3)

31-36: Consider increasing readiness probe delays.

The readiness probe has aggressive timing (initialDelaySeconds: 2, periodSeconds: 2). In slower test environments or under load, nginx might not be fully ready within 2 seconds, potentially causing test flakiness.

🔎 Suggested adjustment

         readinessProbe:
           httpGet:
             path: /
             port: 80
-          initialDelaySeconds: 2
-          periodSeconds: 2
+          initialDelaySeconds: 5
+          periodSeconds: 5

---

111-114: Consider adding functional verification of the LoadBalancer service.

The test verifies that endpoints are registered but doesn't confirm the LoadBalancer service is actually reachable and functional. For a complete LoadBalancer test, consider making an HTTP request through the LB IP/hostname to verify end-to-end connectivity.

🔎 Suggested addition after endpoint verification

  # Get LoadBalancer IP or hostname
  lb_address=$(kubectl get svc $name -n $ns \
    -o jsonpath='{.status.loadBalancer.ingress[0].ip}')
  if [ -z "$lb_address" ]; then
    lb_address=$(kubectl get svc $name -n $ns \
      -o jsonpath='{.status.loadBalancer.ingress[0].hostname}')
  fi
  
  # Verify service is reachable through LoadBalancer
  timeout 30 sh -ec "
    until curl -sf http://$lb_address/ | grep -q 'Hello from backend'; do
      sleep 2
    done
  "

---

116-118: Consider more robust cleanup.

The cleanup doesn't use --ignore-not-found, which could cause errors if resources weren't created due to earlier test failures. While BATS typically doesn't fail on cleanup errors, adding this flag improves robustness.

🔎 Suggested improvement

   # Cleanup
-  kubectl delete deployment backend1 backend2 -n $ns
-  kubectl delete service $name -n $ns
+  kubectl delete deployment backend1 backend2 -n $ns --ignore-not-found=true
+  kubectl delete service $name -n $ns --ignore-not-found=true

Configuration used: defaults

Review profile: CHILL

Plan: Pro

hack/e2e-apps/run-kubernetes.sh (1)

188-197: Consider fixing indentation for consistency.

These lines lack the 2-space indentation used elsewhere in the function, making the code harder to read.

🔎 Proposed fix

-LB_ADDR=$(
-  kubectl get svc --kubeconfig tenantkubeconfig-${test_name} "${test_name}-backend" \
-    -n tenant-test \
-    -o jsonpath='{.status.loadBalancer.ingress[0].ip}{.status.loadBalancer.ingress[0].hostname}'
-)
-
-if [ -z "$LB_ADDR" ]; then
-  echo "LoadBalancer address is empty" >&2
-  exit 1
-fi
+  LB_ADDR=$(
+    kubectl get svc --kubeconfig tenantkubeconfig-${test_name} "${test_name}-backend" \
+      -n tenant-test \
+      -o jsonpath='{.status.loadBalancer.ingress[0].ip}{.status.loadBalancer.ingress[0].hostname}'
+  )
+
+  if [ -z "$LB_ADDR" ]; then
+    echo "LoadBalancer address is empty" >&2
+    exit 1
+  fi

Configuration used: defaults

Review profile: CHILL

Plan: Pro

hack/e2e-apps/run-kubernetes.sh (2)

89-90: Stale comment: timeout value doesn't match.

The comment says "200 second timeout" but the actual timeout is 500s. This was previously flagged in past reviews but remains unfixed.

🔎 Proposed fix

-  # Set up port forwarding to the Kubernetes API server for a 200 second timeout
+  # Set up port forwarding to the Kubernetes API server for a 500 second timeout
   bash -c 'timeout 500s kubectl port-forward service/kubernetes-'"${test_name}"' -n tenant-test '"${port}"':6443 > /dev/null 2>&1 &'

---

128-174: Critical: Missing namespace and service selector mismatch.

Two issues prevent this LoadBalancer test from working:

1. Missing namespace creation: The Deployment and Service reference namespace: tenant-test (lines 134, 166) in the tenant cluster, but this namespace is never created. The kubectl apply commands (lines 129, 161) will fail because the namespace doesn't exist in the tenant cluster.

2. Service selector mismatch: The Service selector app: "${test_name}-backend" (line 170) won't match the pod labels app: backend and backend: "${test_name}-backend" (lines 139-140, 144-145). The LoadBalancer will have no endpoints and health checks will fail. This issue was flagged in previous reviews but remains unfixed.

🔎 Proposed fixes

Fix 1: Create the namespace in the tenant cluster

Add this before line 128:

+  # Create namespace in tenant cluster for backend test
+  kubectl create namespace tenant-test --kubeconfig tenantkubeconfig-${test_name} --dry-run=client -o yaml | \
+    kubectl apply --kubeconfig tenantkubeconfig-${test_name} -f-
+
   # Backend 1

Fix 2: Align service selector with pod labels

   selector:
-    app: "${test_name}-backend"
+    app: backend
+    backend: "${test_name}-backend"

Configuration used: defaults

Review profile: CHILL

Plan: Pro

packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/379.diff (1)

99-155: Address code duplication as previously noted.

This function duplicates significant logic from getDesiredEndpoints (lines 40-94). The conversion of a VMI to an Endpoint object is repeated in both places.

Consider extracting the VMI-to-Endpoint conversion logic into a separate helper function as suggested in the previous review. This would improve maintainability and reduce the risk of divergence between the two code paths.

🔎 Example helper function structure

// vmiToEndpoint converts a VirtualMachineInstance to a discovery.Endpoint.
// Returns nil if the VMI cannot be converted (missing NodeName, no default interface, etc.)
func vmiToEndpoint(vmi *kubevirtv1.VirtualMachineInstance) *discovery.Endpoint {
	if vmi.Status.NodeName == "" {
		klog.Warningf("Skipping VMI %s/%s: NodeName is empty", vmi.Namespace, vmi.Name)
		return nil
	}
	
	ready := vmi.Status.Phase == kubevirtv1.Running
	serving := vmi.Status.Phase == kubevirtv1.Running
	terminating := vmi.Status.Phase == kubevirtv1.Failed || vmi.Status.Phase == kubevirtv1.Succeeded
	
	for _, i := range vmi.Status.Interfaces {
		if i.Name == "default" {
			if i.IP == "" {
				klog.Warningf("VMI %s/%s interface %q has no IP, skipping", vmi.Namespace, vmi.Name, i.Name)
				return nil
			}
			nodeNamePtr := &vmi.Status.NodeName
			return &discovery.Endpoint{
				Addresses: []string{i.IP},
				Conditions: discovery.EndpointConditions{
					Ready:       &ready,
					Serving:     &serving,
					Terminating: &terminating,
				},
				NodeName: nodeNamePtr,
			}
		}
	}
	return nil
}

Then use it in both functions:

// In getDesiredEndpoints
if endpoint := vmiToEndpoint(vmi); endpoint != nil {
	desiredEndpoints = append(desiredEndpoints, endpoint)
}

// In getAllVMIEndpoints
if endpoint := vmiToEndpoint(vmi); endpoint != nil {
	endpoints = append(endpoints, endpoint)
}

packages/apps/kubernetes/images/kubevirt-cloud-provider/patches/379.diff (1)

169-219: Consider adding test coverage for the fallback behavior.

This test validates that endpoints without NodeName are skipped, but it doesn't test the new fallback logic (lines 31-36) where getAllVMIEndpoints() is called. Consider adding additional test cases to cover:

1. The fallback to getAllVMIEndpoints() when all tenant endpoints lack NodeName
2. Direct testing of the getAllVMIEndpoints() function with various VMI states
3. Scenarios with valid VMIs that have both NodeName and IP addresses

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to internal/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in internal directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to cmd/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in cmd directory