Switch to EndpointSlices #154

frobware · 2020-07-20T16:59:34Z

Handle endpoint slices so that we can deal with dual-stack pods.

~~Needs: openshift/cluster-ingress-operator#426~~ - now merged

~~Needs: openshift/openshift-apiserver#125~~ - now merged

openshift/cluster-ingress-operator#428 allows us to turn on/off support for endpointslices via an ingresscontroller or the ingress config.

frobware · 2020-07-20T17:00:27Z

Expecting this to fail without openshift/cluster-ingress-operator#426 and currently testing via cluster-bot.

/hold

frobware · 2020-07-22T13:56:06Z

(I think) This is currently failing some of the haproxy tests because the dependency on openshift/cluster-ingress-operator#426 is not in any CI release at the moment.

426 was in this build/release https://openshift-release.apps.ci.l2s4.p1.openshiftapps.com/releasestream/4.6.0-0.ci/release/4.6.0-0.ci-2020-07-22-003338 but latest CI release is currently https://openshift-release.apps.ci.l2s4.p1.openshiftapps.com/releasestream/4.6.0-0.ci/release/4.6.0-0.ci-2020-07-21-114552.

frobware · 2020-07-22T14:08:17Z

E0722 13:01:35.668887       1 reflector.go:178] github.com/openshift/router/pkg/router/controller/factory/factory.go:131: Failed to list *v1beta1.EndpointSlice: endpointslices.discovery.k8s.io is forbidden: User "system:serviceaccount:e2e-test-unprivileged-router-8nsx9:default" cannot list resource "endpointslices" in API group "discovery.k8s.io" in the namespace "e2e-test-unprivileged-router-8nsx9"

root cause is lack of RBAC for endpointslices. But this may be for just new routers. Investigating.

pkg/router/controller/factory/factory.go

pkg/router/controller/router_controller.go

sgreene570 · 2020-07-22T18:45:48Z

pkg/router/template/plugin.go

