Improve traffic-managers podCIDR discovery #2093

thallgren · 2021-09-17T15:03:45Z

Description

This PR contains two improvements to the pod CIDR discovery mechanism.

Performance improved by replacing the kates watchers with standard k8s APIs ([v2] traffic-manager high CPU usage after some undetermined amount of usage #2084).
Make pod discovery strategy configurable in the Helm chart (Subnet retrieval should be more flexible #2023).

Checklist

I made sure to update ./CHANGELOG.md.
My change is adequately tested.

josecv

looks good but i'm somewhat confused about the watch function --just wanna make sure that it actually is being invoked, otherwise none of this will work 🙃

josecv · 2021-09-17T15:32:10Z

charts/telepresence/values.yaml

+#  nodePodCIDRs extract CIDRs from the podCIDR and podCIDRs field of the Node Spec.
+#  coverPodIPs  extract IPs from the podIP and podIPs field of the Pod Status and compute the CIDRs needed to cover those IPs.
+#  environment  use CIDRs listed in the space separated POD_CIDRS environment variable verbatim.
+#  auto         first tru nodePodCIDRs and if that fails, tru coverPodIPs


typo -- it says tru instead of try

josecv · 2021-09-17T17:21:46Z

cmd/traffic/cmd/manager/internal/cluster/nodewatcher.go

+	informer cache.SharedIndexInformer
+	subnets  []*net.IPNet
+	changed  time.Time
+	lock     sync.Mutex // Protects all access to ipsMap


should this read Protects all access to subnets?

josecv · 2021-09-17T17:28:42Z

cmd/traffic/cmd/manager/internal/cluster/nodewatcher.go

+	return changed
+}
+
+func (w *nodeWatcher) watch(ctx context.Context) {


maybe i'm just confused but it seems like watch is never invoked?

josecv · 2021-09-17T17:30:12Z

cmd/traffic/cmd/manager/internal/cluster/nodewatcher.go

+func (w *nodeWatcher) viable(ctx context.Context) bool {
+	nodes, err := w.lister.List(labels.Everything())
+	if err != nil {
+		dlog.Errorf(ctx, "unable to list nodes: %v", err)
+		return false
+	}
+
+	// Create the initial snapshot
+	w.lock.Lock()
+	defer w.lock.Unlock()
+	changed := false
+	dlog.Infof(ctx, "Scanning %d nodes", len(nodes))
+	for _, node := range nodes {
+		if w.addLocked(nodeSubnets(ctx, node)) {
+			changed = true
+		}
+	}
+	if changed {
+		dlog.Infof(ctx, "Found %d subnets", len(w.subnets))
+		w.changed = time.Now()
+	} else {
+		dlog.Info(ctx, "No subnets found")
+	}
+	return changed
+}


it seems like this function combines initialization with a precondition check (i.e. "Are we actually able to watch subnets from nodes?") -- that's okay, but calling it twice in a row will make the second call return false, no? Maybe we should short-circuit return if w.subnets is already populated -- after all, if they're there then getting them is definitely viable

josecv · 2021-09-17T17:45:44Z

cmd/traffic/cmd/manager/internal/cluster/podwatcher.go

@@ -0,0 +1,247 @@
+package cluster


same comments apply here as for the nodewatcher :)

The kates package provides nice and simple to use generalisations, but it also introduces overhead and removes the ability to control some aspects of watching. This commit replaces the traffic-manager's use of the kates package with more standard k8s APIs. Signed-off-by: Thomas Hallgren <[email protected]>

Fixes the ``` goroutine "/daemon/watch-cluster-info" exited with error: PANIC: close of closed channel ``` that happens the second time the traffic-manager reports status info. Signed-off-by: Thomas Hallgren <[email protected]>

Adds two environment variables to the traffic manager that controls how it discovers what subnets the cluster is using for the pods. The environment variables are configurable in the Helm chart. Signed-off-by: Thomas Hallgren <[email protected]>

Signed-off-by: Thomas Hallgren <[email protected]>

thallgren · 2021-09-18T16:06:53Z

The watch function was never called, but that only meant that future updates to podCIDRs after the initial scan where undetected. The initial scan happened anyway (in viable). Not good of course, but not a case of "none of this will work" either.

Creates a new `subnet.Set` datatype with methods for adding, deleting, and appending `net.IPNet` instances. Signed-off-by: Thomas Hallgren <[email protected]>

Signed-off-by: Thomas Hallgren <[email protected]>

josecv · 2021-09-20T17:19:17Z

charts/telepresence/values.yaml

@@ -79,7 +79,7 @@ podCIDRs: []
 #  nodePodCIDRs extract CIDRs from the podCIDR and podCIDRs field of the Node Spec.
 #  coverPodIPs  extract IPs from the podIP and podIPs field of the Pod Status and compute the CIDRs needed to cover those IPs.
 #  environment  use CIDRs listed in the space separated POD_CIDRS environment variable verbatim.
-#  auto         first tru nodePodCIDRs and if that fails, tru coverPodIPs
+#  auto         first try nodePodCIDRs and if that fails, tru coverPodIPs


still says tru coverPodIPs :^)

