Skip to content

Refactor BMC ID Mapping and add deterministic generated XNAMEs#114

Merged
davidallendj merged 1 commit into
OpenCHAMI:mainfrom
erl-hpe:add-xname-mapping
Aug 6, 2025
Merged

Refactor BMC ID Mapping and add deterministic generated XNAMEs#114
davidallendj merged 1 commit into
OpenCHAMI:mainfrom
erl-hpe:add-xname-mapping

Conversation

@erl-hpe

@erl-hpe erl-hpe commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

Description

This PR corrects a regression that was caused by the previous BMC ID Mapping implementation which removed generated XNAMEs from 'collect' altogether. It creates a new infrastructure for generating BMC IDs using (currently) either a user provided ID Map or an algorithmically generated XNAME as the id. This infrastructure is extensible by adding new types of mappers that can generate new kinds of BMC IDs in the future, and by expanding the available BMC ID selector key set from only the IPv4 address of the BMC to whatever additional items become available in the future. Each mapper implementation can select its preferred BMC ID selector key from the available keys based on its own internal criteria.

Related Issues

Resolves #113

Testing

This PR was tested on OpenCHAMI on vTDS by running 'collect' against the provided RedFish emulator using both user supplied BMD ID Mapping and generated XNAME id mapping and observing the results in both cases. In both cases valid XNAMEs were produced as BMC IDs. In the case of the user provided map, the XNAMEs matched the mapped values.

This PR was also tested on OpenCHAMI on vTDS using a combination of RIE simulated RedFish servers and sushy-emulator RedFish serves, referencing Virtual Compute Nodes. In this testing, it was integrated with SMD (i.e. it sent its results to SMD for handling). It was tested both with user provided BMC ID mapping and with generated XNAMES. In both cases, the data showed up in SMD correctly.

@erl-hpe erl-hpe changed the title Refactor BMC ID Mapping and add deterministic generated XNAMEs Refactor BMC ID Mapping and add deterministic generated XNAMEs Resolves #113 Aug 6, 2025
@erl-hpe erl-hpe changed the title Refactor BMC ID Mapping and add deterministic generated XNAMEs Resolves #113 Refactor BMC ID Mapping and add deterministic generated XNAMEs Aug 6, 2025
@synackd

synackd commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

Thanks for this. Can you provide example usage with and without the mapping flag so it can be tested outside of vTDS?

Comment thread pkg/idmap_generated_xname.go Outdated
Comment thread pkg/idmap_generated_xname.go Outdated

@davidallendj davidallendj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of things regarding structuring.

Does this change how you use the command or flags from the previous PR?

Comment thread cmd/collect.go
@@ -129,7 +129,7 @@ var CollectCmd = &cobra.Command{
ForceUpdate: forceUpdate,
AccessToken: accessToken,
SecretStore: store,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REVIEWERS: I went through and made the use of "ID" in symbols consistently same case: either "id" or "ID" for all of my symbols in hopes of making the names easier to predict and use. Hence this one change here.

Comment thread pkg/collect.go
// no matches found, so return an empty string
return "", fmt.Errorf("no ethernet interfaces found with IP address")
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REVIEWERS: I moved this code to the User Porvided ID Mapper implementation.

Comment thread pkg/idmap.go
@erl-hpe
erl-hpe force-pushed the add-xname-mapping branch from 4b6e8ff to b0e2a4b Compare August 6, 2025 17:20
Signed-off-by: Eric Lund <77127214+erl-hpe@users.noreply.github.com>
@erl-hpe
erl-hpe force-pushed the add-xname-mapping branch from b0e2a4b to 3ffb3ba Compare August 6, 2025 17:32
@erl-hpe

erl-hpe commented Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

Couple of things regarding structuring.

Does this change how you use the command or flags from the previous PR?

No, the command line arguments remain the same and are used the same way. If no mapping is provided on the command line, I fall back to whatever other schemes are available. Since there is only one other scheme at the moment, the generated xname scheme is used. There is no longer a scheme that just generates IP addresses, since that was only needed when we had no reasonable fall-back from the mapping. I updated the README (basically taking out reference to that fallback) to reflect this.

@davidallendj

davidallendj commented Aug 6, 2025

Copy link
Copy Markdown
Collaborator

No, the command line arguments remain the same and are used the same way. If no mapping is provided on the command line, I fall back to whatever other schemes are available. Since there is only one other scheme at the moment, the generated xname scheme is used. There is no longer a scheme that just generates IP addresses, since that was only needed when we had no reasonable fall-back from the mapping. I updated the README (basically taking out reference to that fallback) to reflect this.

In that case, @synackd you should be able to test using this for generated xnames:

magellan collect -v -o nodes.yaml

And with the mappings.yaml file something like this:

map_key: bmc-ip-addr
id_map:
    172.21.0.1: x0c0s1b0
    172.21.0.2: x0c0s2b0
    ...
magellan collect --bmc-id-map @mappings.yaml -o nodes.yaml

@erl-hpe Is that correct?

Comment thread pkg/idmap.go
@erl-hpe

erl-hpe commented Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

@erl-hpe Is that correct?

Yes, in fact, those are both tests I ran.

@erl-hpe

erl-hpe commented Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

@synackd sorry I missed this. See the above comments. The syntax has not changed since the last PR. The behavior has changed only insofar as the fallback IDs are now XNAMEs again instead of IP addresses.

One note on the mapping file in the example David provided, leave out the elipsis at the end, that is not part of the file.

Thanks for this. Can you provide example usage with and without the mapping flag so it can be tested outside of vTDS?

@davidallendj
davidallendj self-requested a review August 6, 2025 18:24

@davidallendj davidallendj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I tried testing this with and without the mappings file and both seemed to work. Not including a mappings file will create a xname like before but with more meaningful logic.

@davidallendj
davidallendj merged commit 9da26e3 into OpenCHAMI:main Aug 6, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE]Re-Introduce BMC XNAME Generation as a Sub-Case of BMC ID Mapping in collect

3 participants