Pod IP selection rules for dual-stack environments #794

brownleej · 2021-06-28T20:24:07Z

Description

This issue provides an option for using specific addresses from the podIPs list in dual-stack environments. This should unblock testing with IPv6 in those environments, but there is more work to be done to fill out the IPv6 solution.

This relates to #85, but we should talk more about how we want to track the feature work for IPv6.

This also includes an option to disable the readiness probe for the sidecar, as discussed in #668. I added this here because the current probes can only go to the main pod IP, as discussed in kubernetes/kubernetes#101324.

Type of change

Please select one of the options below.

New feature (non-breaking change which adds functionality)

Discussion

Are there any design details that you would like to discuss further?

I chose to use a pattern string instead of encoding knowledge about the address family. This gives more flexibility but will make it less obvious how to configure IPv6 through this path.

When changing the IP pattern, this does a rolling bounce. That bounce will change coordinator IPs, meaning that the operator has to repeatedly select new coordinators. I considered alternatives like migrating to new pods or configuring multiple listeners, but I think it's not worth the complexity for our first pass at this.

Testing

Please describe the tests that you ran to verify your changes. Unit tests?
Manual testing?

I created a cluster in a testing environment and switched between patterns to select the IPv4 address and the IPv6 address.

Documentation

Did you update relevant documentation within this repository?

No. I think we want to do more testing before documenting IPv6, but we should track it as part of the bigger project.

Follow-up

Are there any follow-up issues that we should pursue in the future?

We have more aspects of the IPv6 story that we should fill out:

Cleaner migration paths
IPv6 support for services
IPv6 support for probes

brownleej · 2021-06-28T21:58:06Z

I hit an issue with this where the black checker was giving contradictory requirements to pycodestyle. I think we're better off restricting ourselves to only one of these checkers.

johscheuer

I'm a little bit concerned about the custom string parsing logic, that will be error prone (at least from my past experience) and we should try to make use of the according libraries in go/Python. I'm also not sure if PodIPPattern is a good idea to allow any arbitrary regex? I would rather prefer to just have an IPv4 or IPv6 option.

johscheuer · 2021-06-29T10:17:21Z

.github/workflows/pull_request.yml

@@ -63,7 +63,6 @@ jobs:
      - name: Run Python lint
        run: |
          pycodestyle --max-line-length=120 ./**/*.py
-          black --check --diff ./


