Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reducing calls to checkConflictingNodes() #915

Merged
merged 1 commit into from
Jul 19, 2017
Merged

Reducing calls to checkConflictingNodes() #915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions calico_node/startup/startup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

checkConflictingNodes(client, node)
}

// Configure the node AS number.
configureASNumber(node)

// Check for conflicting node configuration
checkConflictingNodes(client, node)

// Check expected filesystem
ensureFilesystemAsExpected()

Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should log in these two branches.

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
Expand Down
50 changes: 49 additions & 1 deletion calico_node/startup/startup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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() {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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),
)
})
2 changes: 1 addition & 1 deletion calico_node/tests/st/bgp/test_ipip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down
1 change: 1 addition & 0 deletions master/reference/node/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down