+	formatIPAddr := func(addr string) string {
+		ip := net.ParseIP(addr)
+		if ip != nil && strings.Count(addr, ":") >= 2 {
+			return "[" + addr + "]"


Would it be better to use ip in this return line than addr? Also, I believe a valid ipv6 addr parsed by net.ParseIP(addr) will meet the condition strings.Count(addr, ":") >= 2. Thoughts?

Edge cases:

Would it be better to use ip in this return line than addr? Also, I believe a valid ipv6 addr parsed by net.ParseIP(addr) will meet the condition strings.Count(addr, ":") >= 2. Thoughts?

This needs more thought and unit tests:

package main import ( "fmt" "net" ) func main() { ip := net.ParseIP("::FFFF:127.0.0.1") fmt.Println(ip) fmt.Println("is IPv4", ip.To4()) }

https://play.golang.org/p/4xXQTQNs45g

Still expecting this to fail CI haproxy tests where the system:router cluster role does not have privileges to list/watch endpointslices.

Needs: openshift/openshift-apiserver#125

Known failures are:

"[sig-network][Feature:Router] The HAProxy router converges when multiple routers are writing status [Suite:openshift/conformance/parallel]" "[sig-network][Feature:Router] The HAProxy router should override the route host for overridden domains with a custom value [Suite:openshift/conformance/parallel]" "[sig-network][Feature:Router] The HAProxy router should override the route host with a custom value [Suite:openshift/conformance/parallel]" "[sig-network][Feature:Router] The HAProxy router should run even if it has no access to update status [Suite:openshift/conformance/parallel]" "[sig-network][Feature:Router] The HAProxy router should serve a route that points to two services and respect weights [Suite:openshift/conformance/parallel]" "[sig-network][Feature:Router] The HAProxy router should serve the correct routes when scoped to a single namespace and label set [Suite:openshift/conformance/parallel]"

With latest changes all are now passing with the exception of:

"[sig-network][Feature:Router] The HAProxy router should serve a route that points to two services and respect weights [Suite:openshift/conformance/parallel]"

pkg/router/template/plugin.go

frobware · 2020-07-23T12:12:13Z

Still expecting this to fail CI haproxy tests where the system:router cluster role does not have privileges to list/watch endpointslices.

Needs: openshift/openshift-apiserver#125

frobware · 2020-07-23T12:13:23Z

@sgreene570 thanks for the reviews. Continuously pushing changes as I run into issues.

pkg/router/controller/factory/factory.go

pkg/router/controller/router_controller.go

pkg/router/template/plugin.go

Miciah · 2020-07-23T20:38:25Z

pkg/router/controller/factory/factory.go

+			if serviceName := endpointSliceServiceName(eps); serviceName == "" {
+				utilruntime.HandleError(fmt.Errorf("EndpointSlice %s/%s has no %q label", eps.Namespace, eps.Name, ServiceNameLabel))
+			} else {
+				objMeta := eps.ObjectMeta.DeepCopy()


Is using a copy necessary?

Miciah · 2020-07-23T20:38:37Z

pkg/router/controller/factory/factory.go

+
+	for i := range objs {
+		eps := objs[i].(*discoveryv1beta1.EndpointSlice)
+		fullSet = append(fullSet, *eps.DeepCopy())


Is using a copy necessary here?

It comes out of the store. But will look to see if we mutate at all and if not we can use it as-is.

pkg/router/controller/router_controller.go

frobware · 2020-07-24T10:14:06Z

/retest

frobware · 2020-07-24T10:19:39Z

I tried to use the auto commit suggestions - does this still build?

/retest

frobware · 2020-07-24T17:17:47Z

@Miciah thanks for the review and suggestions. Mostly taken all in the now squashed commits. Currently debugging one CI flake which may be related to the change. Need to discuss what it means to sort the complete subset of endpoints.

openshift-bot · 2020-07-29T18:33:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-29T21:35:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-29T21:50:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-29T23:19:05Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-30T01:33:25Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-30T05:40:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

frobware · 2020-07-30T06:50:39Z

Last failure was:

release "release-initial" failed: could not create watcher for pod: unknown (get pods)

openshift-bot · 2020-07-30T07:11:18Z

/retest

Please review the full test history for this PR and help us cut down flakes.

frobware · 2020-07-30T08:39:59Z

/hold

Not sure if openshift/kubernetes#300 is the fix for upgrade failures but there's no point continuously failing here (on the upgrade job).

frobware · 2020-07-30T14:16:11Z

/hold cancel

openshift/kubernetes#300 now merged.

frobware · 2020-07-30T14:16:54Z

/retest

openshift-bot · 2020-07-30T14:33:22Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-30T16:17:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-30T16:30:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

frobware · 2020-07-30T21:09:19Z

/hold

I pushed a2e6e2c to flush out whether shifting to endpointslices and the CI failures are related.

Miciah · 2020-07-30T23:58:13Z

/lgtm

openshift-ci-robot · 2020-07-30T23:58:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/cmd/infra/router/OWNERS~~ [Miciah,frobware]
~~pkg/router/OWNERS~~ [Miciah,frobware]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Miciah · 2020-07-31T03:54:51Z

error: build error: failed to pull image: error pulling image configuration: Get https://docker-registry.default.svc:5000/v2/ci-op-84vhvll6/pipeline/blobs/sha256:2ce56bf6f5c9e7bcf5a0c6ad0b27fb1e81bd87431e9888718387c324c1246fe2: EOF

/test e2e
/test e2e-upgrade

frobware · 2020-07-31T08:18:34Z

/retest

frobware · 2020-07-31T08:25:07Z

/hold cancel

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2020

openshift-ci-robot requested review from knobunc and smarterclayton July 20, 2020 16:59

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2020

frobware force-pushed the endpointslices branch 4 times, most recently from fd483ce to a52dacd Compare July 21, 2020 07:34

frobware mentioned this pull request Jul 21, 2020

[WIP] - Allow EndpointSlices to be disabled openshift/cluster-ingress-operator#428

Closed

frobware force-pushed the endpointslices branch 6 times, most recently from 9147242 to fc04b2f Compare July 22, 2020 12:01

sgreene570 reviewed Jul 22, 2020

View reviewed changes

frobware mentioned this pull request Jul 23, 2020

Update system:router cluster role for endpointslices openshift/openshift-apiserver#125

Merged

frobware force-pushed the endpointslices branch from fc04b2f to 32175d4 Compare July 23, 2020 12:02

frobware requested a review from sgreene570 July 23, 2020 12:10

Miciah reviewed Jul 23, 2020

View reviewed changes

frobware force-pushed the endpointslices branch 2 times, most recently from dfa50bd to 1e94a11 Compare July 24, 2020 17:11

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2020

frobware force-pushed the endpointslices branch from a2e6e2c to a25b686 Compare July 30, 2020 23:57

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2020

openshift-merge-robot merged commit a8577e5 into openshift:master Jul 31, 2020

frobware mentioned this pull request Dec 2, 2020

Bug 1903206: Check Ready condition status in Endpointslices #229

Merged

frobware deleted the endpointslices branch May 1, 2024 12:04

Switch to EndpointSlices #154

Switch to EndpointSlices #154

Conversation

frobware commented Jul 20, 2020 • edited

frobware commented Jul 20, 2020 • edited

frobware commented Jul 22, 2020

frobware commented Jul 22, 2020

sgreene570 Jul 22, 2020

Choose a reason for hiding this comment

frobware Jul 23, 2020

Choose a reason for hiding this comment

frobware Jul 23, 2020

Choose a reason for hiding this comment

frobware Jul 24, 2020 • edited

Choose a reason for hiding this comment

frobware commented Jul 23, 2020

frobware commented Jul 23, 2020

Miciah Jul 23, 2020

Choose a reason for hiding this comment

Miciah Jul 23, 2020

Choose a reason for hiding this comment

frobware Jul 24, 2020

Choose a reason for hiding this comment

frobware commented Jul 24, 2020

frobware commented Jul 24, 2020 • edited

frobware commented Jul 24, 2020

openshift-bot commented Jul 29, 2020

openshift-bot commented Jul 29, 2020

openshift-bot commented Jul 29, 2020

openshift-bot commented Jul 29, 2020

openshift-bot commented Jul 30, 2020

openshift-bot commented Jul 30, 2020

frobware commented Jul 30, 2020

openshift-bot commented Jul 30, 2020

frobware commented Jul 30, 2020

frobware commented Jul 30, 2020

frobware commented Jul 30, 2020

openshift-bot commented Jul 30, 2020

openshift-bot commented Jul 30, 2020

openshift-bot commented Jul 30, 2020

frobware commented Jul 30, 2020

Miciah commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 2020

Miciah commented Jul 31, 2020

frobware commented Jul 31, 2020

frobware commented Jul 31, 2020

frobware commented Jul 20, 2020 •

edited

frobware commented Jul 20, 2020 •

edited

frobware Jul 24, 2020 •

edited

frobware commented Jul 24, 2020 •

edited