From e219c35cbf6dd5951fac818f2868a431d7f43cd1 Mon Sep 17 00:00:00 2001 From: Devon Bautista <17506592+synackd@users.noreply.github.com> Date: Thu, 28 Aug 2025 16:39:14 -0600 Subject: [PATCH 1/2] feat(discover): add 'groups' and deprecate 'group' Add a 'groups' directive in the static discovery specification that allows nodes to be members of multiple groups. These groups are deduplicated and created in SMD during discovery. Deprecate the 'group' directive in the static discovery specification. As of this commit, 'groups' will still work, but a deprecation warning is printed if it is used in a node definition. If both 'group' and 'groups' are specified, the 'group' entry is merged with the 'groups' list as if it were another entry in the list. The 'group' directive will be removed in a future release. Signed-off-by: Devon Bautista <17506592+synackd@users.noreply.github.com> --- cmd/discover-static.go | 36 +++++++++++++++++++++---- man/ochami-discover.1.sc | 9 ++++--- pkg/discover/discover.go | 27 ++++++++++++++----- pkg/discover/discover_test.go | 50 +++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 15 deletions(-) diff --git a/cmd/discover-static.go b/cmd/discover-static.go index 9f847a1e..84c9e183 100644 --- a/cmd/discover-static.go +++ b/cmd/discover-static.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "strings" "github.com/spf13/cobra" @@ -35,7 +36,9 @@ nodes: xname: x1000c1s7b0n0 bmc_mac: de:ca:fc:0f:ee:ee bmc_ip: 172.16.0.101 - group: compute + groups: + - compute + - slurm interfaces: - mac_addr: de:ad:be:ee:ee:f1 ip_addrs: @@ -327,17 +330,40 @@ See ochami-discover(1) for more details.`, // Put together list of groups to add and which components to add to those groups groupsToAdd := make(map[string]smd.Group) for _, node := range nodes.Nodes { + // node.Group IS DEPRECATED IN FAVOR OF node.groups. This block should be + // deleted when node.Group is removed. + // + // For now, we merge node.Group with node.Groups. Since we use a dictionary for + // deduplication, this is trivial. if node.Group != "" { + if len(strings.Trim(node.Name, " \t")) == 0 { + log.Logger.Warn().Msgf("node %s contains 'group', which is deprecated; use 'groups' instead", node.Xname) + } else { + log.Logger.Warn().Msgf("node %s (%s) contains 'group', which is deprecated; use 'groups' instead", node.Xname, node.Name) + } if g, ok := groupsToAdd[node.Group]; !ok { + // Group doesn't exist yet, populate groupsToAdd with it newGroup := smd.Group{ Label: node.Group, Description: fmt.Sprintf("The %s group", node.Group), } - newGroup.Members.IDs = []string{node.Xname} - groupsToAdd[node.Group] = newGroup + groupsToAdd[node.Group] = discover.AddMemberToGroup(newGroup, node.Xname) + } else { + // Update group membership with new node in groupsToAdd map + groupsToAdd[node.Group] = discover.AddMemberToGroup(g, node.Xname) + } + } + for _, group := range node.Groups { + if g, ok := groupsToAdd[group]; !ok { + // Group doesn't exist yet, populate groupsToAdd with it + newGroup := smd.Group{ + Label: group, + Description: fmt.Sprintf("The %s group", group), + } + groupsToAdd[group] = discover.AddMemberToGroup(newGroup, node.Xname) } else { - g.Members.IDs = append(g.Members.IDs, node.Xname) - groupsToAdd[node.Group] = g + // Update group membership with new node in groupsToAdd map + groupsToAdd[group] = discover.AddMemberToGroup(g, node.Xname) } } } diff --git a/man/ochami-discover.1.sc b/man/ochami-discover.1.sc index 4bf278b3..196aceb2 100644 --- a/man/ochami-discover.1.sc +++ b/man/ochami-discover.1.sc @@ -26,7 +26,8 @@ nodes: xname: x1000c1s7b0n0 bmc_mac: de:ca:fc:0f:ee:ee bmc_ip: 172.16.0.101 - group: compute + groups: + - compute interfaces: - mac_addr: de:ad:be:ee:ee:f1 ip_addrs: @@ -56,8 +57,10 @@ is used as the unique identifier for the node within the Component that gets created for node. - *bmc_mac* - MAC address of node's BMC. - *bmc_ip* - Desired IP address of node's BMC. -- *group* - Optional group to add node to. This will get created during -discovery if it does not exist. +- *group* - *DEPRECATED.* Use *groups* instead. *group* will be removed in a +future release. +- *groups* - Optional list of groups to add node to. These will get created +during discovery if they do not exist. - *interfaces* - A list of network interfaces for the node. - *mac_addr* - MAC address of network interface. - *ip_addrs* - List of IP addresses assigned to interface. diff --git a/pkg/discover/discover.go b/pkg/discover/discover.go index 4528eecc..c9b91ec0 100644 --- a/pkg/discover/discover.go +++ b/pkg/discover/discover.go @@ -35,13 +35,14 @@ func (nl NodeList) String() string { // Node represents a node entry in a payload file. Multiple of these are send to // SMD to "discover" them. type Node struct { - Name string `json:"name" yaml:"name"` - NID int64 `json:"nid" yaml:"nid"` - Xname string `json:"xname" yaml:"xname"` - Group string `json:"group" yaml:"group"` - BMCMac string `json:"bmc_mac" yaml:"bmc_mac"` - BMCIP string `json:"bmc_ip" yaml:"bmc_ip"` - Ifaces []Iface `json:"interfaces" yaml:"interfaces"` + Name string `json:"name" yaml:"name"` + NID int64 `json:"nid" yaml:"nid"` + Xname string `json:"xname" yaml:"xname"` + Group string `json:"group" yaml:"group"` // DEPRECATED + Groups []string `json:"groups" yaml:"groups"` + BMCMac string `json:"bmc_mac" yaml:"bmc_mac"` + BMCIP string `json:"bmc_ip" yaml:"bmc_ip"` + Ifaces []Iface `json:"interfaces" yaml:"interfaces"` } func (n Node) String() string { @@ -241,3 +242,15 @@ func DiscoveryInfoV2(baseURI string, nl NodeList) (smd.ComponentSlice, smd.Redfi } return comps, rfes, ifaces, nil } + +// AddMemberToGroup adds xname to group, ensuring deduplication. +func AddMemberToGroup(group smd.Group, xname string) smd.Group { + for _, x := range group.Members.IDs { + if x == xname { + return group + } + } + g := group + g.Members.IDs = append(g.Members.IDs, xname) + return g +} diff --git a/pkg/discover/discover_test.go b/pkg/discover/discover_test.go index f97cebc1..8e1b4f60 100644 --- a/pkg/discover/discover_test.go +++ b/pkg/discover/discover_test.go @@ -7,6 +7,8 @@ import ( "github.com/google/uuid" "github.com/openchami/schemas/schemas" + + "github.com/OpenCHAMI/ochami/pkg/client/smd" ) func TestNodeList_String(t *testing.T) { @@ -221,3 +223,51 @@ func TestDiscoveryInfoV2_Success(t *testing.T) { } } } + +func TestAddMemberToGroup(t *testing.T) { + newGroup := func(members []string) smd.Group { + var g smd.Group + g.Members.IDs = members + return g + } + tests := []struct { + name string + group smd.Group + xname string + expected smd.Group + }{ + { + name: "add new member to empty group", + group: newGroup([]string{}), + xname: "x1000c0s0b0n0", + expected: newGroup([]string{"x1000c0s0b0n0"}), + }, + { + name: "add new member to non-empty group", + group: newGroup([]string{"x1000c0s0b0n0", "x1000c0s0b1n0"}), + xname: "x1000c0s0b2n0", + expected: newGroup([]string{"x1000c0s0b0n0", "x1000c0s0b1n0", "x1000c0s0b2n0"}), + }, + { + name: "member already exists in group", + group: newGroup([]string{"x1000c0s0b0n0", "x1000c0s0b1n0"}), + xname: "x1000c0s0b1n0", + expected: newGroup([]string{"x1000c0s0b0n0", "x1000c0s0b1n0"}), + }, + { + name: "add member when group has one element", + group: newGroup([]string{"x1000c0s0b0n0"}), + xname: "x1000c0s0b1n0", + expected: newGroup([]string{"x1000c0s0b0n0", "x1000c0s0b1n0"}), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := AddMemberToGroup(tt.group, tt.xname) + if !reflect.DeepEqual(got, tt.expected) { + t.Errorf("AddMemberToGroup() = %+v, want %+v", got, tt.expected) + } + }) + } +} From ada0f4142abb67bf269c261fd2431fa161486035 Mon Sep 17 00:00:00 2001 From: Devon Bautista <17506592+synackd@users.noreply.github.com> Date: Fri, 29 Aug 2025 15:45:34 -0600 Subject: [PATCH 2/2] refactor(discover): move dedup maps outside node loop Deduplication maps for BMC managers and Systems were declared within the loop that iterates over each node, which is where duplication checks happen. This is obviously futile, so the declarations are moved outside of the loop so that the checks occur properly. Most testing has used a single unique BMC per node, which is probably why this was not caught earlier. Signed-off-by: Devon Bautista <17506592+synackd@users.noreply.github.com> --- pkg/discover/discover.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/discover/discover.go b/pkg/discover/discover.go index c9b91ec0..4b72fa80 100644 --- a/pkg/discover/discover.go +++ b/pkg/discover/discover.go @@ -113,8 +113,11 @@ func DiscoveryInfoV2(baseURI string, nl NodeList) (smd.ComponentSlice, smd.Redfi return comps, rfes, ifaces, fmt.Errorf("invalid URI: %s", baseURI) } - // Deduplication map for Components - compMap := make(map[string]string) + var ( + compMap = make(map[string]string) // Deduplication map for SMD Components + systemMap = make(map[string]string) // Deduplication map for BMC Systems + managerMap = make(map[string]string) // Deduplication map for BMC Managers + ) for _, node := range nl.Nodes { log.Logger.Debug().Msgf("generating component structure for node with xname %s", node.Xname) if _, ok := compMap[node.Xname]; !ok { @@ -150,10 +153,6 @@ func DiscoveryInfoV2(baseURI string, nl NodeList) (smd.ComponentSlice, smd.Redfi rfe.IPAddress = node.BMCIP rfe.SchemaVersion = 1 // Tells SMD to use new (v2) parsing code - // Deduplication maps for fake BMC Managers and Systems - systemMap := make(map[string]string) - managerMap := make(map[string]string) - // Create fake BMC "System" for node if it doesn't already exist if _, ok := systemMap[node.Xname]; !ok { log.Logger.Debug().Msgf("node %s: generating fake BMC System", node.Xname)