redfishpower: support setplugs configuration #157

chu11 · 2024-03-02T05:47:01Z

Problem: In the future it may be convenient to identify hosts
anonymously for easier configuration. Otherwise, many device
files will have to be created to configure different hosts.

Solution: Support a new setplug config which allows users to map
logical plugs to the indicies of the hosts configured. For example:

setplugs Node[0-15] [0-15]

This creates a set of plugs named Node[0-15]. These may be used
in place of nodenames input on the command line.

built on top of #156

garlick · 2024-03-03T05:55:51Z

The terminology is getting a little confusing.

The powerman model is that a "device" has a set of "plugs". "nodes" are mapped to "plugs" in the powerman.conf, and the device only ever sees "plugs".

So whatever you are getting from powerman is a plug (no matter what it looks like) and should be referred to as such in the docs and comments. If you start calling those things nodes it gets really confusing.

chu11 · 2024-03-03T07:57:05Z

Fixed as much as I could in the docs & manpage and comments, so hopefully things are more clear. I tried to move to "plug" terminology everywhere appropriate once setplugs was in place. Also, I added a commit which removed a lot of the "hostname" and "host" usage in variables / function names. They are generalized now as "targets".

garlick

Maybe it would make sense to create a class for plugs (preferably broken out to a separate source file)?

In the simulator I already did this, sort of. The logic does this:

      /* We have a valid power command, now ensure that plugs are
         * instantiated.  If "setplugs" was not called assume the device
         * script is not defining plugs and we must build plugs from
         * '-h hostlist'.
         */
        if (zhashx_size (plugs) == 0) {
            _plugs_add(plugs, hosts);
        }

I know the code was originally written with hosts as the first class entity and plugs added on but it really should be the other way IMHO, where plugs are the first class entity and it's a special case when the plug names match the hostnames.

garlick · 2024-03-03T13:48:10Z

src/redfishpower/redfishpower.c

@@ -151,10 +159,30 @@ void help(void)
    printf("  setstatpath path\n");
    printf("  setonpath path [postdata]\n");
    printf("  setoffpath path [postdata]\n");
+    printf("  setplugs plugnames hostindexes\n");


s/indexes/indices/ ?

garlick · 2024-03-03T13:58:40Z

src/redfishpower/redfishpower.c

+static int get_host_and_plug(char *input_target,
+                             char **target_hostname,
+                             char **plugname)
+{
+    if (hostlist_find(hosts, input_target) < 0) {
+        struct plug_data *pd;
+        /* not a host, perhaps is a plug name */
+        if (!(pd = zhashx_lookup(plug_map, input_target)))
+            return -1;
+        (*target_hostname) = pd->hostname;
+        (*plugname) = input_target;
+    }
+    else {
+        (*target_hostname) = input_target;
+        (*plugname) = input_target;
+    }
+    return 0;
 }


Suggestion, to reinforce terminlogy

static const char *plug2host (const char *plug) { struct plug_data *pd = zhashx_lookup (plug_map, plug); if (pd) return pd->hostname; if (hostlist_find (hosts, plug) >= 0) return plug; return NULL; }

garlick · 2024-03-03T14:05:22Z

src/redfishpower/redfishpower.c

+    if (get_host_and_plug(hostname, &target_hostname, &plugname) < 0) {
+        printf("unknown host/plug specified: %s\n", hostname);
+        return NULL;
+    }


Suggestion: rename the hostname parameter of stat_cmd_host() to plug. Maybe "host" shouldn't be in the function name either.

Error message should say "unknown plug specified".

The thing powerman wants you to operate on is always a plug, even if it happens to be the same as a hostname.

garlick · 2024-03-03T14:17:27Z

src/redfishpower/redfishpower.c

+    else {
+        /* if plugs are used, use all plugs, otherwise use all hosts */
+        if (hostlist_count(plugs) > 0)
+            hostsptr = &plugs;
+        else
+            hostsptr = &hosts;
+    }


This function also uses hosts where plugs would be more appropriate (e.g. hostsptr) and the comment feels slightly off since plugs are always used - the question is whether they are defined by setplugs or not

garlick · 2024-03-03T14:29:54Z

Also, I added a commit which removed a lot of the "hostname" and "host" usage in variables / function names. They are generalized now as "targets".

Oh crud, I think I was reviewing the wrong version. Sorry!

Why not just use "plugs" though? It's always a plug. Reminder: this is a powerman plugin.

chu11 · 2024-03-03T20:50:39Z

Oh crud, I think I was reviewing the wrong version. Sorry!

