Reducing calls to checkConflictingNodes()

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

calico_node/startup/startup.go

@@ -84,14 +84,17 @@ func main() {
 	node := getNode(client, nodeName)
 
 	// Configure and verify the node IP addresses and subnets.
-	configureIPsAndSubnets(node)
+	checkConflicts := configureIPsAndSubnets(node)
+
+	// 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" {
+		checkConflictingNodes(client, node)
+	}
 
 	// Configure the node AS number.
 	configureASNumber(node)
 
-	// Check for conflicting node configuration
-	checkConflictingNodes(client, node)
-
 	// Check expected filesystem
 	ensureFilesystemAsExpected()
 
@@ -235,8 +238,8 @@ 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) {
+// information to use for BGP.  This returns true if we detect a change in Node IP address.
+func configureIPsAndSubnets(node *api.Node) bool {
 	// If the node resource currently has no BGP configuration, add an empty
 	// set of configuration as it makes the processing below easier, and we
 	// must end up configuring some BGP fields before we complete.
@@ -245,6 +248,9 @@ func configureIPsAndSubnets(node *api.Node) {
 		node.Spec.BGP = &api.NodeBGPSpec{}
 	}
 
+	oldIpv4 := node.Spec.BGP.IPv4Address
+	oldIpv6 := node.Spec.BGP.IPv6Address
+
 	// Determine the autodetection type for IPv4 and IPv6.  Note that we
 	// only autodetect IPv4 when it has not been specified.  IPv6 must be
 	// explicitly requested using the "autodetect" value.
@@ -308,6 +314,17 @@ func configureIPsAndSubnets(node *api.Node) {
 		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) {
+		log.Info("Node IPv4 changed, will check for conflicts")
+		return true
+	}
+	if (oldIpv6 == nil && node.Spec.BGP.IPv6Address != nil) || (oldIpv6 != nil && !node.Spec.BGP.IPv6Address.IP.Equal(oldIpv6.IP)) {
+		log.Info("Node IPv6 changed, will check for conflicts")
+		return true
+	}
+
+	return false
 }
 
 // fetchAndValidateIPAndNetwork fetches and validates the IP configuration from

calico_node/startup/startup_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/projectcalico/libcalico-go/lib/client"
 	"github.com/projectcalico/libcalico-go/lib/ipip"
 	"github.com/projectcalico/libcalico-go/lib/testutils"
+	"github.com/projectcalico/libcalico-go/lib/net"
 )
 
 var exitCode int
@@ -35,6 +36,28 @@ func fakeExitFunction(ec int) {
 	exitCode = ec
 }
 
+// makeNode creates an api.Node with some BGPSpec info populated.
+func makeNode(ipv4 string, ipv6 string) *api.Node {
+	ip4, ip4net, _ := net.ParseCIDR(ipv4)
+	ip4net.IP = ip4.IP
+
+	ip6, ip6net, _ := net.ParseCIDR(ipv6)
+	// Guard against nil here in case we pass in an empty string for IPv6.
+	if ip6 != nil {
+		ip6net.IP = ip6.IP
+	}
+
+	n := &api.Node{
+		Spec: api.NodeSpec{
+			BGP: &api.NodeBGPSpec{
+				IPv4Address: ip4net,
+				IPv6Address: ip6net,
+			},
+		},
+	}
+	return n
+}
+
 var _ = Describe("Non-etcd related tests", func() {
 
 	Describe("Logging tests", func() {
@@ -69,7 +92,7 @@ type EnvItem struct {
 }
 
 var _ = Describe("FV tests against a real etcd", func() {
-	changedEnvVars := []string{"CALICO_IPV4POOL_CIDR", "CALICO_IPV6POOL_CIDR", "NO_DEFAULT_POOLS", "CALICO_IPV4POOL_IPIP", "CALICO_IPV6POOL_NAT_OUTGOING", "CALICO_IPV4POOL_NAT_OUTGOING"}
+	changedEnvVars := []string{"CALICO_IPV4POOL_CIDR", "CALICO_IPV6POOL_CIDR", "NO_DEFAULT_POOLS", "CALICO_IPV4POOL_IPIP", "CALICO_IPV6POOL_NAT_OUTGOING", "CALICO_IPV4POOL_NAT_OUTGOING", "IP"}
 
 	BeforeEach(func() {
 		for _, envName := range changedEnvVars {
@@ -291,3 +314,28 @@ var _ = Describe("FV tests against a real etcd", func() {
 		}
 	})
 })
+
+var _ = Describe("UT for Node IP assignment and conflict checking.", func() {
+
+	DescribeTable("Test variations on how IPs are detected.",
+		func(node *api.Node, items []EnvItem, expected bool) {
+
+			for _, item := range items {
+				os.Setenv(item.key, item.value)
+			}
+
+			check := configureIPsAndSubnets(node)
+
+			Expect(check).To(Equal(expected))
+		},
+
+		Entry("Test with no \"IP\" env var set", &api.Node{}, []EnvItem{{"IP", ""}}, true),
+		Entry("Test with \"IP\" env var set to IP", &api.Node{}, []EnvItem{{"IP", "192.168.1.10/24"}}, true),
+		Entry("Test with \"IP\" env var set to IP and BGP spec populated with same IP", makeNode("192.168.1.10/24", ""), []EnvItem{{"IP", "192.168.1.10/24"}}, false),
+		Entry("Test with \"IP\" env var set to IP and BGP spec populated with different IP", makeNode("192.168.1.10/24", ""), []EnvItem{{"IP", "192.168.1.11/24"}}, true),
+		Entry("Test with no \"IP6\" env var set", &api.Node{}, []EnvItem{{"IP6", ""}}, true),
+		Entry("Test with \"IP6\" env var set to IP", &api.Node{}, []EnvItem{{"IP6", "2001:db8:85a3:8d3:1319:8a2e:370:7348/32"}}, true),
+		Entry("Test with \"IP6\" env var set to IP and BGP spec populated with same IP", makeNode("192.168.1.10/24", "2001:db8:85a3:8d3:1319:8a2e:370:7348/32"), []EnvItem{{"IP", "192.168.1.10/24"}, {"IP6", "2001:db8:85a3:8d3:1319:8a2e:370:7348/32"}}, false),
+		Entry("Test with \"IP6\" env var set to IP and BGP spec populated with different IP", makeNode("192.168.1.10/24", "2001:db8:85a3:8d3:1319:8a2e:370:7348/32"), []EnvItem{{"IP", "192.168.1.10/24"}, {"IP6", "2001:db8:85a3:8d3:1319:8a2e:370:7349/32"}}, true),
+	)
+})

calico_node/tests/st/bgp/test_ipip.py

@@ -281,7 +281,7 @@ def check():
                 assert self.get_tunl_tx(host1) == orig_tx + 2
             else:
                 assert self.get_tunl_tx(host1) == orig_tx
-        retry_until_success(check)
+        retry_until_success(check, retries=10)
 
     def get_tunl_tx(self, host):
         """

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. | bool | false |
 | AS | The AS number for this node. When specified, the value is saved in the node resource configuration for this host, overriding any previously configured value. When omitted, if an AS number has been previously configured in the node resource, that AS number is used for the peering.  When omitted, if an AS number has not yet been configured in the node resource, the node will use the global value (managed through `calicoctl config set/get asnumber`). | int | |
 | DATASTORE_TYPE | Type of datastore. | kubernetes, etcdv2 | etcdv2 |
 | WAIT_FOR_DATASTORE | Wait for connection to datastore before starting. If a successful connection is not made, node will shutdown. | boolean | false |