From c45652ab8c113487b9d4fbfb107782cbcf8a85b0 Mon Sep 17 00:00:00 2001 From: Lang Martin Date: Thu, 11 Apr 2019 10:25:19 -0400 Subject: [PATCH] Revert accidental merge of pr #5482 Revert "fingerprint Constraints and Affinities have Equals, as set" This reverts commit 596f16fb5f1a4a6766a57b3311af806d22382609. Revert "client tests assert the independent handling of interface and speed" This reverts commit 7857ac5993a578474d0570819f99b7b6e027de40. Revert "structs missed applying a style change from the review" This reverts commit 658916e3274efa438beadc2535f47109d0c2f0f2. Revert "client, structs comments" This reverts commit be2838d6baa9d382a5013fa80ea016856f28ade2. Revert "client fingerprint updateNetworks preserves the network configuration" This reverts commit fc309cb430e62d8e66267a724f006ae9abe1c63c. Revert "client_test cleanup comments from review" This reverts commit bc0bf4efb9114e699bc662f50c8f12319b6b3445. Revert "client Networks Equals is set equality" This reverts commit f8d432345b54b1953a4a4c719b9269f845e3e573. Revert "struct cleanup indentation in RequestedDevice Equals" This reverts commit f4746411cab328215def6508955b160a53452da3. Revert "struct Equals checks for identity before value checking" This reverts commit 0767a4665ed30ab8d9586a59a74db75d51fd9226. Revert "fix client-test, avoid hardwired platform dependecy on lo0" This reverts commit e89dbb2ab182b6368507dbcd33c3342223eb0ae7. Revert "refactor error in client fingerprint to include the offending data" This reverts commit a7fed726c6e0264d42a58410d840adde780a30f5. Revert "add client updateNodeResources to merge but preserve manual config" This reverts commit 84bd433c7e1d030193e054ec23474380ff3b9032. Revert "refactor struts.RequestedDevice to have its own Equals" This reverts commit 689782524090e51183474516715aa2f34908b8e6. Revert "refactor structs.Resource.Networks to have its own Equals" This reverts commit 49e2e6c77bb3eaa4577772b36c62205061c92fa1. Revert "refactor structs.Resource.Devices to have its own Equals" This reverts commit 4ede9226bb971ae42cc203560ed0029897aec2c9. Revert "add COMPAT(0.10): Remove in 0.10 notes to impl for structs.Resources" This reverts commit 49fbaace5298d5ccf031eb7ebec93906e1d468b5. Revert "add structs.Resources Equals" This reverts commit 8528a2a2a6450e4462a1d02741571b5efcb45f0b. Revert "test that fingerprint resources are updated, net not clobbered" This reverts commit 8ee02ddd23bafc87b9fce52b60c6026335bb722d. --- client/client.go | 69 ++++++-------- client/client_test.go | 75 --------------- client/fingerprint/network.go | 4 +- nomad/job_endpoint.go | 4 +- nomad/structs/structs.go | 168 ++++------------------------------ 5 files changed, 52 insertions(+), 268 deletions(-) diff --git a/client/client.go b/client/client.go index cdecfb6456ce..d2c12b05dafe 100644 --- a/client/client.go +++ b/client/client.go @@ -1227,30 +1227,14 @@ func (c *Client) updateNodeFromFingerprint(response *fingerprint.FingerprintResp } // COMPAT(0.10): Remove in 0.10 - // 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.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.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 response.NodeResources != nil && !c.config.Node.NodeResources.Equals(response.NodeResources) { + nodeHasChanged = true + c.config.Node.NodeResources.Merge(response.NodeResources) } if nodeHasChanged { @@ -1260,27 +1244,32 @@ func (c *Client) updateNodeFromFingerprint(response *fingerprint.FingerprintResp return c.configCopy.Node } -// 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} - } - } - // if not matched, ns has the old data +// 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 c.NetworkSpeed != 0 { - for _, n := range ns { - n.MBits = c.NetworkSpeed + if len(first.Networks) != len(second.Networks) { + return false + } + for i, e := range first.Networks { + if len(second.Networks) < i { + return false + } + f := second.Networks[i] + if !e.Equals(f) { + return false } } - return ns + return true } // retryIntv calculates a retry interval value given the base diff --git a/client/client_test.go b/client/client_test.go index ea034ddf58fb..a2f0b8e6aada 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -1243,81 +1243,6 @@ 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 9cc4fcc658f7..2d706a1e916b 100644 --- a/client/fingerprint/network.go +++ b/client/fingerprint/network.go @@ -67,9 +67,7 @@ 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 %s during fingerprinting: %v", - cfg.NetworkInterface, - err) + return fmt.Errorf("Error while detecting network interface during fingerprinting: %v", err) case intf == nil: // No interface could be found return nil diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 746c5e3cf0e7..1a5cdaf4300e 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.Equals(vaultConstraint) { + if c.Equal(vaultConstraint) { found = true break } @@ -288,7 +288,7 @@ func setImplicitConstraints(j *structs.Job) { found := false for _, c := range tg.Constraints { - if c.Equals(sigConstraint) { + if c.Equal(sigConstraint) { found = true break } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 4a975764cbd5..43dd3c438224 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 ResourceDevices + Devices []*RequestedDevice } const ( @@ -1771,7 +1771,6 @@ 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 @@ -1790,52 +1789,6 @@ 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. @@ -1854,7 +1807,6 @@ 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() @@ -1903,7 +1855,6 @@ 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) } @@ -1911,7 +1862,6 @@ 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" @@ -1927,7 +1877,6 @@ 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 @@ -1948,7 +1897,6 @@ 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) } @@ -2126,24 +2074,11 @@ type RequestedDevice struct { // Constraints are a set of constraints to apply when selecting the device // to use. - Constraints Constraints + Constraints []*Constraint // Affinities are a set of affinites to apply when selecting the device // to use. - 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) + Affinities []*Affinity } func (r *RequestedDevice) Copy() *RequestedDevice { @@ -2314,9 +2249,15 @@ func (n *NodeResources) Equals(o *NodeResources) bool { if !n.Disk.Equals(&o.Disk) { return false } - if !n.Networks.Equals(&o.Networks) { + + if len(n.Networks) != len(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) { @@ -2326,28 +2267,7 @@ func (n *NodeResources) Equals(o *NodeResources) bool { return true } -// 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 +// DevicesEquals returns true if the two device arrays are equal func DevicesEquals(d1, d2 []*NodeDeviceResource) bool { if len(d1) != len(d2) { return false @@ -6495,11 +6415,10 @@ type Constraint struct { } // Equal checks if two constraints are equal -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) Equal(o *Constraint) bool { + return c.LTarget == o.LTarget && + c.RTarget == o.RTarget && + c.Operand == o.Operand } func (c *Constraint) Copy() *Constraint { @@ -6575,29 +6494,6 @@ 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 @@ -6608,12 +6504,11 @@ type Affinity struct { } // Equal checks if two affinities are equal -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) Equal(o *Affinity) bool { + return a.LTarget == o.LTarget && + a.RTarget == o.RTarget && + a.Operand == o.Operand && + a.Weight == o.Weight } func (a *Affinity) Copy() *Affinity { @@ -6694,29 +6589,6 @@ 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