Forward ClusterIP traffic to inter-node communication port before CQL is ready #1614

zimnx · 2023-11-30T21:24:01Z

Kubernetes reconciles Endpoints and EndpointSlices for Services having
selector by taking entire Pod readiness into account. Because we need to
allow inter-node communication before CQL traffic we used
PublishNonReadyEndpoints on Services to overcome this.
But at the same time, we would like stop accepting new connections when
Pod is tearing down. These two requirements contradicts with each other.

To satisfy both requirements, Operator will reconcile Endpoints and
EndpointSlice resources in per-container way. If Pod container specifies
port and has its own Readiness probe, Endpoint/EndpointSlice
will become ready for this port when given container is ready. If these
two conditions aren't met, given port becomes ready when entire Pod is
ready.
Controller logic will consider Pod being deleted (non-nil
deletionTimestamp) as fully non-ready so that all endpoints for all ports
and containers will removed.

Additional container added to Scylla Pod controls readiness of ports
used for inter-node communication.

With above logic, nodes observing other node going down, won't be
able to reconnect until restarted node opens a port for inter-node
communication because traffic will be rejected immediately when node
starts terminating. This fixes an issue of traffic disruption during rolling
restarts happening on ScyllaClusters using ClusterIP for inter-node
communication which was caused by nodes being stuck on reconnection
attempts.

Prerequisites:

Fixes #1077

scylla-operator-bot · 2023-11-30T21:24:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zimnx

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:

~~OWNERS~~ [zimnx]

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

zimnx · 2024-03-01T16:47:24Z

Flake - #1516 (comment)
/retest

zimnx · 2024-03-11T08:41:43Z

/test images
/retest

zimnx · 2024-03-11T09:51:37Z

Cassandra timeouts, both seems to be related to PDs
/retest

zimnx · 2024-03-11T11:25:07Z

Cassandra timeouts, both seems to be related to PDs
/retest

pkg/controller/scyllacluster/conditions.go

pkg/controller/scyllacluster/resource.go

tnozicka · 2024-03-25T12:51:31Z

pkg/controller/scyllacluster/resource.go

+		}
+
+		endpointSliceLabels := map[string]string{}
+		maps.Copy(endpointSliceLabels, sc.Labels)


I'd do just the svc labels (the have their inheriting rules from scyllacluster) already. (also the controlling object of the endpoint is the service)

It was added on your request: #1614 (comment)

one of those guys was wrong but I am not sure which one :)

Member services may have labels/annotation with special meaning like allocating a LB or its options so it may or may not be desirable to copy them. I am afraid this will end up with extra API field but I'd not like to make you start with it until such case is raised and justified. Should we start with just the scyllacluster annotations to be safe?

tnozicka · 2024-03-25T12:52:51Z

pkg/controller/scyllacluster/resource.go

+		endpointSliceLabels[discoveryv1.LabelServiceName] = svc.Name
+
+		endpointSliceAnnotations := map[string]string{}
+		maps.Copy(endpointSliceAnnotations, sc.Annotations)


tnozicka · 2024-03-25T13:38:07Z

pkg/controller/scyllacluster/resource.go