Lemme re-work this PR. Maybe I'll split it up. One PR that renames everything + adds simple re-architecture to ensure everything internally is a plug. Then we can do the setplugs config afterwards.

garlick · 2024-03-04T14:34:07Z

I think this logic assumes setplugs will only be called once? But it will need to be called multiple times when the parent plug field is added, assuming you are following the design proposed in #127, e.g.

                # setplugs <pluglist> <hostindices> [<parentplug>]
                send "setplugs Enclosure 0\n"
                expect "redfishpower> "
                send "setplugs Perif[0-7],Blade[0-7] 0 Enclosure\n"
                expect "redfishpower> "
                send "setplugs Node[0-1] [1-2] Blade0\n"
                expect "redfishpower> "
                send "setplugs Node[2-3] [3-4] Blade1\n"
                expect "redfishpower> "
                send "setplugs Node[4-5] [5-6] Blade2\n"
                expect "redfishpower> "
                send "setplugs Node[6-7] [7-8] Blade3\n"
                expect "redfishpower> "
                send "setplugs Node[8-9] [9-10] Blade4\n"
                expect "redfishpower> "
                send "setplugs Node[10-11] [11-12] Blade5\n"
                expect "redfishpower> "
                send "setplugs Node[12-13] [13-14] Blade6\n"
                expect "redfishpower> "
                send "setplugs Node[14-15] [15-16] Blade7\n"
                expect "redfishpower> "

I haven't looked at the whole PR stack yet so apologies if this is sorted out later.

Also, might be good to create "plugs" class or similar in another source file. Then unit tests could be added later, and it would reduce the amount of effort needed to grok redfishpower.c.

chu11 · 2024-03-04T18:02:21Z

I haven't looked at the whole PR stack yet so apologies if this is sorted out later.

Yeah it is sorted out later. Because there is no plug substitution yet and different paths for different plugs isn't there yet, some configurations make no sense at this point in the patch series. So it'll work itself out later.

Also, might be good to create "plugs" class or similar in another source file. Then unit tests could be added later, and it would reduce the amount of effort needed to grok redfishpower.c.

