diff --git a/client/client.go b/client/client.go index d2c12b05d..cdecfb645 100644 --- a/client/client.go +++ b/client/client.go @@ -1227,14 +1227,30 @@ func (c *Client) updateNodeFromFingerprint(response *fingerprint.FingerprintResp } // COMPAT(0.10): Remove in 0.10 - if response.Resources != nil && !resourcesAreEqual(c.config.Node.Resources, response.Resources) { - nodeHasChanged = true - c.config.Node.Resources.Merge(response.Resources) + // update the response networks with the config + // if we still have node changes, merge them + if response.Resources != nil { + response.Resources.Networks = updateNetworks( + c.config.Node.Resources.Networks, + response.Resources.Networks, + c.config) + if !c.config.Node.Resources.Equals(response.Resources) { + c.config.Node.Resources.Merge(response.Resources) + nodeHasChanged = true + } } - if response.NodeResources != nil && !c.config.Node.NodeResources.Equals(response.NodeResources) { - nodeHasChanged = true - c.config.Node.NodeResources.Merge(response.NodeResources) + // update the response networks with the config + // if we still have node changes, merge them + if response.NodeResources != nil { + response.NodeResources.Networks = updateNetworks( + c.config.Node.NodeResources.Networks, + response.NodeResources.Networks, + c.config) + if !c.config.Node.NodeResources.Equals(response.NodeResources) { + c.config.Node.NodeResources.Merge(response.NodeResources) + nodeHasChanged = true + } } if nodeHasChanged { @@ -1244,32 +1260,27 @@ func (c *Client) updateNodeFromFingerprint(response *fingerprint.FingerprintResp return c.configCopy.Node } -// resourcesAreEqual is a temporary function to compare whether resources are -// equal. We can use this until we change fingerprinters to set pointers on a -// return type. -func resourcesAreEqual(first, second *structs.Resources) bool { - if first.CPU != second.CPU { - return false - } - if first.MemoryMB != second.MemoryMB { - return false - } - if first.DiskMB != second.DiskMB { - return false - } - if len(first.Networks) != len(second.Networks) { - return false - } - for i, e := range first.Networks { - if len(second.Networks) < i { - return false +// updateNetworks preserves manually configured network options, but +// applies fingerprint updates +func updateNetworks(ns structs.Networks, up structs.Networks, c *config.Config) structs.Networks { + if c.NetworkInterface == "" { + ns = up + } else { + // if a network is configured, use only that network + // use the fingerprinted data + for _, n := range up { + if c.NetworkInterface == n.Device { + ns = []*structs.NetworkResource{n} + } } - f := second.Networks[i] - if !e.Equals(f) { - return false + // if not matched, ns has the old data + } + if c.NetworkSpeed != 0 { + for _, n := range ns { + n.MBits = c.NetworkSpeed } } - return true + return ns } // retryIntv calculates a retry interval value given the base diff --git a/client/client_test.go b/client/client_test.go index a2f0b8e6a..ea034ddf5 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -1243,6 +1243,81 @@ func TestClient_UpdateNodeFromDevicesAccumulates(t *testing.T) { } +// TestClient_UpdateNodeFromFingerprintKeepsConfig asserts manually configured +// network interfaces take precedence over fingerprinted ones. +func TestClient_UpdateNodeFromFingerprintKeepsConfig(t *testing.T) { + t.Parallel() + + // Client without network configured updates to match fingerprint + client, cleanup := TestClient(t, nil) + defer cleanup() + // capture the platform fingerprinted device name for the next test + dev := client.config.Node.NodeResources.Networks[0].Device + client.updateNodeFromFingerprint(&fingerprint.FingerprintResponse{ + NodeResources: &structs.NodeResources{ + Cpu: structs.NodeCpuResources{CpuShares: 123}, + Networks: []*structs.NetworkResource{{Device: "any-interface"}}, + }, + Resources: &structs.Resources{ + CPU: 80, + Networks: []*structs.NetworkResource{{Device: "any-interface"}}, + }, + }) + assert.Equal(t, int64(123), client.config.Node.NodeResources.Cpu.CpuShares) + assert.Equal(t, "any-interface", client.config.Node.NodeResources.Networks[0].Device) + assert.Equal(t, 80, client.config.Node.Resources.CPU) + assert.Equal(t, "any-interface", client.config.Node.Resources.Networks[0].Device) + + // Client with network interface configured keeps the config + // setting on update + name := "TestClient_UpdateNodeFromFingerprintKeepsConfig2" + client, cleanup = TestClient(t, func(c *config.Config) { + c.NetworkInterface = dev + c.Node.Name = name + // Node is already a mock.Node, with a device + c.Node.NodeResources.Networks[0].Device = dev + c.Node.Resources.Networks = c.Node.NodeResources.Networks + }) + defer cleanup() + client.updateNodeFromFingerprint(&fingerprint.FingerprintResponse{ + NodeResources: &structs.NodeResources{ + Cpu: structs.NodeCpuResources{CpuShares: 123}, + Networks: []*structs.NetworkResource{ + {Device: "any-interface", MBits: 20}, + {Device: dev, MBits: 20}, + }, + }, + Resources: &structs.Resources{ + CPU: 80, + Networks: []*structs.NetworkResource{{Device: "any-interface"}}, + }, + }) + assert.Equal(t, int64(123), client.config.Node.NodeResources.Cpu.CpuShares) + // only the configured device is kept + assert.Equal(t, 1, len(client.config.Node.NodeResources.Networks)) + assert.Equal(t, dev, client.config.Node.NodeResources.Networks[0].Device) + // network speed updates to the configured network are kept + assert.Equal(t, 20, client.config.Node.NodeResources.Networks[0].MBits) + assert.Equal(t, 80, client.config.Node.Resources.CPU) + assert.Equal(t, dev, client.config.Node.Resources.Networks[0].Device) + + // Network speed is applied to all NetworkResources + client.config.NetworkInterface = "" + client.config.NetworkSpeed = 100 + client.updateNodeFromFingerprint(&fingerprint.FingerprintResponse{ + NodeResources: &structs.NodeResources{ + Cpu: structs.NodeCpuResources{CpuShares: 123}, + Networks: []*structs.NetworkResource{{Device: "any-interface", MBits: 20}}, + }, + Resources: &structs.Resources{ + CPU: 80, + Networks: []*structs.NetworkResource{{Device: "any-interface"}}, + }, + }) + assert.Equal(t, "any-interface", client.config.Node.NodeResources.Networks[0].Device) + assert.Equal(t, 100, client.config.Node.NodeResources.Networks[0].MBits) +} + func TestClient_computeAllocatedDeviceStats(t *testing.T) { logger := testlog.HCLogger(t) c := &Client{logger: logger} diff --git a/client/fingerprint/network.go b/client/fingerprint/network.go index 2d706a1e9..9cc4fcc65 100644 --- a/client/fingerprint/network.go +++ b/client/fingerprint/network.go @@ -67,7 +67,9 @@ func (f *NetworkFingerprint) Fingerprint(req *FingerprintRequest, resp *Fingerpr intf, err := f.findInterface(cfg.NetworkInterface) switch { case err != nil: - return fmt.Errorf("Error while detecting network interface during fingerprinting: %v", err) + return fmt.Errorf("Error while detecting network interface %s during fingerprinting: %v", + cfg.NetworkInterface, + err) case intf == nil: // No interface could be found return nil diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 1a5cdaf43..746c5e3cf 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -263,7 +263,7 @@ func setImplicitConstraints(j *structs.Job) { found := false for _, c := range tg.Constraints { - if c.Equal(vaultConstraint) { + if c.Equals(vaultConstraint) { found = true break } @@ -288,7 +288,7 @@ func setImplicitConstraints(j *structs.Job) { found := false for _, c := range tg.Constraints { - if c.Equal(sigConstraint) { + if c.Equals(sigConstraint) { found = true break } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 43dd3c438..4a975764c 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1715,7 +1715,7 @@ type Resources struct { DiskMB int IOPS int // COMPAT(0.10): Only being used to issue warnings Networks Networks - Devices []*RequestedDevice + Devices ResourceDevices } const ( @@ -1771,6 +1771,7 @@ func (r *Resources) Validate() error { } // Merge merges this resource with another resource. +// COMPAT(0.10): Remove in 0.10 func (r *Resources) Merge(other *Resources) { if other.CPU != 0 { r.CPU = other.CPU @@ -1789,6 +1790,52 @@ func (r *Resources) Merge(other *Resources) { } } +// COMPAT(0.10): Remove in 0.10 +func (r *Resources) Equals(o *Resources) bool { + if r == o { + return true + } + if r == nil || o == nil { + return false + } + return r.CPU == o.CPU && + r.MemoryMB == o.MemoryMB && + r.DiskMB == o.DiskMB && + r.IOPS == o.IOPS && + r.Networks.Equals(&o.Networks) && + r.Devices.Equals(&o.Devices) +} + +// COMPAT(0.10): Remove in 0.10 +// ResourceDevices are part of Resources +type ResourceDevices []*RequestedDevice + +// COMPAT(0.10): Remove in 0.10 +// Equals ResourceDevices as set keyed by Name +func (d *ResourceDevices) Equals(o *ResourceDevices) bool { + if d == o { + return true + } + if d == nil || o == nil { + return false + } + if len(*d) != len(*o) { + return false + } + m := make(map[string]*RequestedDevice, len(*d)) + for _, e := range *d { + m[e.Name] = e + } + for _, oe := range *o { + de, ok := m[oe.Name] + if !ok || !de.Equals(oe) { + return false + } + } + return true +} + +// COMPAT(0.10): Remove in 0.10 func (r *Resources) Canonicalize() { // Ensure that an empty and nil slices are treated the same to avoid scheduling // problems since we use reflect DeepEquals. @@ -1807,6 +1854,7 @@ func (r *Resources) Canonicalize() { // MeetsMinResources returns an error if the resources specified are less than // the minimum allowed. // This is based on the minimums defined in the Resources type +// COMPAT(0.10): Remove in 0.10 func (r *Resources) MeetsMinResources() error { var mErr multierror.Error minResources := MinResources() @@ -1855,6 +1903,7 @@ func (r *Resources) Copy() *Resources { } // NetIndex finds the matching net index using device name +// COMPAT(0.10): Remove in 0.10 func (r *Resources) NetIndex(n *NetworkResource) int { return r.Networks.NetIndex(n) } @@ -1862,6 +1911,7 @@ func (r *Resources) NetIndex(n *NetworkResource) int { // Superset checks if one set of resources is a superset // of another. This ignores network resources, and the NetworkIndex // should be used for that. +// COMPAT(0.10): Remove in 0.10 func (r *Resources) Superset(other *Resources) (bool, string) { if r.CPU < other.CPU { return false, "cpu" @@ -1877,6 +1927,7 @@ func (r *Resources) Superset(other *Resources) (bool, string) { // Add adds the resources of the delta to this, potentially // returning an error if not possible. +// COMPAT(0.10): Remove in 0.10 func (r *Resources) Add(delta *Resources) error { if delta == nil { return nil @@ -1897,6 +1948,7 @@ func (r *Resources) Add(delta *Resources) error { return nil } +// COMPAT(0.10): Remove in 0.10 func (r *Resources) GoString() string { return fmt.Sprintf("*%#v", *r) } @@ -2074,11 +2126,24 @@ type RequestedDevice struct { // Constraints are a set of constraints to apply when selecting the device // to use. - Constraints []*Constraint + Constraints Constraints // Affinities are a set of affinites to apply when selecting the device // to use. - Affinities []*Affinity + Affinities Affinities +} + +func (r *RequestedDevice) Equals(o *RequestedDevice) bool { + if r == o { + return true + } + if r == nil || o == nil { + return false + } + return r.Name == o.Name && + r.Count == o.Count && + r.Constraints.Equals(&o.Constraints) && + r.Affinities.Equals(&o.Affinities) } func (r *RequestedDevice) Copy() *RequestedDevice { @@ -2249,15 +2314,9 @@ func (n *NodeResources) Equals(o *NodeResources) bool { if !n.Disk.Equals(&o.Disk) { return false } - - if len(n.Networks) != len(o.Networks) { + if !n.Networks.Equals(&o.Networks) { return false } - for i, n := range n.Networks { - if !n.Equals(o.Networks[i]) { - return false - } - } // Check the devices if !DevicesEquals(n.Devices, o.Devices) { @@ -2267,7 +2326,28 @@ func (n *NodeResources) Equals(o *NodeResources) bool { return true } -// DevicesEquals returns true if the two device arrays are equal +// Equals equates Networks as a set +func (n *Networks) Equals(o *Networks) bool { + if n == o { + return true + } + if n == nil || o == nil { + return false + } + if len(*n) != len(*o) { + return false + } + for _, ne := range *n { + for _, oe := range *o { + if !ne.Equals(oe) { + return false + } + } + } + return true +} + +// DevicesEquals returns true if the two device arrays are set equal func DevicesEquals(d1, d2 []*NodeDeviceResource) bool { if len(d1) != len(d2) { return false @@ -6415,10 +6495,11 @@ type Constraint struct { } // Equal checks if two constraints are equal -func (c *Constraint) Equal(o *Constraint) bool { - return c.LTarget == o.LTarget && - c.RTarget == o.RTarget && - c.Operand == o.Operand +func (c *Constraint) Equals(o *Constraint) bool { + return c == o || + c.LTarget == o.LTarget && + c.RTarget == o.RTarget && + c.Operand == o.Operand } func (c *Constraint) Copy() *Constraint { @@ -6494,6 +6575,29 @@ func (c *Constraint) Validate() error { return mErr.ErrorOrNil() } +type Constraints []*Constraint + +// Equals compares Constraints as a set +func (xs *Constraints) Equals(ys *Constraints) bool { + if xs == ys { + return true + } + if xs == nil || ys == nil { + return false + } + if len(*xs) != len(*ys) { + return false + } + for _, x := range *xs { + for _, y := range *ys { + if !x.Equals(y) { + return false + } + } + } + return true +} + // Affinity is used to score placement options based on a weight type Affinity struct { LTarget string // Left-hand target @@ -6504,11 +6608,12 @@ type Affinity struct { } // Equal checks if two affinities are equal -func (a *Affinity) Equal(o *Affinity) bool { - return a.LTarget == o.LTarget && - a.RTarget == o.RTarget && - a.Operand == o.Operand && - a.Weight == o.Weight +func (a *Affinity) Equals(o *Affinity) bool { + return a == o || + a.LTarget == o.LTarget && + a.RTarget == o.RTarget && + a.Operand == o.Operand && + a.Weight == o.Weight } func (a *Affinity) Copy() *Affinity { @@ -6589,6 +6694,29 @@ type Spread struct { str string } +type Affinities []*Affinity + +// Equals compares Affinities as a set +func (xs *Affinities) Equals(ys *Affinities) bool { + if xs == ys { + return true + } + if xs == nil || ys == nil { + return false + } + if len(*xs) != len(*ys) { + return false + } + for _, x := range *xs { + for _, y := range *ys { + if !x.Equals(y) { + return false + } + } + } + return true +} + func (s *Spread) Copy() *Spread { if s == nil { return nil