+			}
+
+			// Container serves given Service port and decides about readiness
+			if ok && containerServingPort.ReadinessProbe != nil {


Why do you need to check the probe setup? I'd assume that kubelet uniformly reports the pod status as ready if there is no probe set.
(This would avoid the cases where say liveness set and readiness missing would imply the other.)

I think this is where it could instead find the container status as well would could avoid another loop and have it nicely colocated - unless I am missing something.

Kubelet reports container readiness as ready if container doesn't have readiness probe. Which is opposite of what it's implemented here. Why would you want to change this behavior?

Which container doesn't define readiness and shouldn't be ready? the container ports are defined on the container that defines the probe as well, right? At least the storage one is

scylla-operator/pkg/controller/scyllacluster/resource.go

Line 779 in 2aa6bd3

ReadinessProbe: &corev1.Probe{

(From the PR description)

If these two conditions aren't met, given port becomes ready when entire Pod is ready.

It would be useful to explain why an possibly mention a concrete case.

pkg/controller/scyllacluster/resource.go

tnozicka · 2024-03-25T14:11:26Z

pkg/controller/scyllacluster/resource.go

+		return h, err
+	}
+
+	fnvHash := fnv.New32a()


encoding seem to be more appropriate then hashing a hash

encoding sha512 wouldn't solve it, as it's too long to be used as part of a name, hence different hashing algorithm. I changed it to not hash a hash though.

encoding sha512 wouldn't solve it, as it's too long to be used as part of a name

I didn't get that. Encoding converts the char sets. Hashes (and their encodings) can be arbitrarily truncated as they have random distribution.

Kubernetes reconciles Endpoints and EndpointSlices for Services having selector by taking entire Pod readiness into account. Because we need to allow inter-node communication before CQL traffic we used PublishNonReadyEndpoints on Services to overcome this. But at the same time, we would like stop accepting new connections when Pod is tearing down. These two requirements contradicts with each other. To satisfy both requirements, Operator will reconcile Endpoints and EndpointSlice resources in per-container way. If Pod container specifies port and has its own Readiness probe, Endpoint/EndpointSlice will become ready for this port when given container is ready. If these two conditions aren't met, given port becomes ready when entire Pod is ready. Controller logic will consider Pod being deleted (non-nil deletionTimestamp) as fully non-ready so that all endpoints for all ports and containers will removed. Additional container added to Scylla Pod controls readiness of ports used for inter-node communication.

tnozicka · 2024-04-19T16:00:00Z

Hi, apologies for the delay getting back with everything that has been going on. This is large enough so I don't want you and me to context switch in small batches and rather allocate a continuous chunk to get this finished. I'll be on PTO next week but I have scheduled this for the top of my list when I get back. If I don't get this submitted by Apr 30th EOD, please ping me!

rzetelskik

left a couple of comments, only skimmed through the tests for now

rzetelskik · 2024-04-23T16:14:25Z

pkg/controller/scyllacluster/sync.go

+		},
+	)
+	if err != nil {
+		objectErrs = append(objectErrs, err)


I know the other invocations don't wrap this error, but is there actually a reason for not wrapping it?

rzetelskik · 2024-04-23T16:16:09Z

pkg/controller/scyllacluster/resource.go

-			PublishNotReadyAddresses: true,
+			Type:     corev1.ServiceTypeClusterIP,
+			Ports:    servicePorts(sc),
+			Selector: nil,


nit: shall we have a comment explaining why there's no selector? Or maybe even a higher-level comment how we work around it? It's obvious in this PR's context, but probably not so much for someone looking at it in the future. On the other hand it can be backtracked to this commit, so I'm just leaving it as a nit.

rzetelskik · 2024-04-23T16:27:21Z

pkg/controller/scyllacluster/resource.go

+`,
+							},
+							ReadinessProbe: &corev1.Probe{
+								TimeoutSeconds:   int32(30),


nit: shall we have comments explaining these values?

rzetelskik · 2024-04-23T16:31:05Z

pkg/controller/scyllacluster/resource.go

+			return endpointSlices, fmt.Errorf("can't get Pod %q: %w", naming.ManualRef(sc.Namespace, svc.Name), err)
+		}
+
+		// Don't publish endpoints for Pod that are being deleted.


Suggested change

// Don't publish endpoints for Pod that are being deleted.

// Don't publish endpoints for Pods that are being deleted.

rzetelskik · 2024-04-23T16:32:13Z

pkg/controller/scyllacluster/resource.go

+		// Don't publish endpoints for Pod that are being deleted.
+		// Removing endpoints prevents from new connections being established while still allowing
+		// for existing connections to survive and finish their requests.
+		// We need to do this early, as gap between when Pod is actually deleted and Endpoint


Suggested change

// We need to do this early, as gap between when Pod is actually deleted and Endpoint

// We need to do this early, as a gap between when Pod is actually deleted and Endpoint

rzetelskik · 2024-04-23T17:06:22Z

pkg/controller/scyllacluster/resource_test.go

+	}
+
+	for i := range tt {
+		tc := tt[i]


no need for this anymore

rzetelskik · 2024-04-23T17:06:59Z

pkg/controllerhelpers/core_test.go

+	}
+
+	for i := range tt {
+		tc := tt[i]


rzetelskik · 2024-04-23T17:07:43Z

pkg/controllerhelpers/convert_test.go

+	}
+
+	for i := range tt {
+		tc := tt[i]


rzetelskik · 2024-04-23T17:08:03Z

pkg/controllerhelpers/core_test.go

+				t.Errorf("expected found %#v, got %#v", tc.expectedFound, found)
+			}
+			if !reflect.DeepEqual(container, tc.expectedContainer) {
+				t.Errorf("expected container %#v, got %#v", tc.expectedContainer, container)


can we use cmp.Diff instead?

rzetelskik · 2024-04-23T17:09:30Z

pkg/controller/scyllacluster/resource_test.go

+
+	collidedContainerName, collision := hashes[hash]
+	if collision {
+		t.Errorf("found collision on container endpoint ports, both pod and it's controlled ports and %q container with its serving port generate same hash", collidedContainerName)


Suggested change

t.Errorf("found collision on container endpoint ports, both pod and it's controlled ports and %q container with its serving port generate same hash", collidedContainerName)

t.Errorf("found collision on container endpoint ports, both pod and its controlled ports and %q container with its serving port generate same hash", collidedContainerName)

rzetelskik · 2024-04-23T17:22:33Z

pkg/controllerhelpers/convert_test.go

+				t.Fatal(err)
+			}
+			if !apiequality.Semantic.DeepEqual(endpoints, tc.expectedEndpoints) {
+				t.Errorf("expected and actual Endpoints(s) differ: %s", cmp.Diff(tc.expectedEndpoints, endpoints))


nit: Endpoints(s) doesn't really make sense

rzetelskik · 2024-04-24T15:03:48Z

pkg/controller/scyllacluster/resource_test.go

+			},
+		},
+		{
+			name:    "EndpointSlices to Service ports backed by container having readiness probe are ready when container is ready regardless of Pod readiness",


Suggested change

name: "EndpointSlices to Service ports backed by container having readiness probe are ready when container is ready regardless of Pod readiness",

name: "EndpointSlices for Service ports backed by container having readiness probe are ready when container is ready regardless of Pod readiness",

for consistency mostly

rzetelskik · 2024-04-24T15:06:08Z

pkg/controller/scyllacluster/resource_test.go

+			},
+		},
+		{
+			name:    "EndpointSlice have IPV6 AddressType when PodIP from Pod.Status is an IPv6 address",


Suggested change

name: "EndpointSlice have IPV6 AddressType when PodIP from Pod.Status is an IPv6 address",

name: "EndpointSlice has IPV6 AddressType when PodIP from Pod.Status is an IPv6 address",

rzetelskik · 2024-04-24T15:08:17Z

pkg/controller/scyllacluster/resource_test.go

+	for _, svcPort := range svc.Spec.Ports {
+		containerServingPort, ok, err := controllerhelpers.FindContainerServingPort(svcPort, sts.Spec.Template.Spec.Containers)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		ep := discoveryv1.EndpointPort{
+			Name:        pointer.Ptr(svcPort.Name),
+			Port:        pointer.Ptr(svcPort.Port),
+			Protocol:    pointer.Ptr(svcPort.Protocol),
+			AppProtocol: svcPort.AppProtocol,
+		}
+
+		if ok && containerServingPort.ReadinessProbe != nil {
+			containerPortsMap[containerServingPort.Name] = append(containerPortsMap[containerServingPort.Name], ep)
+		} else {
+			podPorts = append(podPorts, ep)
+		}
+	}


could this be abstracted to a func shared between the test and implementation, so that we're sure they don't diverge?

rzetelskik · 2024-04-24T15:09:47Z

pkg/controllerhelpers/convert_test.go

+			},
+		},
+		{
+			name: "multi EndpointSlices per Service",


nit: multiple

rzetelskik · 2024-04-24T15:11:06Z

pkg/controllerhelpers/core_test.go

+				t.Errorf("expected error %#v, got %#v", tc.expectedError, err)
+			}
+			if !reflect.DeepEqual(found, tc.expectedFound) {
+				t.Errorf("expected found %#v, got %#v", tc.expectedFound, found)


%t instead of %#v?

tnozicka · 2024-05-02T13:30:07Z

@zimnx can you please link the issue on the kubernetes repo we've talked about filling in the past that explains the problem and why the existing API concepts didn't cover it? Was there any feedback you got on the approach or alternative ideas from the community?

pkg/controller/scyllacluster/resource.go

tnozicka · 2024-05-03T17:39:01Z

pkg/controller/scyllacluster/resource.go

+		pod, err := podLister.Pods(sc.Namespace).Get(svc.Name)
+		if err != nil {
+			if apierrors.IsNotFound(err) {
+				continue


tnozicka · 2024-05-03T17:55:50Z

pkg/controller/scyllacluster/resource.go

+		}
+
+		endpointSliceLabels := map[string]string{}
+		maps.Copy(endpointSliceLabels, sc.Labels)


one of those guys was wrong but I am not sure which one :)

Member services may have labels/annotation with special meaning like allocating a LB or its options so it may or may not be desirable to copy them. I am afraid this will end up with extra API field but I'd not like to make you start with it until such case is raised and justified. Should we start with just the scyllacluster annotations to be safe?

tnozicka · 2024-05-03T18:08:57Z

pkg/controllerhelpers/core.go

@@ -249,3 +251,38 @@ func GetScyllaContainerID(pod *corev1.Pod) (string, error) {

 	return cs.ContainerID, nil
 }
+
+func FindContainerServingPort(port corev1.ServicePort, containers []corev1.Container) (corev1.Container, bool, error) {


Suggested change

func FindContainerServingPort(port corev1.ServicePort, containers []corev1.Container) (corev1.Container, bool, error) {

func FindContainerServingPort(svcPort corev1.ServicePort, containers []corev1.Container) (corev1.Container, bool, error) {

port is ambiguous

tnozicka · 2024-05-03T18:19:10Z

pkg/controllerhelpers/core.go

+			if port.Name == cp.Name {
+				return true
+			}
+
+			if port.Port == cp.ContainerPort {
+				return true
+			}


what if the name matches but not the port?

tnozicka · 2024-05-03T18:47:15Z

pkg/controller/scyllacluster/resource.go

+			}
+
+			// Container serves given Service port and decides about readiness
+			if ok && containerServingPort.ReadinessProbe != nil {


Which container doesn't define readiness and shouldn't be ready? the container ports are defined on the container that defines the probe as well, right? At least the storage one is

scylla-operator/pkg/controller/scyllacluster/resource.go

Line 779 in 2aa6bd3

ReadinessProbe: &corev1.Probe{

(From the PR description)

If these two conditions aren't met, given port becomes ready when entire Pod is ready.

It would be useful to explain why an possibly mention a concrete case.

tnozicka · 2024-05-03T18:55:45Z

pkg/naming/constants.go

+	SidecarInjectorContainerName              = "sidecar-injection"
+	PerftuneContainerName                     = "perftune"
+	CleanupContainerName                      = "cleanup"
+	InterNodeTrafficProbeSidecarContainerName = "inter-node-traffic-probe-sidecar"


Suggested change

InterNodeTrafficProbeSidecarContainerName = "inter-node-traffic-probe-sidecar"

InterNodeTrafficProbeContainerName = "inter-node-traffic-probe"

tnozicka · 2024-05-03T18:58:55Z

pkg/controller/scyllacluster/resource.go

+		return h, err
+	}
+
+	fnvHash := fnv.New32a()


encoding sha512 wouldn't solve it, as it's too long to be used as part of a name

I didn't get that. Encoding converts the char sets. Hashes (and their encodings) can be arbitrarily truncated as they have random distribution.

pkg/controller/scyllacluster/conditions.go

scylla-operator-bot · 2024-05-16T14:04:55Z

@zimnx: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gke-parallel-clusterip	`1f80b92`	link	true	`/test e2e-gke-parallel-clusterip`
ci/prow/e2e-gke-release-script-latest	`1f80b92`	link	true	`/test e2e-gke-release-script-latest`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

scylla-operator-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-kind needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 30, 2023

scylla-operator-bot bot requested a review from rzetelskik November 30, 2023 21:24

scylla-operator-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 30, 2023

scylla-operator-bot bot requested a review from tnozicka November 30, 2023 21:24

scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2023

zimnx force-pushed the mz/endpointslices-controller branch 5 times, most recently from 4dc2f8f to d77e0f4 Compare December 4, 2023 17:33

zimnx force-pushed the mz/endpointslices-controller branch 3 times, most recently from 8d9c038 to 28f6bd1 Compare December 8, 2023 15:41

zimnx changed the title ~~[WIP] Endpointslices controller~~ [WIP] Fix traffic disruption happening during rolling restarts Dec 8, 2023

zimnx changed the title ~~[WIP] Fix traffic disruption happening during rolling restarts~~ Fix traffic disruption happening during rolling restarts Dec 8, 2023

scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2023

zimnx added kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 8, 2023

scylla-operator-bot bot removed do-not-merge/needs-kind labels Dec 8, 2023

zimnx added do-not-merge/needs-kind and removed kind/feature Categorizes issue or PR as related to a new feature. labels Dec 8, 2023

scylla-operator-bot bot removed the do-not-merge/needs-kind label Dec 8, 2023

zimnx added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. do-not-merge/needs-kind labels Dec 8, 2023

scylla-operator-bot bot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 8, 2023

zimnx force-pushed the mz/endpointslices-controller branch 3 times, most recently from 15ac37f to e7b623b Compare February 14, 2024 13:31

zimnx force-pushed the mz/endpointslices-controller branch 5 times, most recently from 4509e1c to 2709a33 Compare February 20, 2024 21:09

scylla-operator-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2024

zimnx force-pushed the mz/endpointslices-controller branch from 2709a33 to dbeb62b Compare March 1, 2024 15:30

scylla-operator-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2024

zimnx force-pushed the mz/endpointslices-controller branch from dbeb62b to 4f0d7d4 Compare March 11, 2024 13:58

zimnx force-pushed the mz/endpointslices-controller branch from 4f0d7d4 to b9aa243 Compare March 19, 2024 20:41

tnozicka reviewed Mar 25, 2024

View reviewed changes

zimnx added 2 commits March 27, 2024 23:18

Update autogenerated

1f80b92

zimnx force-pushed the mz/endpointslices-controller branch from b9aa243 to 1f80b92 Compare March 27, 2024 22:18

zimnx requested a review from tnozicka March 27, 2024 22:20

rzetelskik reviewed Apr 23, 2024

View reviewed changes

rzetelskik reviewed Apr 24, 2024

View reviewed changes

zimnx mentioned this pull request Apr 25, 2024

Traffic stability during rolling upgrades #1577

Open

tnozicka reviewed May 3, 2024

View reviewed changes

	// Don't publish endpoints for Pod that are being deleted.
	// Don't publish endpoints for Pods that are being deleted.

	// We need to do this early, as gap between when Pod is actually deleted and Endpoint
	// We need to do this early, as a gap between when Pod is actually deleted and Endpoint

	t.Errorf("found collision on container endpoint ports, both pod and it's controlled ports and %q container with its serving port generate same hash", collidedContainerName)
	t.Errorf("found collision on container endpoint ports, both pod and its controlled ports and %q container with its serving port generate same hash", collidedContainerName)

	name: "EndpointSlices to Service ports backed by container having readiness probe are ready when container is ready regardless of Pod readiness",
	name: "EndpointSlices for Service ports backed by container having readiness probe are ready when container is ready regardless of Pod readiness",

	name: "EndpointSlice have IPV6 AddressType when PodIP from Pod.Status is an IPv6 address",
	name: "EndpointSlice has IPV6 AddressType when PodIP from Pod.Status is an IPv6 address",

	func FindContainerServingPort(port corev1.ServicePort, containers []corev1.Container) (corev1.Container, bool, error) {
	func FindContainerServingPort(svcPort corev1.ServicePort, containers []corev1.Container) (corev1.Container, bool, error) {

	InterNodeTrafficProbeSidecarContainerName = "inter-node-traffic-probe-sidecar"
	InterNodeTrafficProbeContainerName = "inter-node-traffic-probe"

Forward ClusterIP traffic to inter-node communication port before CQL is ready #1614

Are you sure you want to change the base?

Forward ClusterIP traffic to inter-node communication port before CQL is ready #1614

Conversation

zimnx commented Nov 30, 2023 • edited

scylla-operator-bot bot commented Nov 30, 2023

zimnx commented Mar 1, 2024

zimnx commented Mar 11, 2024

zimnx commented Mar 11, 2024

zimnx commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimnx Mar 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka commented Apr 19, 2024

rzetelskik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzetelskik Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scylla-operator-bot bot commented May 16, 2024

zimnx commented Nov 30, 2023 •

edited

zimnx Mar 27, 2024 •

edited

rzetelskik Apr 23, 2024 •

edited