chaos / powerman

cluster power control
GNU General Public License v2.0
43 stars 19 forks source link

redfishpower: support setplugs configuration #157

Closed chu11 closed 7 months ago

chu11 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

chu11 commented 7 months ago

re-pushed, rebased on now merged #160

garlick commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

chu11 commented 7 months ago

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.