Now that I think about it, the way I began prototyping I didn't put a lot of data into each struct plug_data structure. So perhaps it'll work out better if I put it into another file and add everything into that struct. Lemme try that (most of the heavy work would begin in PR #160 though)

chu11 · 2024-03-05T05:55:38Z

re-pushed based on the plugs module that was introduced in #160 (reminder that #160 comes before this)

chu11 · 2024-03-14T17:07:38Z

re-pushed, rebased on now merged #160

garlick · 2024-03-16T18:05:54Z

Sorry for the delay - I'll try to get this reviewed ASAP.

One quick comment - should setplugs require that the plug names be explicitly defined in the dev script, and then all references to plugs be checked against that?

chu11 · 2024-03-16T21:58:38Z

One quick comment - should setplugs require that the plug names be explicitly defined in the dev script, and then all references to plugs be checked against that?

Not quite sure I follow. Since setplugs can be called multiple times I can't check within setplugs that all plugs have been assigned, bc some will be in the future.

I could check if all hosts have a plug assigned before doing on/off/stat for the first time. But I thought that was overkill since I do a "valid plug name" check all the time.

Or are you saying needs some extra documentation for this fact?

garlick · 2024-03-16T22:07:31Z

Oh sorry! I think I was confused there. Strike that!

garlick

Sorry this took so long! I added some comments inline. You can pick and choose which you think are important to do here and I'll mark this approved.

garlick · 2024-03-19T15:00:41Z

src/redfishpower/redfishpower.c

+    }
+
+    if (!(lplugs = hostlist_create(av[0]))) {
+        printf("illegal plugs input\n");


would it be handy to prefix all the setplugs failure messages with setplugs: to disambiguate these failures from others involving plugs? E.g. setplugs: malformed pluglist

garlick · 2024-03-19T15:03:11Z

src/redfishpower/redfishpower.c

+    if ((plugcount = hostlist_count(lplugs)) < 0)
+        err_exit(true, "hostlist_count error");
+    if ((indexcount = hostlist_count(indices)) < 0)
+        err_exit(true, "hostlist_count error");


it doesn't look like hostlist_count() can fail.

garlick · 2024-03-19T15:08:19Z

src/redfishpower/redfishpower.c

+    if (!(lplugsitr = hostlist_iterator_create(lplugs)))
+        err_exit(true, "hostlist_iterator_create");
+    if (!(indicesitr = hostlist_iterator_create(indices)))
+        err_exit(true, "hostlist_iterator_create");


These functions can't fail.

Note that the behavior of all the libsd functions on ENOMEM is configurable by setting WITH_LSD_NOMEM_ERROR_FUNC which we do project wide.

garlick · 2024-03-19T15:15:47Z

src/redfishpower/redfishpower.c

+    if ((indexcount = hostlist_count(indices)) < 0)
+        err_exit(true, "hostlist_count error");
+    if (plugcount != indexcount) {
+        printf("plugs count not equal to index count");


Maybe index should be "host index" since "index" is pretty nonspecific?

garlick · 2024-03-19T15:20:08Z

src/redfishpower/redfishpower.c

+           && (indexstr = hostlist_next(indicesitr))) {
+        char *host;
+        char *endptr;
+        int index;
+
+        errno = 0;
+        index = strtol (indexstr, &endptr, 10);
+        if (errno
+            || endptr[0] != '\0'
+            || index < 0) {
+            printf("invalid index %s specified\n", indexstr);
+            free(plug);
+            free(indexstr);
+            goto cleanup;
+        }


It might clean this block up a bit if

instead of two iterators, just do for (int i = 0; i < plugcount; i++)

use hostlist_nth() instead of the iterator values

add a function for looking up a host from the nth host index

ugh I'm dumb ... later on in the PR series I create a "setup_plug()" function. Let me just bring that down now and it'll clean up the code a lot.

garlick · 2024-03-19T15:22:46Z

man/redfishpower.8.in

@@ -60,6 +60,10 @@ Set path and optional post data to turn on plug.
 .I "setoffpath <path> [postdata]"
 Set path and optional post data to turn off plug.
 .TP
+.I "setplugs plugnames hostindexes"


indices is plural of index

garlick · 2024-03-19T15:25:10Z

man/redfishpower.8.in

@@ -60,6 +60,10 @@ Set path and optional post data to turn on plug.
 .I "setoffpath <path> [postdata]"
 Set path and optional post data to turn off plug.
 .TP
+.I "setplugs plugnames hostindexes"
+Set plug names for index of hosts input to redfishpower.  Can be


Would it be better to say "associate a plug name with one of the hostnames specified on the command line, referred to by its zero origin index".

garlick · 2024-03-19T15:25:43Z

man/redfishpower.8.in

@@ -60,6 +60,10 @@ Set path and optional post data to turn on plug.
 .I "setoffpath <path> [postdata]"
 Set path and optional post data to turn off plug.
 .TP
+.I "setplugs plugnames hostindexes"
+Set plug names for index of hosts input to redfishpower.  Can be
+called multiple times to configure all possible targets.


s/targets/plugs

garlick · 2024-03-19T15:27:50Z

t/etc/redfishpower-setplugs.dev

+# Variant of redfishpower-cray-r272z30.dev that covers use of setplugs
+# configuration
+specification "redfishpower-setplugs" {
+	timeout 	60


Would it be helpful to develop a sharness test that runs redfishpower standalone rather than through powerman? This test seems awfully complicated just to check that setplugs works, and presumably we'll eventually have a dev script that uses all the new features in test (like the one I proposed).

yeah, the main reason I ran through powerman is b/c there isn't a facility to run redfishpower somewhat independently of powerman at the moment.

For the time being, I'll splice out the "redfishpower" specific tests into a new file, rather than keep it within the "redfish" set of tests. Since they are clearly of a different beast than the device files in /etc.

Probably I'm oversimplifying again :-)

I was vaguely thinking you could write a sharness test that drove redfishpower with a here-is document in a sharness script but maybe that's not practical.

Problem: Some tests are not so much "redfish device" tests, but rather tests to ensure configuration within redfishpower is working. And more tests of this nature will be added in the future. Move these redfishpower specific configuration tests to a new test file t0034-redfishpower.t.

chu11 · 2024-03-19T17:55:22Z

re-pushed with fixes per comments above. The only new commit is splicing out tests into a a new t0034-redfishpower.t file that are redfishpower specific vs "redfish device" tests. There's a lot more tests coming so I think the split is a good idea at this point.

going to rebase rest of the PR chain ... as long as I didn't bork something up too big will set MWP after that.

Problem: Redfishpower currently assumes plugnames are equal to the hosts configured on the redfishpower command line. In the future this can lead to difficulty doing advanced configuration. A different device file would be needed for any hosts that are different. Solution: Support a new setplug config which allows users to map logical plugs to the indicies of the hosts configured. For example: setplugs Node[0-15] [0-15] This creates a set of plugs named Node[0-15]. These may be used in place of nodenames input on the command line.

Problem: redfishpower is out of date with recent setplugs support. Document setplugs interactive command.

Problem: There is no coverage for the new redfishpower setplugs configuration. Add basic coverage in t0034-redfishpower.t and add test device file t/etc/redfishpower-setplugs.dev.

chu11 mentioned this pull request Mar 2, 2024

redfishpower: support setpath configuration #158

Merged

chu11 force-pushed the redfishpower_setplugs branch from 3c19d26 to 8cf518c Compare March 3, 2024 05:27

chu11 force-pushed the redfishpower_setplugs branch 2 times, most recently from e7e6e61 to d0124ee Compare March 3, 2024 07:54

garlick reviewed Mar 3, 2024

View reviewed changes

chu11 mentioned this pull request Mar 4, 2024

redfishpower: refactor internals to use plugs #160

Merged

chu11 force-pushed the redfishpower_setplugs branch from d0124ee to b7bc634 Compare March 4, 2024 07:31

chu11 force-pushed the redfishpower_setplugs branch from b7bc634 to 2f486a8 Compare March 5, 2024 05:54

chu11 force-pushed the redfishpower_setplugs branch from 2f486a8 to 73e3b9b Compare March 5, 2024 18:13

chu11 force-pushed the redfishpower_setplugs branch from 73e3b9b to 03a590e Compare March 14, 2024 17:06

garlick approved these changes Mar 19, 2024

View reviewed changes

chu11 force-pushed the redfishpower_setplugs branch from 03a590e to 52949a4 Compare March 19, 2024 17:54

chu11 force-pushed the redfishpower_setplugs branch from 52949a4 to f06df25 Compare March 19, 2024 17:58

chu11 force-pushed the redfishpower_setplugs branch from f06df25 to 69a5a4d Compare March 19, 2024 18:42

chu11 added 2 commits March 19, 2024 11:51

man: update redfishpower(8) with setplugs info

d51d341

Problem: redfishpower is out of date with recent setplugs support. Document setplugs interactive command.

t: cover redfishpower setplugs config

05583dc

Problem: There is no coverage for the new redfishpower setplugs configuration. Add basic coverage in t0034-redfishpower.t and add test device file t/etc/redfishpower-setplugs.dev.

chu11 force-pushed the redfishpower_setplugs branch from 69a5a4d to 05583dc Compare March 19, 2024 18:51

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

mergify bot merged commit a8908f2 into chaos:master Mar 19, 2024
8 checks passed

chu11 deleted the redfishpower_setplugs branch March 19, 2024 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redfishpower: support setplugs configuration #157

redfishpower: support setplugs configuration #157

chu11 commented Mar 2, 2024

garlick commented Mar 3, 2024

chu11 commented Mar 3, 2024

garlick left a comment

garlick Mar 3, 2024

garlick Mar 3, 2024

garlick Mar 3, 2024

garlick Mar 3, 2024

garlick commented Mar 3, 2024

chu11 commented Mar 3, 2024

garlick commented Mar 4, 2024

chu11 commented Mar 4, 2024

chu11 commented Mar 5, 2024

chu11 commented Mar 14, 2024

garlick commented Mar 16, 2024

chu11 commented Mar 16, 2024

garlick commented Mar 16, 2024

garlick left a comment

garlick Mar 19, 2024

garlick Mar 19, 2024

garlick Mar 19, 2024

garlick Mar 19, 2024

garlick Mar 19, 2024

chu11 Mar 19, 2024

garlick Mar 19, 2024

garlick Mar 19, 2024

garlick Mar 19, 2024

garlick Mar 19, 2024

chu11 Mar 19, 2024 •

edited

Loading

garlick Mar 19, 2024

chu11 commented Mar 19, 2024

redfishpower: support setplugs configuration #157

redfishpower: support setplugs configuration #157

Conversation

chu11 commented Mar 2, 2024

garlick commented Mar 3, 2024

chu11 commented Mar 3, 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

garlick commented Mar 3, 2024

chu11 commented Mar 3, 2024

garlick commented Mar 4, 2024

chu11 commented Mar 4, 2024

chu11 commented Mar 5, 2024

chu11 commented Mar 14, 2024

garlick commented Mar 16, 2024

chu11 commented Mar 16, 2024

garlick commented Mar 16, 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

chu11 Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chu11 commented Mar 19, 2024

chu11 Mar 19, 2024 •

edited

Loading