The 2 minutes timeout would sometimes fail. Signed-off-by: Thomas Hallgren <[email protected]>

If the read at line 901 returned an error other than `io.EOF`, the surrounding loop would continue in infinity. This commit ensures that the loop brakes on any error. Signed-off-by: Thomas Hallgren <[email protected]>

Signed-off-by: Thomas Hallgren <[email protected]>

thallgren requested a review from josecv September 17, 2021 15:03

thallgren self-assigned this Sep 17, 2021

thallgren added bug Something isn't working feature New feature or enhancement request labels Sep 17, 2021

thallgren added this to the 2021 Cycle 6 milestone Sep 17, 2021

josecv reviewed Sep 17, 2021

View reviewed changes

thallgren force-pushed the thallgren/replace-agent-kates branch 2 times, most recently from b7b0415 to 497ec40 Compare September 18, 2021 14:59

thallgren added 4 commits September 18, 2021 17:04

Let constructor of nodeWatcher and podWatcher add the event handler.

88b8118

Signed-off-by: Thomas Hallgren <[email protected]>

thallgren force-pushed the thallgren/replace-agent-kates branch from 497ec40 to 3c13df6 Compare September 18, 2021 15:05

thallgren added 4 commits September 19, 2021 08:45

Break set of net.IPNet into its own type.

0c08a68

Creates a new `subnet.Set` datatype with methods for adding, deleting, and appending `net.IPNet` instances. Signed-off-by: Thomas Hallgren <[email protected]>

Fix bad comments.

6e4d87c

Signed-off-by: Thomas Hallgren <[email protected]>

Make viable() callable more than once.

0482643

Signed-off-by: Thomas Hallgren <[email protected]>

Fix lint issues.

07706f3

Signed-off-by: Thomas Hallgren <[email protected]>

thallgren force-pushed the thallgren/replace-agent-kates branch from 3c13df6 to 07706f3 Compare September 19, 2021 06:45

josecv approved these changes Sep 20, 2021

View reviewed changes

thallgren force-pushed the thallgren/replace-agent-kates branch 2 times, most recently from 0045fb6 to 07706f3 Compare September 21, 2021 10:02

thallgren added 2 commits September 21, 2021 12:32

Increase lint timeout to 3 minutes.

2bb3e88

The 2 minutes timeout would sometimes fail. Signed-off-by: Thomas Hallgren <[email protected]>

Fix test timeout when body.Read() returns error other than io.EOF

6bbc30f

If the read at line 901 returned an error other than `io.EOF`, the surrounding loop would continue in infinity. This commit ensures that the loop brakes on any error. Signed-off-by: Thomas Hallgren <[email protected]>

donnyyung mentioned this pull request Sep 21, 2021

Add gather-logs command #2067

Merged

4 tasks

thallgren force-pushed the thallgren/replace-agent-kates branch from f2326be to b592579 Compare September 21, 2021 13:42

Fix weird root log checks in CLI test.

3fd1746

Signed-off-by: Thomas Hallgren <[email protected]>

thallgren force-pushed the thallgren/replace-agent-kates branch from b592579 to 3fd1746 Compare September 21, 2021 14:23

thallgren merged commit d4f8deb into release/v2 Sep 21, 2021

thallgren deleted the thallgren/replace-agent-kates branch September 21, 2021 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve traffic-managers podCIDR discovery #2093

Improve traffic-managers podCIDR discovery #2093

thallgren commented Sep 17, 2021

josecv left a comment

josecv Sep 17, 2021

josecv Sep 17, 2021

josecv Sep 17, 2021

josecv Sep 17, 2021

josecv Sep 17, 2021

thallgren commented Sep 18, 2021

josecv Sep 20, 2021

Improve traffic-managers podCIDR discovery #2093

Improve traffic-managers podCIDR discovery #2093

Conversation

thallgren commented Sep 17, 2021

Description

Checklist

josecv left a comment

Choose a reason for hiding this comment

josecv Sep 17, 2021

Choose a reason for hiding this comment

josecv Sep 17, 2021

Choose a reason for hiding this comment

josecv Sep 17, 2021

Choose a reason for hiding this comment

josecv Sep 17, 2021

Choose a reason for hiding this comment

josecv Sep 17, 2021

Choose a reason for hiding this comment

thallgren commented Sep 18, 2021

josecv Sep 20, 2021

Choose a reason for hiding this comment