From f931eb6a0aedceacd13c18a8c420282e34765677 Mon Sep 17 00:00:00 2001 From: Jonathon Anderson Date: Thu, 30 Jan 2025 00:56:16 -0700 Subject: [PATCH] Fixed negation for slice field elements during profile/node merge - Fixes: #1677 Signed-off-by: Jonathon Anderson --- CHANGELOG.md | 1 + internal/app/wwctl/node/list/main_test.go | 6 +- internal/pkg/node/constructors_test.go | 2 +- internal/pkg/node/mergo.go | 1 + internal/pkg/node/mergo_test.go | 208 +++++++++++++++------- internal/pkg/node/methods.go | 9 + 6 files changed, 156 insertions(+), 71 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8558974b..8da3db3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Fix `make install` when `sudo` does not set `$PWD`. #1660 - Use sh to parse and exec IPMI command. #1663 - Use configured warewulf.conf path in `wwctl upgrade`. #1658 +- Fixed negation for slice field elements during profile/node merge. #1677 ## v4.6.0rc1, 2025-01-29 diff --git a/internal/app/wwctl/node/list/main_test.go b/internal/app/wwctl/node/list/main_test.go index 01cff18e..7c3ab9e7 100644 --- a/internal/app/wwctl/node/list/main_test.go +++ b/internal/app/wwctl/node/list/main_test.go @@ -267,7 +267,7 @@ nodes: `, }, { - name: "multiple overlays list long with negation", + name: "multiple overlays list long", args: []string{"-l"}, wantErr: false, stdout: ` @@ -287,13 +287,13 @@ nodes: `, }, { - name: "multiple overlays list long", + name: "multiple overlays list long with negation", args: []string{"-l"}, wantErr: false, stdout: ` NODE NAME KERNEL VERSION IMAGE OVERLAYS (S/R) --------- -------------- ----- -------------- -n01 -- -- sop1/rop1,rop2,nop1,~rop1 +n01 -- -- sop1/rop2,nop1 `, inDb: `nodeprofiles: p1: diff --git a/internal/pkg/node/constructors_test.go b/internal/pkg/node/constructors_test.go index 5bdcd501..fd53d3ea 100644 --- a/internal/pkg/node/constructors_test.go +++ b/internal/pkg/node/constructors_test.go @@ -214,7 +214,7 @@ nodes: assert.Contains(nodemap, "node4") assert.ElementsMatch(nodemap["node4"].RuntimeOverlay, []string{"p1o1", "p1o2", "p2o1", "p2o2", "n1o1", "n1o2"}) assert.Contains(nodemap, "node5") - assert.ElementsMatch(nodemap["node5"].RuntimeOverlay, []string{"p1o1", "p1o2", "~p1o2", "p2o1", "p2o2", "n1o1"}) + assert.ElementsMatch(nodemap["node5"].RuntimeOverlay, []string{"p1o1", "p2o1", "p2o2", "n1o1"}) } func Test_negated_list(t *testing.T) { diff --git a/internal/pkg/node/mergo.go b/internal/pkg/node/mergo.go index 513f9e3b..79e20d11 100644 --- a/internal/pkg/node/mergo.go +++ b/internal/pkg/node/mergo.go @@ -153,6 +153,7 @@ func (config *NodesYaml) MergeNode(id string) (node Node, fields fieldMap, err e node.id = id node.valid = true node.updatePrimaryNetDev() + node.Profile.cleanLists() return node, fields, nil } diff --git a/internal/pkg/node/mergo_test.go b/internal/pkg/node/mergo_test.go index 27029692..6470046e 100644 --- a/internal/pkg/node/mergo_test.go +++ b/internal/pkg/node/mergo_test.go @@ -194,16 +194,18 @@ nodeprofiles: func Test_MergeNode(t *testing.T) { var tests = map[string]struct { - nodesConf string - node string - field string - source string - value string - nodes []string - fields []string - sources []string - values []string - jsonValues []string + nodesConf string + node string + field string + source string + value string + fieldValue string + nodes []string + fields []string + sources []string + values []string + fieldValues []string + jsonValues bool }{ "node comment": { nodesConf: ` @@ -585,6 +587,9 @@ nodeprofiles: }, "node profiles": { nodesConf: ` +nodeprofiles: + p1: {} + p2: {} nodes: n1: profiles: @@ -604,7 +609,8 @@ nodes: nodeprofiles: p1: profiles: - - p2`, + - p2 + p2: {}`, node: "n1", field: "Profiles", source: "", @@ -792,7 +798,92 @@ nodes: nodes: []string{"n1"}, fields: []string{"Resources[fstab]"}, sources: []string{"p1,n1"}, - jsonValues: []string{`[{"file":"/home","spec":"warewulf:/home","vfstype":"nfs"},{"file":"/opt","spec":"warewulf:/opt","vfstype":"nfs"}]`}, + values: []string{`[{"file":"/home","spec":"warewulf:/home","vfstype":"nfs"},{"file":"/opt","spec":"warewulf:/opt","vfstype":"nfs"}]`}, + jsonValues: true, + }, + "node1 gets kernel data from profile": { + nodesConf: ` +nodeprofiles: + default: + kernel: + version: v1.0.0 + args: + - arg1 + - arg2 +nodes: + node1: + profiles: + - default`, + nodes: []string{"node1", "node1"}, + fields: []string{"Kernel.Version", "Kernel.Args"}, + values: []string{"v1.0.0", "arg1,arg2"}, + }, + "node can negate kernel args from profile": { + nodesConf: ` +nodeprofiles: + default: + kernel: + version: v1.0.0 + args: + - arg1 + - arg2 +nodes: + node1: + profiles: + - default + kernel: + args: + - ~arg2 + - arg3`, + nodes: []string{"node1", "node1"}, + fields: []string{"Kernel.Version", "Kernel.Args"}, + sources: []string{"default", "default,node1"}, + values: []string{"v1.0.0", "arg1,arg3"}, + fieldValues: []string{"v1.0.0", "arg1,arg2,~arg2,arg3"}, + }, + "node1 doesn't get interference data from node2": { + nodesConf: ` +nodeprofiles: + default: {} +nodes: + node1: {} + node2: + profiles: + - default + kernel: + version: v1.0.0 + args: + - kernel-args`, + nodes: []string{"node1", "node1", "node2", "node2"}, + fields: []string{"Kernel.Version", "Kernel.Args", "Kernel.Version", "Kernel.Args"}, + values: []string{"UNDEF", "UNDEF", "v1.0.0", "kernel-args"}, + fieldValues: []string{"", "", "v1.0.0", "kernel-args"}, + }, + "overlay negation": { + nodesConf: ` +nodeprofiles: + default: + runtime overlay: + - ro1 + - ro2 + system overlay: + - so1 + - so2 +nodes: + n1: + profiles: + - default + runtime overlay: + - ~ro1 + - ro3 + system overlay: + - so4 + - ~so2`, + nodes: []string{"n1", "n1"}, + fields: []string{"RuntimeOverlay", "SystemOverlay"}, + values: []string{"ro2,ro3", "so1,so4"}, + fieldValues: []string{"ro1,ro2,~ro1,ro3", "so1,so2,so4,~so2"}, + sources: []string{"default,n1", "default,n1"}, }, } @@ -811,31 +902,53 @@ nodes: value, valueErr := getNestedFieldString(node, tt.field) assert.NoError(t, valueErr) - assert.Equal(t, tt.value, value) - assert.Equal(t, tt.value, fields.Value(tt.field)) + + ttFieldValue := tt.value + if tt.fieldValue != "" { + ttFieldValue = tt.fieldValue + } + + if tt.jsonValues { + assert.JSONEq(t, tt.value, value) + assert.JSONEq(t, ttFieldValue, fields.Value(tt.field)) + } else { + assert.Equal(t, tt.value, value) + assert.Equal(t, ttFieldValue, fields.Value(tt.field)) + } assert.Equal(t, tt.source, fields.Source(tt.field)) } - var nodes []Node for i := range tt.nodes { - node, _, mergeErr := registry.MergeNode(tt.nodes[i]) + node, fields, mergeErr := registry.MergeNode(tt.nodes[i]) assert.NoError(t, mergeErr) - nodes = append(nodes, node) - } - for i := range tt.nodes { - _, fields, _ := registry.MergeNode(tt.nodes[i]) - value, valueErr := getNestedFieldString(nodes[i], tt.fields[i]) - assert.NoError(t, valueErr) + value, valueErr := getNestedFieldString(node, tt.fields[i]) + if valueErr != nil { + value = "UNDEF" + } + if len(tt.values) > i { - assert.Equal(t, tt.values[i], value) - assert.Equal(t, tt.values[i], fields.Value(tt.fields[i])) + if tt.jsonValues { + assert.JSONEq(t, tt.values[i], value) + } else { + assert.Equal(t, tt.values[i], value) + } } - if len(tt.jsonValues) > i { - assert.JSONEq(t, tt.jsonValues[i], value) - assert.Equal(t, tt.jsonValues[i], fields.Value(tt.fields[i])) + + if len(tt.fields) > i { + ttFieldValues := tt.values + if len(tt.fieldValues) > 0 { + ttFieldValues = tt.fieldValues + } + if tt.jsonValues { + assert.JSONEq(t, ttFieldValues[i], fields.Value(tt.fields[i])) + } else { + assert.Equal(t, ttFieldValues[i], fields.Value(tt.fields[i])) + } + } + if len(tt.sources) > i { + assert.Equal(t, tt.sources[i], fields.Source(tt.fields[i])) } - assert.Equal(t, tt.sources[i], fields.Source(tt.fields[i])) } }) } @@ -963,45 +1076,6 @@ nodes: } } -func Test_MergeNodeKernel(t *testing.T) { - var tests = map[string]struct { - nodesConf string - node string - kernel *KernelConf - }{ - "interference": { - nodesConf: ` -nodeprofiles: - default: {} -nodes: - node1: {} - test: - profiles: - - default - kernel: - version: v1.0.0 - args: - - kernel-args`, - node: "node1", - kernel: nil, - }, - } - - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - env := testenv.New(t) - defer env.RemoveAll() - env.WriteFile("/etc/warewulf/nodes.conf", tt.nodesConf) - - registry, regErr := New() - assert.NoError(t, regErr) - node, _, mergeErr := registry.MergeNode(tt.node) - assert.NoError(t, mergeErr) - assert.Equal(t, tt.kernel, node.Kernel) - }) - } -} - func Test_MergeNodeIpmi(t *testing.T) { var tests = map[string]struct { nodesConf string diff --git a/internal/pkg/node/methods.go b/internal/pkg/node/methods.go index c2f1606f..a7fc5064 100644 --- a/internal/pkg/node/methods.go +++ b/internal/pkg/node/methods.go @@ -382,6 +382,15 @@ func negList(list []string) (ret []string) { return } +func (p *Profile) cleanLists() { + p.Profiles = cleanList(p.Profiles) + p.SystemOverlay = cleanList(p.SystemOverlay) + p.RuntimeOverlay = cleanList(p.RuntimeOverlay) + if p.Kernel != nil { + p.Kernel.Args = cleanList(p.Kernel.Args) + } +} + // clean a list from negated tokens func cleanList(list []string) (ret []string) { neg := negList(list)