diff --git a/client/client.go b/client/client.go index baf463dd3..63a6cdc0b 100644 --- a/client/client.go +++ b/client/client.go @@ -212,7 +212,7 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulServic allocUpdates: make(chan *structs.Allocation, 64), shutdownCh: make(chan struct{}), triggerDiscoveryCh: make(chan struct{}), - triggerNodeUpdate: make(chan struct{}, 64), + triggerNodeUpdate: make(chan struct{}, 8), serversDiscoveredCh: make(chan struct{}), } @@ -1028,7 +1028,7 @@ func resourcesAreEqual(first, second *structs.Resources) bool { return false } f := second.Networks[i] - if e != f { + if !e.Equals(f) { return false } } @@ -1538,11 +1538,12 @@ func (c *Client) updateNode() { // it will update the client node copy and re-register the node. func (c *Client) watchNodeUpdates() { var hasChanged bool - syncTicker := time.NewTicker(c.retryIntv(nodeUpdateRetryIntv)) + timer := time.NewTimer(c.retryIntv(nodeUpdateRetryIntv)) + defer timer.Stop() for { select { - case <-syncTicker.C: + case <-timer.C: if !hasChanged { continue } @@ -1557,8 +1558,6 @@ func (c *Client) watchNodeUpdates() { c.retryRegisterNode() hasChanged = false - syncTicker.Stop() - syncTicker = time.NewTicker(c.retryIntv(nodeUpdateRetryIntv)) case <-c.triggerNodeUpdate: hasChanged = true case <-c.shutdownCh: diff --git a/client/client_test.go b/client/client_test.go index 513bd5499..de5d35ee9 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -125,51 +125,6 @@ func TestClient_Fingerprint(t *testing.T) { require.NotEqual("", node.Attributes["driver.mock_driver"]) } -func TestClient_TriggerNodeUpdate(t *testing.T) { - driver.CheckForMockDriver(t) - t.Parallel() - - // These constants are only defined when nomad_test is enabled, so these fail - // our linter without explicit disabling. - c1 := TestClient(t, func(c *config.Config) { - c.Options = map[string]string{ - driver.ShutdownPeriodicAfter: "true", // nolint: varcheck - driver.ShutdownPeriodicDuration: "3", // nolint: varcheck - } - }) - defer c1.Shutdown() - - mockDriverName := "driver.mock_driver" - - go c1.watchNodeUpdates() - c1.updateNode() - // This needs to be directly called as otherwise the client hangs on - // attempt to register with a server. Specifically, retryRegisterNode is - // blocking - - // Test that the client's copy of the node is also updated - testutil.WaitForResult(func() (bool, error) { - mockDriverStatusCopy := c1.configCopy.Node.Attributes[mockDriverName] - if mockDriverStatusCopy == "" { - return false, fmt.Errorf("mock driver attribute should be set on the client") - } - return true, nil - }, func(err error) { - t.Fatalf("err: %v", err) - }) - - // Test that the client's copy of the node is also updated - testutil.WaitForResult(func() (bool, error) { - mockDriverStatusCopy := c1.configCopy.Node.Attributes[mockDriverName] - if mockDriverStatusCopy != "" { - return false, fmt.Errorf("mock driver attribute should not be set on the client") - } - return true, nil - }, func(err error) { - t.Fatalf("err: %v", err) - }) -} - func TestClient_Fingerprint_Periodic(t *testing.T) { driver.CheckForMockDriver(t) t.Parallel() diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 526308533..7f3e2e3c3 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1432,6 +1432,50 @@ type NetworkResource struct { DynamicPorts []Port // Host Dynamically assigned ports } +func (nr *NetworkResource) Equals(other *NetworkResource) bool { + if nr.Device != other.Device { + return false + } + + if nr.CIDR != other.CIDR { + return false + } + + if nr.IP != other.IP { + return false + } + + if nr.MBits != other.MBits { + return false + } + + if len(nr.ReservedPorts) != len(other.ReservedPorts) { + return false + } + + for i, port := range nr.ReservedPorts { + if len(other.ReservedPorts) <= i { + return false + } + if port != other.ReservedPorts[i] { + return false + } + } + + if len(nr.DynamicPorts) != len(other.DynamicPorts) { + return false + } + for i, port := range nr.DynamicPorts { + if len(other.DynamicPorts) <= i { + return false + } + if port != other.DynamicPorts[i] { + return false + } + } + return true +} + func (n *NetworkResource) Canonicalize() { // Ensure that an empty and nil slices are treated the same to avoid scheduling // problems since we use reflect DeepEquals. diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index e7d0fb0f5..07b10a256 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/nomad/helper/uuid" "github.com/kr/pretty" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestJob_Validate(t *testing.T) { @@ -3104,3 +3105,162 @@ func TestTaskEventPopulate(t *testing.T) { } } } + +func TestNetworkResourcesEquals(t *testing.T) { + require := require.New(t) + var networkResourcesTest = []struct { + input []*NetworkResource + expected bool + errorMsg string + }{ + { + []*NetworkResource{ + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{{"web", 80}}, + }, + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{{"web", 80}}, + }, + }, + true, + "Equal network resources should return true", + }, + { + []*NetworkResource{ + &NetworkResource{ + IP: "10.0.0.0", + MBits: 50, + ReservedPorts: []Port{{"web", 80}}, + }, + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{{"web", 80}}, + }, + }, + false, + "Different IP addresses should return false", + }, + { + []*NetworkResource{ + &NetworkResource{ + IP: "10.0.0.1", + MBits: 40, + ReservedPorts: []Port{{"web", 80}}, + }, + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{{"web", 80}}, + }, + }, + false, + "Different MBits values should return false", + }, + { + []*NetworkResource{ + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{{"web", 80}}, + }, + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{{"web", 80}, {"web", 80}}, + }, + }, + false, + "Different ReservedPorts lengths should return false", + }, + { + []*NetworkResource{ + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{{"web", 80}}, + }, + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{}, + }, + }, + false, + "Empty and non empty ReservedPorts values should return false", + }, + { + []*NetworkResource{ + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{{"web", 80}}, + }, + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + ReservedPorts: []Port{{"notweb", 80}}, + }, + }, + false, + "Different valued ReservedPorts values should return false", + }, + { + []*NetworkResource{ + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + DynamicPorts: []Port{{"web", 80}}, + }, + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + DynamicPorts: []Port{{"web", 80}, {"web", 80}}, + }, + }, + false, + "Different DynamicPorts lengths should return false", + }, + { + []*NetworkResource{ + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + DynamicPorts: []Port{{"web", 80}}, + }, + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + DynamicPorts: []Port{}, + }, + }, + false, + "Empty and non empty DynamicPorts values should return false", + }, + { + []*NetworkResource{ + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + DynamicPorts: []Port{{"web", 80}}, + }, + &NetworkResource{ + IP: "10.0.0.1", + MBits: 50, + DynamicPorts: []Port{{"notweb", 80}}, + }, + }, + false, + "Different valued DynamicPorts values should return false", + }, + } + for _, testCase := range networkResourcesTest { + first := testCase.input[0] + second := testCase.input[1] + require.Equal(testCase.expected, first.Equals(second), testCase.errorMsg) + } +}