I would like to still have both, they target for different things and we just have to configure them properly. black ensures a common formatting (e.g. like go fmt a nd pycodestyle also checks some other code issues.

johscheuer · 2021-06-29T10:24:43Z

api/v1beta1/foundationdbcluster_types.go


-	if len(components) < 2 {
+	ipEnd := strings.Index(address, "]:") + 1


Instead of building this custom logic I would prefer to use net.SplitHostPort that should make everything more robust.

How would that work with the custom flags in our addresses like the :tls suffix?

I got a solution for that in mind (more or less the same FDB currently does). I would work on that in #352 that should simplify thee logic here. For the PR itself that's fine and we can improve things step by step.

api/v1beta1/foundationdbcluster_types.go

johscheuer · 2021-06-29T10:25:50Z

api/v1beta1/foundationdbcluster_types.go

+	// This supports the values `pod` and `service`.
+	PublicIPSource *PublicIPSource `json:"publicIPSource,omitempty"`
+
+	// PodIPIndex tells the pod to use a specific entry from the podIPs list


Where is the PodIPIndex?

This is an artifact of an earlier form of the solution.

johscheuer · 2021-06-29T10:26:48Z

api/v1beta1/foundationdbcluster_types.go

+type RoutingConfig struct {
+	// Headless determines whether we want to run a headless service for the
+	// cluster.
+	HeadlessService *bool `json:"headlessService,omitempty"`


Strictly speaking we never use the headless service for routing.

Yeah, but I felt that "Routing" was closer than "Service" for this group of settings, since in the long term the plan is to use the headless service for discovery. Do you have any other suggestions for what we could call this?

I felt like Routing captured the intent of these fields better than Service, but I'm open to other suggestions about how to make the meaning clearer.

What about Discovery? I mean that's also an overloaded term. Otherwise I don't have any better idea :( Like always:

"There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors." :)

controllers/cluster_controller_test.go

johscheuer · 2021-06-29T10:41:58Z

foundationdb-kubernetes-sidecar/sidecar.py

        log.info(f"Listening on {address}:{port}")
-        server = ThreadingHTTPServer((address, int(port)), cls)
+
+        if ":" in address:


Please use ipaddress.ip_address(address).version == 6 instead of checking for : the latter is error prone.

johscheuer · 2021-06-29T10:50:34Z

foundationdb-kubernetes-sidecar/sidecar.py

+        ip = matching_ips[0]
+        if ":" in ip:
+            ip = "[%s]" % ip
+        return ip


The returned IP address for IPv6 will be invalid since [::] is not an valid IPv6 address

We're enclosing the addresses in brackets because everywhere we are using it in the monitor conf, it gets paired with a port, so it needs the brackets. We could modify the monitor conf to contain the brackets, but I think that would lead to more challenges with the rollouts.

I think for the current state but we might to revisit this. Sadly Python doesn't have https://golang.org/pkg/net/#JoinHostPort (at least to my knowledge) and I remember doing the same fun stuff: https://github.com/FoundationDB/fdb-kubernetes-operator/pull/341/files#diff-361ca47879a27b8387987496c32b845dbea647c753333511aebc6d93a19be6edR505-R521 I think I just moved it to a later state to prevent parsing the address for the web server again.

johscheuer · 2021-06-29T10:50:56Z

foundationdb-kubernetes-sidecar/sidecar.py

+        ips = string.split(",")
+        matching_ips = [ip for ip in ips if pattern.search(ip)]
+        if len(matching_ips) == 0:
+            raise Exception("Failed to find IP matching %s in %s" % (pattern, ips))


Suggested change

raise Exception("Failed to find IP matching %s in %s" % (pattern, ips))

raise Exception(f"Failed to find IP matching {pattern} in {ips}")

johscheuer · 2021-06-29T10:51:28Z

foundationdb-kubernetes-sidecar/sidecar.py

+        if len(matching_ips) == 0:
+            raise Exception("Failed to find IP matching %s in %s" % (pattern, ips))
+        ip = matching_ips[0]
+        if ":" in ip:


Please use ipaddress.ip_address(ip).version == 6 here

johscheuer

Only a few comments other than that LGTM for the first steps towards IPv6.

johscheuer · 2021-07-01T07:07:07Z

api/v1beta1/foundationdbcluster_types.go

+type RoutingConfig struct {
+	// Headless determines whether we want to run a headless service for the
+	// cluster.
+	HeadlessService *bool `json:"headlessService,omitempty"`


What about Discovery? I mean that's also an overloaded term. Otherwise I don't have any better idea :( Like always:

"There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors." :)

johscheuer · 2021-07-01T07:08:30Z

api/v1beta1/foundationdbcluster_types.go


-	if len(components) < 2 {
+	ipEnd := strings.Index(address, "]:") + 1


I got a solution for that in mind (more or less the same FDB currently does). I would work on that in #352 that should simplify thee logic here. For the PR itself that's fine and we can improve things step by step.

johscheuer · 2021-07-01T07:10:18Z

controllers/cluster_controller.go

 	}

 	return []string{instance.Pod.ObjectMeta.Annotations[fdbtypes.PublicIPAnnotation]}
 }

+func getPublicIpsForPod(pod *corev1.Pod) []string {


This is still unchanged? Or is GitHub just trolling me?

controllers/cluster_controller_test.go

johscheuer · 2021-07-01T07:13:09Z

foundationdb-kubernetes-sidecar/sidecar.py

+            "--public-ip-family",
+            help=(
+                "Tells the sidecar to treat the public IP as a comma-separated "
+                "list, and use the first entry in the specified IP family"


Could you add an example what the sidecar actually expects?

johscheuer · 2021-07-01T07:14:21Z

foundationdb-kubernetes-sidecar/sidecar.py

+            raise Exception(f"Failed to find IPv{version} entry in {ips}")
+        ip = matching_ips[0]
+        if version == 6:
+            ip = "[%s]" % ip


Suggested change

ip = "[%s]" % ip

ip = f"[{ip}]"

johscheuer · 2021-07-01T07:20:28Z

foundationdb-kubernetes-sidecar/sidecar.py

+        ip = matching_ips[0]
+        if ":" in ip:
+            ip = "[%s]" % ip
+        return ip


I think for the current state but we might to revisit this. Sadly Python doesn't have https://golang.org/pkg/net/#JoinHostPort (at least to my knowledge) and I remember doing the same fun stuff: https://github.com/FoundationDB/fdb-kubernetes-operator/pull/341/files#diff-361ca47879a27b8387987496c32b845dbea647c753333511aebc6d93a19be6edR505-R521 I think I just moved it to a later state to prevent parsing the address for the web server again.

johscheuer · 2021-07-01T07:22:36Z

kubectl-fdb/cmd/deprecation.go

@@ -193,3 +193,29 @@ func checkDeprecation(cmd *cobra.Command, kubeClient client.Client, inputCluster
 	cmd.Printf("%d cluster(s) without deprecations\n", clusterCounter)
 	return nil
 }
+
+func getMinimalYAML(object interface{}) ([]byte, error) {


That could also be a FoundationDBClusterSpec? I assume you used an interface{} to use that method later for different structs?

Yes, that was my thinking.

…or a pod.

…dIPs to use.

…elp with testing.

Make the liveness probe for the sidecar enabled by default in the future, while retaining an option to disable it explicitly. Remove empty fields from the YAML output in the plugin deprecation command.

Remove temporary code for reading the IP pattern from an annotation.

…IP families.

brownleej requested a review from johscheuer June 28, 2021 20:24

brownleej force-pushed the ipv6 branch from 4db1648 to c84668e Compare June 28, 2021 21:31

johscheuer requested changes Jun 29, 2021

View reviewed changes

brownleej requested a review from johscheuer June 30, 2021 23:14

johscheuer approved these changes Jul 1, 2021

View reviewed changes

brownleej added 13 commits July 1, 2021 17:32

Add support for using a specific IP from the podIPs list.

3be2b9d

Use the configured pod IP index to determine which public IP to use f…

9b3e0bc

…or a pod.

Allow specifying an arbitrary pattern to determine which IP in the po…

20d18ae

…dIPs to use.

Temporarily allow specifying the pod IP pattern as an annotation to h…

f3aa981

…elp with testing.

Fix issues with parsing IPv6 addresses.

dd85473

Add an option to disable readiness probes.

07dea65

Make the liveness probe for the sidecar enabled by default in the future, while retaining an option to disable it explicitly. Remove empty fields from the YAML output in the plugin deprecation command.

Update documentation.

a3887b0

Remove temporary code for reading the IP pattern from an annotation.

Add a comment on the sidecar probe config in the default test cluster.

04a9cc0

Fix an old variable name.

c68b424

Style fixes.

71a6f07

Remove the black formatter from our python linting.

a29d083

Replace the custom pod IP patterns with a more explicit selector for …

3dc3a25

…IP families.

Update based on PR feedback.

f62a936

brownleej force-pushed the ipv6 branch from 2d73a34 to f62a936 Compare July 2, 2021 00:36

Fix a use of a renamed method.

fe13130

brownleej merged commit 0de1b2d into FoundationDB:master Jul 2, 2021

brownleej deleted the ipv6 branch July 21, 2021 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pod IP selection rules for dual-stack environments #794

Pod IP selection rules for dual-stack environments #794

brownleej commented Jun 28, 2021

brownleej commented Jun 28, 2021

johscheuer left a comment

johscheuer Jun 29, 2021

johscheuer Jun 29, 2021

brownleej Jun 30, 2021

johscheuer Jul 1, 2021

johscheuer Jun 29, 2021

brownleej Jun 30, 2021

johscheuer Jun 29, 2021

brownleej Jun 29, 2021

brownleej Jun 30, 2021

johscheuer Jul 1, 2021

johscheuer Jun 29, 2021

johscheuer Jun 29, 2021

brownleej Jun 30, 2021

johscheuer Jul 1, 2021

johscheuer Jun 29, 2021

johscheuer Jun 29, 2021

johscheuer left a comment

johscheuer Jul 1, 2021

johscheuer Jul 1, 2021

johscheuer Jul 1, 2021

johscheuer Jul 1, 2021

johscheuer Jul 1, 2021

johscheuer Jul 1, 2021

johscheuer Jul 1, 2021

brownleej Jul 2, 2021


		if len(components) < 2 {
		ipEnd := strings.Index(address, "]:") + 1

	raise Exception("Failed to find IP matching %s in %s" % (pattern, ips))
	raise Exception(f"Failed to find IP matching {pattern} in {ips}")

Pod IP selection rules for dual-stack environments #794

Pod IP selection rules for dual-stack environments #794

Conversation

brownleej commented Jun 28, 2021

Description

Type of change

Discussion

Testing

Documentation

Follow-up

brownleej commented Jun 28, 2021

johscheuer 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

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

johscheuer 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