Skip to content

fix: remove appended index to component ID in parseRedfishEndpointV2 #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
davidallendj wants to merge 2 commits into
base: main
Choose a base branch
from
+1 −1
Open

fix: remove appended index to component ID in parseRedfishEndpointV2 #99

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffae75c
refactor: removed appended index to component ID in parseRedfishEndpo…
davidallendj Jul 14, 2026
238d05f
fix: added missing comma in smd-api.go
davidallendj Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/smd/smd-api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3073,7 +3073,7 @@ func (s *SmD) parseRedfishEndpointDataV2(w http.ResponseWriter, data []byte, for

enabled := true
component := base.Component{
ID: root.ID + fmt.Sprintf("n%d", ceNum),
ID: root.ID,

@travisbcotton travisbcotton Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't this just gonna create the component with the BMC xname? There won't be a nX at the end?

@davidallendj davidallendj Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's what it looks like.

@synackd synackd Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I presume that we will want to set the xname either via Magellan or ochami static discovery, right? is there a use case where we need to let SMD set it dynamically?

@travisbcotton travisbcotton Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The way SMD is configured you create the BMC component (x1000c1s1b1) and then the attached node(s) are x1000c1s1b1n[0-n]. The problem here is that if I add x1000c1s1b1n1 before I add x1000c1s1b1n0 then they get swapped in the RedfishEndpoints. If we're gonna add via discovery, static or magellan, then the payload is gonna have to specify the xname of the BMC and all attached nodes

@davidallendj davidallendj Aug 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think there's a way to do that already in magellan using the BMC ID mapping feature that was added a while ago. I assumed there's a way to do it with ochami as well.

Edit: Actually I don't think there's a way to specify the nodes at all since there's no way to do that in the parsing code itself. I think we could add a code path to look for all the node names managed by a BMC or something like that. I'd have to look at the code again for the specifics though.

@synackd synackd Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The way SMD is configured you create the BMC component (x1000c1s1b1) and then the attached node(s) are x1000c1s1b1n[0-n]. The problem here is that if I add x1000c1s1b1n1 before I add x1000c1s1b1n0 then they get swapped in the RedfishEndpoints. If we're gonna add via discovery, static or magellan, then the payload is gonna have to specify the xname of the BMC and all attached nodes

I'm also pretty sure that ochami does this since xnames are either explicitly defined or inferred from the discovery YAML.

@synackd synackd Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

BMC xname is mandatory in ochami and nodes are assigned BMCs either explicitly or implicitly.

@davidallendj davidallendj Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@travisbcotton Can you try using this PR and see if it fixes your issue? If we don't need to change anything, we can go ahead and get this merged.

@travisbcotton travisbcotton Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean this is creating a Node type component is it not? So it can't be the BMC xname

@synackd synackd Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah it's under the Systems loop, which createa the nodes.

Right now, there is no way to get the desired component xname from the RedfishEndpoint requests, since InventoryDetail (the structure each System uses) does not have a field for the desired xname, which is likely why this loop was put here in the first place. We should rethink if we should expose it as a field so tools like ochami and Magellan can set it on request and/or let SMD dynamically it if not provided.

NID: nid,
State: "On",
Type: xnametypes.Node.String(),
Expand Down
Loading