Reducing calls to checkConflictingNodes() #915

heschlie · 2017-07-14T18:20:10Z

Description

This fix should result in less calls to checkConflictingNodes() which makes API calls to client.Nodes.List() which is a very expensive call. This should result in less strain during large clusters with node churn.

I'm not sure if this should have a release note tied to it.

Release Notes

calico/node will now only check for conflicting Node IPs when initially getting an IP or when a change in IP is detected.  This should reduce the load on the cluster when a large number of nodes are restarting.

robbrockbank · 2017-07-14T18:22:47Z

calico_node/startup/startup.go

+			// we need to update it and check for conflicts.
+			if node.Spec.BGP.IPv6Address == nil || !node.Spec.BGP.IPv6Address.IP.Equal(cidr.IP) {
+				node.Spec.BGP.IPv6Address = cidr
+				checkConflictingNodes(client, node)


you potentially call this twice - once for ipv4 and once for ipv6.

Gah you're right, which kinda defeats the purpose, will refactor it to only call once

caseydavenport · 2017-07-14T20:11:24Z

calico_node/startup/startup.go

@@ -84,14 +84,11 @@ func main() {
 	node := getNode(client, nodeName)

 	// Configure and verify the node IP addresses and subnets.
-	configureIPsAndSubnets(node)
+	configureIPsAndSubnets(client, node)


minor nit - rather than pass the client here it might be nicer to make it so configureIPsAndSubnets returns whether or not the an IP on the node changed, and we can use it below.

caseydavenport · 2017-07-14T20:13:05Z

calico_node/startup/startup.go

-			node.Spec.BGP.IPv4Address = cidr
+			// We autodetected an IPv4 address, if we have no previous IP, or it is different than the previous IP,
+			// we need to update it and check for conflicts.
+			if node.Spec.BGP.IPv4Address == nil || !node.Spec.BGP.IPv4Address.IP.Equal(cidr.IP) {


Hm, this only checks the IP part of the CIDR, not the netmask.

I think we might want to set node.Spec.BGP.IPv4Address = cidr always - it's possible that the IP stays the same but the subnet changes.

caseydavenport · 2017-07-14T20:20:09Z

In the original issue, there was also a suggestion that we add a config option to disable the check entirely - @heschlie could you add that to this PR?

caseydavenport

A couple of comments.

caseydavenport · 2017-07-14T21:59:22Z

@heschlie we'll need a corresponding docs PR to document the new configuration option. Could you link to it here once you've created it?

heschlie · 2017-07-14T22:01:38Z

Does it need a separate PR or could/should it just be in this PR?

caseydavenport · 2017-07-14T22:45:33Z

@heschlie oops, forgot we're in the Calico repo now!

It can/should share this PR.

robbrockbank · 2017-07-17T22:36:14Z

calico_node/startup/startup.go

@@ -267,7 +274,11 @@ func configureIPsAndSubnets(node *api.Node) {
 		adm := os.Getenv("IP_AUTODETECTION_METHOD")
 		cidr := autoDetectCIDR(adm, 4)
 		if cidr != nil {
-			// We autodetected an IPv4 address so update the value in the node.
+			// We autodetected an IPv4 address, if we have no previous IP, or it is different than the previous IP,


Sorry I didn't notice before, but this check is not quite sufficient as it only covers the auto-detection case.

I think we also need to handle the case when the IP address has been explicitly specified in the environment.

Yea that's true, I'll add similar checks there as well.

perhaps easier to store the initial IPv4 and IPv6 addresses and do the comparison once at the end of the fn?

I think this comment is wrong now that we've moved the check outside of this branch - should probably just be reverted to what it was.

Same for a few other comments in this PR.

caseydavenport · 2017-07-17T23:56:42Z

calico_node/startup/startup.go

+			if node.Spec.BGP.IPv6Address == nil || !node.Spec.BGP.IPv6Address.IP.Equal(envIp6.IP) {
+				checkConflicts = true
+			}
+			node.Spec.BGP.IPv6Address = envIp6
 		}
 		validateIP(node.Spec.BGP.IPv6Address)
 	}



Would it be nicer and less code to store off the original value at the top of the function, and compare it to the new value at the bottom?

e.g.

oldv4 := node.Spec.BGP.IPv4Address oldv6 := ... // Rest of function. ... if node.Spec.BGP.IPv4Addres != oldv4 || node.Spec.BGP.IPv6Address != oldv6 { ... }

Yup I think I suggested the same thing earlier :-)

Ah, missed that :)

Just driving by.

heschlie · 2017-07-18T22:09:27Z

@caseydavenport @robbrockbank I think this should be good to go now, docs for new env var are in and the configureIPsAndSubnets() call simplified.

caseydavenport · 2017-07-19T02:07:46Z

calico_node/startup/startup.go

@@ -274,7 +285,8 @@ func configureIPsAndSubnets(node *api.Node) {
 		}
 	} else {
 		if ipv4Env != "" {
-			node.Spec.BGP.IPv4Address = parseIPEnvironment("IP", ipv4Env, 4)
+			envIp := parseIPEnvironment("IP", ipv4Env, 4)


I think we should revert this change - not required.

Same for IPv6 below.

caseydavenport · 2017-07-19T02:09:50Z

@heschlie I think we need a unit test for this.

robbrockbank · 2017-07-19T03:26:30Z

calico_node/startup/startup.go

+	if oldIpv4 == nil || !node.Spec.BGP.IPv4Address.IP.Equal(oldIpv4.IP) {
+		checkConflicts = true
+	}
+	if oldIpv6 == nil || !node.Spec.BGP.IPv6Address.IP.Equal(oldIpv6.IP) {


For an IPv4 deployment, oldIpv6 will always be nil every time we start, so we'll always perform the conflict check.

This should be:

if (oldIpv6 == nil && node.Spec.BGP.IPv6Address != nil) || (oldIpv6 != nil && !node.Spec.BGP.IPv6Address.IP.Equal(oldIpv6.IP))

or something like that

robbrockbank · 2017-07-19T03:30:20Z

calico_node/startup/startup.go

 		}
 		validateIP(node.Spec.BGP.IPv6Address)
 	}

+	// Detect if we've seen the IP address change, and flag that we need to check for conflicting Nodes
+	if oldIpv4 == nil || !node.Spec.BGP.IPv4Address.IP.Equal(oldIpv4.IP) {
+		checkConflicts = true


nit: Do you really need to define checkConflicts? You could just return true here.

robbrockbank · 2017-07-19T03:31:01Z

calico_node/startup/startup.go

 		}
 		validateIP(node.Spec.BGP.IPv6Address)
 	}

