redfishpower: support plug substitution #159

chu11 · 2024-03-02T05:51:17Z

Problem: In some chassis, power status/control URIs are all
minorly different from each other when doing power status/control
to blades. This makes it difficult to configure powerman/redfishpower
for bladed environments.

Solution: Support plug substitution in URIs with the pattern %p.
By using plug names that are the minor variants of each URI, paths
can be easily configured for each blade in a chassis. This now allows
the setplugs command to map mutiple plug names to a single index.

Fixes #129

Built on top of #158

chu11 · 2024-03-19T22:58:56Z

re-pushed, rebased now that #158 is in

garlick · 2024-03-21T20:27:28Z

Quick comment: why %p when powerman uses %s as a plug variable? (if there's not a good reason maybe best to make them the same to avoid confusion?)

chu11 · 2024-03-21T20:30:53Z

Quick comment: why %p when powerman uses %s as a plug variable? (if there's not a good reason maybe best to make them the same to avoid confusion?)

It was just to differentiate them in the device file. %s is when powerman "fills it in" (i.e. plugs to power control), but %p is when redfishpower "fills it in" with the plug name.

Also recall that %s could be filled in with a range, but %p will only be filled in with a single plugname.

garlick · 2024-03-21T22:25:23Z

I was wondering about the double percent symbols and found this in powerman.dev(5):

Warning: all the send strings are processed with printf and you can cause powermand to segfault if you include any printf tokens other than the appropriate zero or one %s.

OMG that is terrible.

Uhh, "get it right or segfault" seems a little fragile. What do you think about using mustache templates e.g. {{plug}}? There's code to steal in flux-core...

chu11 · 2024-03-21T22:34:23Z

Uhh, "get it right or segfault" seems a little fragile. What do you think about using mustache templates e.g. {{plug}}? There's code to steal in flux-core...

That's a good idea.

There's code to steal in flux-core...

I was a bit lazy in my implementation, just did a search for %p. We can just search for {{plug}} instead.

chu11 · 2024-03-21T23:03:59Z

re-pushed going with {{plug}} instead of %p.

garlick

Some suggestions inline but seems good!

garlick · 2024-03-25T19:03:51Z

src/redfishpower/plugs.h

+
+ /* if multiple plugs mapped to 1 host, plug substitution via %p
+ * required */


Al omit linkage verbs because Al caveman? 😛

garlick · 2024-03-25T19:29:32Z

src/redfishpower/redfishpower.c

+ /* assumption, at most one {{plug}} in path */
+ if ((ptr = strstr(lpath, "{{plug}}"))) {
+ /* +1 for NUL, -8 for {{plug}}, + strlen(plugname) */
+ char *tmp = xmalloc(strlen(lpath) + 1 - 8 + strlen(plugname));
+ int prefixlen = ptr - lpath;
+ strncpy(tmp, lpath, prefixlen);
+ strcpy(tmp + prefixlen, plugname);
+ strcpy(tmp + prefixlen + strlen(plugname), ptr + 2);
+ (*path) = tmp;


Suggestion: break out to its own function.

garlick · 2024-03-25T19:33:15Z

src/redfishpower/plugs.c

+void plugs_set_plug_sub_required_flag(plugs_t *p,
+ const char *plugname,
+ int plug_sub_required)
+{
+ struct plug_data *pd = plugs_get_data(p, plugname);
+ if (pd)
+ pd->plug_sub_required = plug_sub_required;
+}
+


What's the reason for this flag? It's not clear from the description.

Seems like {{plug}} should be subtituted if there, and not if not, and that's that?

I'll add some comments to clarify this more. But basically before this PR number of plugs == number of indices of hosts. i.e.

setplugs Plugs[0-7] [0-7]

but now that we can do plug substitution, multiple can now point to just 1

setplugs Plugs[0-7] 0

but if you have > 1 pointing to 1, then plug substitution is required. So it's just a flag to say "hey, this plug better be setup with a path with plug substitution".

The reason the flag is necessary is because we setup paths separately, so there's no way to check for this condition during a setplugs call.

Gotcha, although it might be more straightforward to simply check the whole config at the end, ensuring that every plug maps to a unique (host,path) tuple for each command, e.g. after any substitution.

Gotcha, although it might be more straightforward to simply check the whole config at the end, ensuring that every plug maps to a unique (host,path) tuple for each command, e.g. after any substitution.

I've gone back and forth about that. Like maybe the first time someone does a stat/on/off, do a "is config legit?" kinda check? But I feel that nothing should stop me from performing a stat/on/off just because some other host is messed up config wise.

Perhaps we could add a "do_final_check" command that we add at the end of the login device script?

Maybe it's OK to allow non-unique mappings. It's on the dev script developer to get it right?

garlick · 2024-03-25T19:40:09Z

src/redfishpower/redfishpower.c

+ lplugsitr = hostlist_iterator_create(lplugs);
+ hostindicesitr = hostlist_iterator_create(hostindices);
+
+ while ((plug = hostlist_next(lplugsitr))
+ && (hostindexstr = hostlist_next(hostindicesitr))) {
+ if (setup_plug(plug, hostindexstr) < 0)
+ goto cleanup;
+ free(plug);
+ free(hostindexstr);
+ }


Suggestion: use hostlist_nth() and a for loop since the index counts are already known. Iterators shouldn't be needed.

garlick · 2024-03-25T19:50:44Z

man/redfishpower.8.in

 Associate a plug name with one of the hostnames specified on the
-command line, referred to by its zero origin index. Can be called
-multiple times to configure all possible plugs.
+command line, referred to by its zero origin index. Can be called
+multiple times to configure all possible plugs. In most
+cases the number of plugs should equal the number of indices.
+Multiple plugs can be mapped to a single host index if used with plug
+name substitution. The plug name can be substituted into the URI path
+by specifying "{{plug}}" when configuring the path.


The first sentence uses plug (singular) but plugnames can represent multiple plugs. Maybe it should say

Map one or more plug names to one or more hostnames specified on the command line. Hostnames are referenced by their zero-origin list index. For example: "setplugs p[0-7] [0-7]" assigns p0 to the first hostname, p1 to the second, etc..

On the new part, maybe instead of "in most cases" which sounds like a value judgement you could just say:

In some cases, multiple plugs may be mapped to the same hostname, for example when a blade chassis with one network address contains multiple slots, where a plug will represent just one slot in the chassis.

Shouldn't the "{{plug}}" substitution be described with the setpath command?

Shouldn't the "{{plug}}" substitution be described with the setpath command?

I went back on forth on this. Plug substitution only matters when you set > 1 plug to 1 host index, so I felt it belonged more in the setplugs documentation.

I guess I could add info on it in both areas.

Or you could look at it as simply substutitiong something in the path, and whether or not you eventually end up with uniqe plug -> (host, path) mappings is a higher level problem.

Or you could look at it as simply substutitiong something in the path, and whether or not you eventually end up with uniqe plug -> (host, path) mappings is a higher level problem.

(Related to other conversation regarding the plug-sub-required flag) I initially disliked the fact that if a user messes up the config, like pm --off foo-blade0 would actually end up powering off the entire chassis. Which is why I sort of put them all under the setplugs area (and have the plug-sub-required flag).

But maybe it's a higher level problem I shouldn't be concerned about. Like you say, I should just mention "{{plug}}" under setpath and remove the "plugsub required" flag. If the user messes up .. the user messes up?

Yeah I'm ok with that. Where user is the dev script developer.

garlick · 2024-03-25T19:59:16Z

t/etc/redfishpower-plugsub-blades.dev

+ send "setstatpath redfish/Default-{{plug}}/Stat\n"
+ expect "redfishpower> "
+ send "setonpath redfish/Default-{{plug}}/Reset {\"ResetType\":\"On\"}\n"
+ expect "redfishpower> "
+ send "setoffpath redfish/Default-{{plug}}/Reset {\"ResetType\":\"ForceOff\"}\n"
+ expect "redfishpower> "
+ send "setcyclepath redfish/Default-{{plug}}/Reset {\"ResetType\":\"ForceRestart\"}\n"


If setpath with a <cmd> argument is the new way, maybe new dev scripts should start using that instead of the old set<cmd>path form?

setpath requires you to specify plugnames, so not as convenient for setting "default" paths for everything (which is all we need in this test case).

Oh I thought it was just there for backwards compatibility.

it's there for that too :-)

Problem: In some chassis, power status/control URIs are all minorly different from each other when doing power status/control to blades. This makes it difficult to configure powerman/redfishpower for bladed environments. Solution: Support plug substitution in URIs with the pattern {{plug}}. By using plug names that are the minor variants of each URI, paths can be easily configured for each blade in a chassis. This now allows the setplugs command to map multiple plug names to a single index. Fixes chaos#129

Problem: redfishpower is out of date with recent updates supporting plug substituation in paths. Document use of {{plug}} plug substitution.

Problem: There is no coverage for the new redfishpower plug substitution in paths. Add basic coverage in t0034-redfishpower.t and add test device files t/etc/redfishpower-plugsub.dev and t/etc/redfishpower-plugsub-blades.dev.

chu11 · 2024-03-26T03:54:30Z

re-pushed with fixes from comments above

big one is removal of "plug substitution flag" and moving "{{plug}}" in the redfishpower(8) manpage. The user/developer is given a little less hand holding.

chu11 force-pushed the redfishpower_plug_substitution branch 6 times, most recently from f6c40fc to f3590cf Compare March 5, 2024 18:14

chu11 force-pushed the redfishpower_plug_substitution branch 2 times, most recently from 52c901b to 9cb8eff Compare March 19, 2024 20:43

chu11 mentioned this pull request Mar 19, 2024

redfishpower: support plug parents #164

Merged

chu11 force-pushed the redfishpower_plug_substitution branch from 9cb8eff to d0472a2 Compare March 19, 2024 22:58

chu11 force-pushed the redfishpower_plug_substitution branch from d0472a2 to 280c185 Compare March 21, 2024 23:03

garlick approved these changes Mar 25, 2024

View reviewed changes

chu11 added 3 commits March 25, 2024 20:53

man: update redfishpower(8) with plug substitution

e78f7c6

Problem: redfishpower is out of date with recent updates supporting plug substituation in paths. Document use of {{plug}} plug substitution.

t: cover redfishpower plug substitution

61469cd

Problem: There is no coverage for the new redfishpower plug substitution in paths. Add basic coverage in t0034-redfishpower.t and add test device files t/etc/redfishpower-plugsub.dev and t/etc/redfishpower-plugsub-blades.dev.

chu11 force-pushed the redfishpower_plug_substitution branch from 280c185 to 61469cd Compare March 26, 2024 03:53

chu11 added the merge-when-passing label Mar 26, 2024

mergify bot merged commit 35df57c into chaos:master Mar 26, 2024
8 checks passed

chu11 deleted the redfishpower_plug_substitution branch March 26, 2024 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redfishpower: support plug substitution #159

redfishpower: support plug substitution #159

chu11 commented Mar 2, 2024

chu11 commented Mar 19, 2024

garlick commented Mar 21, 2024

chu11 commented Mar 21, 2024

garlick commented Mar 21, 2024

chu11 commented Mar 21, 2024 •

edited

Loading

chu11 commented Mar 21, 2024

garlick left a comment

garlick Mar 25, 2024

garlick Mar 25, 2024

garlick Mar 25, 2024

chu11 Mar 25, 2024

garlick Mar 25, 2024

chu11 Mar 25, 2024

garlick Mar 25, 2024

garlick Mar 25, 2024

garlick Mar 25, 2024

garlick Mar 25, 2024

chu11 Mar 25, 2024

garlick Mar 25, 2024

chu11 Mar 25, 2024

garlick Mar 25, 2024

garlick Mar 25, 2024

chu11 Mar 25, 2024

garlick Mar 25, 2024

chu11 Mar 25, 2024

chu11 commented Mar 26, 2024


		/* if multiple plugs mapped to 1 host, plug substitution via %p
		* required */

redfishpower: support plug substitution #159

redfishpower: support plug substitution #159

Conversation

chu11 commented Mar 2, 2024

chu11 commented Mar 19, 2024

garlick commented Mar 21, 2024

chu11 commented Mar 21, 2024

garlick commented Mar 21, 2024

chu11 commented Mar 21, 2024 • edited Loading

chu11 commented Mar 21, 2024

garlick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chu11 commented Mar 26, 2024

chu11 commented Mar 21, 2024 •

edited

Loading