From c1b914bc58b220ee49d2f4aaf321e02aa944f913 Mon Sep 17 00:00:00 2001 From: Nick Ethier Date: Tue, 31 Mar 2020 15:06:22 -0400 Subject: [PATCH] cni: fix assertion ordering in tests with multiple node networks ignore comparing networking for now in test Apply suggestions from code review Co-authored-by: Seth Hoenig --- client/allocrunner/networking_cni.go | 8 ++++---- client/client.go | 4 +++- client/client_test.go | 10 +++++++--- client/fingerprint/bridge_linux.go | 7 +++++-- client/fingerprint/cni.go | 8 +++++--- client/fingerprint/cni_test.go | 16 +++++++++++----- .../fingerprint/test_fixtures/cni/net2.conflist | 3 +-- nomad/structs/structs.go | 4 ++-- 8 files changed, 38 insertions(+), 22 deletions(-) diff --git a/client/allocrunner/networking_cni.go b/client/allocrunner/networking_cni.go index 27f682c804a8..15cd2745fd49 100644 --- a/client/allocrunner/networking_cni.go +++ b/client/allocrunner/networking_cni.go @@ -43,7 +43,7 @@ type cniNetworkConfigurator struct { func newCNINetworkConfigurator(logger log.Logger, cniPath, cniInterfacePrefix, cniConfDir, networkName string) (*cniNetworkConfigurator, error) { cniConf, err := loadCNIConf(cniConfDir, networkName) if err != nil { - return nil, fmt.Errorf("failed to load cni config: %v", err) + return nil, fmt.Errorf("failed to load CNI config: %v", err) } return newCNINetworkConfiguratorWithConf(logger, cniPath, cniInterfacePrefix, cniConf) @@ -83,7 +83,7 @@ func (c *cniNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Alloc // Depending on the version of bridge cni plugin used, a known race could occure // where two alloc attempt to create the nomad bridge at the same time, resulting - // in one of them to fail. This rety attempts to overcome any + // in one of them to fail. This rety attempts to overcome those erroneous failures. const retry = 3 var firstError error for attempt := 1; ; attempt++ { @@ -112,9 +112,9 @@ func loadCNIConf(confDir, name string) ([]byte, error) { files, err := cnilibrary.ConfFiles(confDir, []string{".conf", ".conflist", ".json"}) switch { case err != nil: - return nil, fmt.Errorf("failed to read cni config file: %v", err) + return nil, fmt.Errorf("failed to detect CNI config file: %v", err) case len(files) == 0: - return nil, fmt.Errorf("no cni network config found in %s", confDir) + return nil, fmt.Errorf("no CNI network config found in %s", confDir) } // files contains the network config files associated with cni network. diff --git a/client/client.go b/client/client.go index a45f4f4f197a..b96169c83432 100644 --- a/client/client.go +++ b/client/client.go @@ -1459,6 +1459,8 @@ func (c *Client) updateNodeFromFingerprint(response *fingerprint.FingerprintResp return c.configCopy.Node } +// updateNetworks filters and overrides network speed of host networks based +// on configured settings func updateNetworks(up structs.Networks, c *config.Config) structs.Networks { if up == nil { return nil @@ -1482,7 +1484,7 @@ func updateNetworks(up structs.Networks, c *config.Config) structs.Networks { up = upd } - // ns is set, apply the config NetworkSpeed to all + // if set, apply the config NetworkSpeed to networks in host mode if c.NetworkSpeed != 0 { for _, n := range up { if n.Mode == "host" { diff --git a/client/client_test.go b/client/client_test.go index a9793ebaf04e..1afb34422fc6 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -1263,7 +1263,7 @@ func Test_UpdateNodeFromFingerprintMultiIP(t *testing.T) { // Client without network configured updates to match fingerprint client, cleanup := TestClient(t, func(c *config.Config) { c.NetworkInterface = dev - c.Node.NodeResources.Networks[0].Device = dev + c.Options["fingerprint.blacklist"] = "network,cni,bridge" c.Node.Resources.Networks = c.Node.NodeResources.Networks }) defer cleanup() @@ -1278,12 +1278,13 @@ func Test_UpdateNodeFromFingerprintMultiIP(t *testing.T) { }, }) - two := structs.Networks{ + nets := structs.Networks{ + mock.Node().NodeResources.Networks[0], {Device: dev, IP: "127.0.0.1"}, {Device: dev, IP: "::1"}, } - require.Equal(t, two, client.config.Node.NodeResources.Networks) + require.Equal(t, nets, client.config.Node.NodeResources.Networks) } func TestClient_computeAllocatedDeviceStats(t *testing.T) { @@ -1474,6 +1475,9 @@ func TestClient_getAllocatedResources(t *testing.T) { result := client.getAllocatedResources(client.config.Node) + // Ignore comparing networks for now + result.Flattened.Networks = nil + expected := structs.ComparableResources{ Flattened: structs.AllocatedTaskResources{ Cpu: structs.AllocatedCpuResources{ diff --git a/client/fingerprint/bridge_linux.go b/client/fingerprint/bridge_linux.go index 336fda4ea329..231d669ecfe9 100644 --- a/client/fingerprint/bridge_linux.go +++ b/client/fingerprint/bridge_linux.go @@ -4,7 +4,7 @@ import ( "bufio" "fmt" "os" - "strings" + "regexp" "github.com/hashicorp/nomad/nomad/structs" ) @@ -36,9 +36,12 @@ func (f *BridgeFingerprint) checkKMod(mod string) error { defer file.Close() scanner := bufio.NewScanner(file) + pattern := fmt.Sprintf("%s\\s+.*$", mod) for scanner.Scan() { - if strings.HasPrefix(scanner.Text(), mod+" ") { + if matched, err := regexp.MatchString(pattern, scanner.Text()); matched { return nil + } else if err != nil { + return fmt.Errorf("could not parse /proc/modules: %v", err) } } diff --git a/client/fingerprint/cni.go b/client/fingerprint/cni.go index 682c029d4bda..9080733db592 100644 --- a/client/fingerprint/cni.go +++ b/client/fingerprint/cni.go @@ -30,7 +30,7 @@ func (f *CNIFingerprint) Fingerprint(req *FingerprintRequest, resp *FingerprintR files, err := libcni.ConfFiles(confDir, []string{".conf", ".conflist", ".json"}) if err != nil { - return fmt.Errorf("failed to read cni conf files: %v", err) + return fmt.Errorf("failed to detect CNI conf files: %v", err) } for _, confFile := range files { @@ -40,7 +40,7 @@ func (f *CNIFingerprint) Fingerprint(req *FingerprintRequest, resp *FingerprintR return fmt.Errorf("failed to load CNI config list file %s: %v", confFile, err) } if _, ok := networks[confList.Name]; ok { - f.logger.Warn("multiple CNI config names found, ignoring file", "name", confList.Name, "file", confFile) + f.logger.Warn("duplicate CNI config names found, ignoring file", "name", confList.Name, "file", confFile) continue } networks[confList.Name] = struct{}{} @@ -50,7 +50,7 @@ func (f *CNIFingerprint) Fingerprint(req *FingerprintRequest, resp *FingerprintR return fmt.Errorf("failed to load CNI config file %s: %v", confFile, err) } if _, ok := networks[conf.Network.Name]; ok { - f.logger.Warn("multiple CNI config names found, ignoring file", "name", conf.Network.Name, "file", confFile) + f.logger.Warn("duplicate CNI config names found, ignoring file", "name", conf.Network.Name, "file", confFile) continue } networks[conf.Network.Name] = struct{}{} @@ -73,3 +73,5 @@ func (f *CNIFingerprint) Fingerprint(req *FingerprintRequest, resp *FingerprintR resp.Detected = true return nil } + +func (f *CNIFingerprint) Reload() {} diff --git a/client/fingerprint/cni_test.go b/client/fingerprint/cni_test.go index d89a930b27a2..3fd125b7d4f6 100644 --- a/client/fingerprint/cni_test.go +++ b/client/fingerprint/cni_test.go @@ -9,6 +9,9 @@ import ( "github.com/stretchr/testify/require" ) +// Test that CNI fingerprinter is reloadable +var _ ReloadableFingerprint = &CNIFingerprint{} + func TestCNIFingerprint(t *testing.T) { cases := []struct { name string @@ -63,16 +66,19 @@ func TestCNIFingerprint(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - require := require.New(t) + r := require.New(t) fp := NewCNIFingerprint(testlog.HCLogger(t)) resp := &FingerprintResponse{} err := fp.Fingerprint(c.req, resp) if c.err { - require.Error(err) - require.Contains(err.Error(), c.errMatch) + r.Error(err) + r.Contains(err.Error(), c.errMatch) } else { - require.NoError(err) - require.Exactly(c.exp, resp) + r.NoError(err) + r.Equal(c.exp.Detected, resp.Detected) + if resp.NodeResources != nil || c.exp.NodeResources != nil { + r.ElementsMatch(c.exp.NodeResources.Networks, resp.NodeResources.Networks) + } } }) } diff --git a/client/fingerprint/test_fixtures/cni/net2.conflist b/client/fingerprint/test_fixtures/cni/net2.conflist index 8d14cfcfb968..cec6c67efa95 100644 --- a/client/fingerprint/test_fixtures/cni/net2.conflist +++ b/client/fingerprint/test_fixtures/cni/net2.conflist @@ -19,8 +19,7 @@ "type": "portmap", "capabilities": { "portMappings": true - }, - "externalSetMarkChain": "KUBE-MARK-MASQ" + } } ] } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 59463af462f7..ea14890dde77 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2239,11 +2239,11 @@ func (nr *NetworkResource) Hash() uint32 { data = append(data, []byte(fmt.Sprintf("%s%s%s%s%d", nr.Mode, nr.Device, nr.CIDR, nr.IP, nr.MBits))...) for i, port := range nr.ReservedPorts { - data = append(data, []byte(fmt.Sprintf("%d%s%d%d", i, port.Label, port.Value, port.To))...) + data = append(data, []byte(fmt.Sprintf("r%d%s%d%d", i, port.Label, port.Value, port.To))...) } for i, port := range nr.DynamicPorts { - data = append(data, []byte(fmt.Sprintf("%d%s%d%d", i, port.Label, port.Value, port.To))...) + data = append(data, []byte(fmt.Sprintf("d%d%s%d%d", i, port.Label, port.Value, port.To))...) } return crc32.ChecksumIEEE(data)