Copilot commented on code in PR #2845:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2845#discussion_r3764483879
##########
test/e2e/crds/v2/status.go:
##########
@@ -187,6 +187,12 @@ spec:
Expect(err).NotTo(HaveOccurred(), "getting service
yaml")
err = yaml.Unmarshal([]byte(serviceYaml), &k8sservice)
Expect(err).NotTo(HaveOccurred(), "unmarshalling
service")
+ // Switching to ExternalName released the allocated
node ports, and a service
+ // in a parallel test namespace may have taken them
since. Ask for fresh ones
+ // instead of the old numbers, which would fail with
"already allocated".
+ for i := range oldSpec.Ports {
+ oldSpec.Ports[i].NodePort = 0
+ }
Review Comment:
Same as the Gateway API suite: clearing `spec.ports[*].nodePort` helps, but
`spec.healthCheckNodePort` can also be allocated and later conflict when
restoring a prior spec (notably for `LoadBalancer` + `externalTrafficPolicy:
Local`). Resetting/omitting `oldSpec.HealthCheckNodePort` as part of the
restore would make this more robust.
##########
test/e2e/gatewayapi/status.go:
##########
@@ -154,6 +154,12 @@ spec:
Expect(err).NotTo(HaveOccurred(), "getting service
yaml")
err = yaml.Unmarshal([]byte(serviceYaml), &k8sservice)
Expect(err).NotTo(HaveOccurred(), "unmarshalling
service")
+ // Switching to ExternalName released the allocated
node ports, and a service
+ // in a parallel test namespace may have taken them
since. Ask for fresh ones
+ // instead of the old numbers, which would fail with
"already allocated".
+ for i := range oldSpec.Ports {
+ oldSpec.Ports[i].NodePort = 0
+ }
Review Comment:
This fixes `spec.ports[*].nodePort` collisions, but if the Service was a
`LoadBalancer` with `externalTrafficPolicy: Local`, Kubernetes can also
allocate `spec.healthCheckNodePort`, which could collide the same way on
restore. Consider also resetting `oldSpec.HealthCheckNodePort` to 0 (or
otherwise ensuring it is omitted) before applying `k8sservice.Spec = oldSpec`
to fully eliminate the port-allocation race.
##########
test/e2e/gatewayapi/status.go:
##########
@@ -154,6 +154,12 @@ spec:
Expect(err).NotTo(HaveOccurred(), "getting service
yaml")
err = yaml.Unmarshal([]byte(serviceYaml), &k8sservice)
Expect(err).NotTo(HaveOccurred(), "unmarshalling
service")
+ // Switching to ExternalName released the allocated
node ports, and a service
+ // in a parallel test namespace may have taken them
since. Ask for fresh ones
+ // instead of the old numbers, which would fail with
"already allocated".
+ for i := range oldSpec.Ports {
+ oldSpec.Ports[i].NodePort = 0
+ }
Review Comment:
This exact `NodePort`-clearing logic is duplicated in
`test/e2e/crds/v2/status.go`. Consider extracting a small helper (e.g., in a
shared e2e test util package) to keep the behavior consistent and reduce the
chance of future fixes being applied to only one suite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
