nic-6443 commented on code in PR #2845:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2845#discussion_r3764497003
##########
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:
I don't think `healthCheckNodePort` can bite here. It's only allocated for
`type: LoadBalancer` with `externalTrafficPolicy: Local`, and neither holds for
these specs: the e2e dataplane Service comes from
`test/e2e/framework/manifests/apisix.yaml`, which is `type: {{ .ServiceType |
default "NodePort" }}`, and `ServiceType` is only ever set to LoadBalancer in
`test/conformance/suite_test.go` — a different suite, with no
capture-and-restore test. `externalTrafficPolicy` isn't set anywhere in the
repo, so it defaults to `Cluster`.
So clearing it would be guarding a combination these tests can't reach, and
I'd rather not add that. Happy to revisit if the e2e suite ever moves to
LoadBalancer.
##########
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 answer as on the Gateway API suite: this Service is `NodePort` (default
in `test/e2e/framework/manifests/apisix.yaml`) and `externalTrafficPolicy` is
never set, so `healthCheckNodePort` is never allocated and can't collide on
restore.
--
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]