+	// Detect if we've seen the IP address change, and flag that we need to check for conflicting Nodes
+	if oldIpv4 == nil || !node.Spec.BGP.IPv4Address.IP.Equal(oldIpv4.IP) {


You should log in these two branches.

robbrockbank · 2017-07-19T03:33:14Z

master/reference/node/configuration.md

@@ -16,6 +16,7 @@ The `calico/node` container is primarily configured through environment variable
 | IP6 | The IPv6 address for Calico will bind to. When specified, the address is saved in the  [node resource configuration]({{site.baseurl}}/{{page.version}}/reference/calicoctl/resources/node) for this host, overriding any previously configured value. When omitted, if an address has not yet been configured in the node resource, IPv6 routing is not enabled. When omitted, if an IPv6 address has been previously configured in the node resource, IPv6 is enabled using the already configured address. | IPv6 | |
 | IP_AUTODETECTION_METHOD| The method to use to autodetect the IPv4 address for this host. This is only used when the IPv4 address is being autodetected. See [IP Autodetection methods](#ip-autodetection-methods) for details of the valid methods. | string | first-found |
 | IP6_AUTODETECTION_METHOD| The method to use to autodetect the IPv6 address for this host. This is only used when the IPv6 address is being autodetected. See [IP Autodetection methods](#ip-autodetection-methods) for details of the valid methods. | string | first-found |
+| DISABLE_NODE_IP_CHECK| Skips checks for duplicate Node IPs. This can reduce the load on the cluster when a large number of Nodes are restarting. | string | false |


string -> boolean

robbrockbank · 2017-07-19T03:39:13Z

calico_node/startup/startup.go

+
+	// If we report an IP change (v4 or v6) we should verify there are no
+	// conflicts between Nodes.
+	if checkConflicts && os.Getenv("DISABLE_NODE_IP_CHECK") != "true" {


It feels like we should be more forgiving about the value that people can pass in - I think anything that resembles a boolean true should be sufficient: true t y 1 (case insensitive)

@caseydavenport @heschlie WDYT?

What do we respect for other true/false variables? Seems fine/good to do, but I'm happy to merge without.

Yes I'm happy to merge without too. I'll raise an issue to track better handling of boolean envs.

caseydavenport · 2017-07-19T04:48:39Z

calico_node/tests/st/bgp/test_backends.py

@@ -56,7 +56,7 @@ def test_bgp_backends(self):
            workload_host3 = host3.create_workload("workload3", network=network1, ip=DEFAULT_IPV4_ADDR_3)

            # Allow network to converge
-            self.assert_true(workload_host1.check_can_ping(workload_host2.ip, retries=10))
+            self.assert_true(workload_host1.check_can_ping(workload_host2.ip, retries=15))


hmm.. I wonder why these changes are required.. have we regressed some sort of timing thing? Any ideas @robbrockbank?

No idea - these should not need to be updated for this PR. Perhaps local environment is slower?

I was trying to reduce the number of inconsistent test failures I was seeing, will revert as I don't think it made any difference.

heschlie · 2017-07-19T20:20:02Z

I've Added in some UT's in a table driven test in the startup_test.go

robbrockbank · 2017-07-19T20:51:42Z

calico_node/startup/startup.go

@@ -236,7 +239,7 @@ func getNode(client *client.Client, nodeName string) *api.Node {

 // configureIPsAndSubnets updates the supplied node resource with IP and Subnet
 // information to use for BGP.
-func configureIPsAndSubnets(node *api.Node) {
+func configureIPsAndSubnets(node *api.Node) bool {


One final thing: I think the comment should indicate that it returns true if the node IP addresses are updated.

robbrockbank

LGTM - one final comment, but I don't need to see it again.

Need to squash before merging.

heschlie · 2017-07-19T21:22:36Z

I've updated the UT to do some IPv6 checks as well.

caseydavenport

LGTM

@heschlie could you please squash before merging? Thanks!

caseydavenport · 2017-07-19T21:43:17Z

Oh, and could you tweak the release note to talk about "calico/node" instead of the startup script? That's more user-facing.

This fix should result in less calls to `checkConflictingNodes()` which makes API calls to `client.Nodes.List()` which is a very expensive call. Added in env car check to `DISABLE_NODE_IP_CHECK`, will skip conflict checks if set to "true". Document changes for new env var Feedback update - Minor fixes from feedback - Reverted retry counts in STs - Created some UTs for new functionality in `configureIPsAndSubnets()` Testing IPv6 for changes Rewrote `makeNode()` to create IPv6 as well, added some test cases for IPv6

…-release-v3.21 [release-v3.21] Semaphore Auto Pin Update

heschlie added area/node labels Jul 14, 2017

heschlie added this to the Calico v2.4.0 milestone Jul 14, 2017

heschlie self-assigned this Jul 14, 2017

heschlie requested review from caseydavenport and robbrockbank July 14, 2017 18:20

robbrockbank reviewed Jul 14, 2017

View reviewed changes

caseydavenport reviewed Jul 14, 2017

View reviewed changes

caseydavenport added the release-note-required Change has user-facing impact (no matter how small) label Jul 14, 2017

caseydavenport requested changes Jul 14, 2017

View reviewed changes

robbrockbank reviewed Jul 17, 2017

View reviewed changes

caseydavenport reviewed Jul 17, 2017

View reviewed changes

heschlie force-pushed the fix-871 branch 2 times, most recently from 6950940 to 468b7bd Compare July 18, 2017 22:08

caseydavenport reviewed Jul 19, 2017

View reviewed changes

robbrockbank reviewed Jul 19, 2017

View reviewed changes

caseydavenport reviewed Jul 19, 2017

View reviewed changes

robbrockbank reviewed Jul 19, 2017

View reviewed changes

robbrockbank approved these changes Jul 19, 2017

View reviewed changes

caseydavenport approved these changes Jul 19, 2017

View reviewed changes

heschlie force-pushed the fix-871 branch from e8ed9aa to 8dfd2de Compare July 19, 2017 21:46

heschlie merged commit 765dc25 into projectcalico:master Jul 19, 2017

heschlie mentioned this pull request Jul 19, 2017

Improve checkConflictingNodes() processing to avoid excessive queries on startup #871

Closed

caseydavenport mentioned this pull request Jul 31, 2017

Release 2.4.0 #981

Merged

3 tasks

caseydavenport pushed a commit that referenced this pull request Dec 14, 2021

Merge pull request #915 from projectcalico/semaphore-auto-pin-updates…

1f569ef

…-release-v3.21 [release-v3.21] Semaphore Auto Pin Update

Reducing calls to checkConflictingNodes() #915

Reducing calls to checkConflictingNodes() #915

Conversation

heschlie commented Jul 14, 2017 • edited Loading

Description

Release Notes

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

caseydavenport commented Jul 14, 2017

caseydavenport left a comment

Choose a reason for hiding this comment

caseydavenport commented Jul 14, 2017

heschlie commented Jul 14, 2017

caseydavenport commented Jul 14, 2017 • edited Loading

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

heschlie commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caseydavenport commented Jul 19, 2017

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

heschlie commented Jul 19, 2017

Choose a reason for hiding this comment

robbrockbank left a comment

Choose a reason for hiding this comment

heschlie commented Jul 19, 2017

caseydavenport left a comment

Choose a reason for hiding this comment

caseydavenport commented Jul 19, 2017

heschlie commented Jul 14, 2017 •

edited

Loading

caseydavenport commented Jul 14, 2017 •

edited

Loading