DAOS-16487 control: Only use hostlist from cfg on cfg-aware dmg cmds #15087
Conversation
|
Ticket title is 'dmg storage must precisely identify NVMe device(s)' |
|
Test stage Test RPMs on EL 8.6 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15087/1/execution/node/1162/log |
|
Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15087/1/execution/node/1183/log |
There was a problem hiding this comment.
Based on the test failures, it looks like some commands that should implement cmdConfigSetter currently aren't (i.e. the config isn't being loaded for them, but should have been). But I think your basic premise is correct, we shouldn't load a config unless the command implements a specific interface.
tanabarr
changed the title
|
Gatekeeper please use the PR description and title as the commit message when landing, TIA |
Feature: control Required-githooks: true Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
tanabarr
force-pushed
the
tanabarr/control-dmg-cfg-load-dependent
branch
from
b248cbc to
45246ef
Compare
| if cfgCmd, ok := cmd.(cmdConfigSetter); ok { | ||
| // Override hostlist with cli value if provided. | ||
| if hlCmd, ok := cmd.(hostListGetter); ok { | ||
| hl := hlCmd.getHostList() | ||
| if len(hl) > 0 { | ||
| ctlCfg.HostList = hl | ||
| } |
There was a problem hiding this comment.
This seems pretty hard to understand to me. I would imagine that it will be even more perplexing in a few months. We already have the singleHostCmd type which could probably be reworked a little to make this use case clearer. For example, the type could have a new getHost() method and then you could define a singleHostGetter interface that could be used to drive the logic for setting the request's hostlist to that single host.
I think I understand the motivation behind maintaining the hostlist stuff in the implementation of the singleHostCmd (who knows, maybe I even advocated for it at the time), but with the benefit of hindsight and context, it just seems overly complicated and kind of confusing now. I wouldn't be opposed to refactoring it so that it is clearly the thing to be used for commands that only accept a single host.
Another change that I would recommend as part of that refactoring is to get rid of the logic in singleHostCmd.getHostList() that picks a default from either a hard-coded "localhost" or the first host in the hostlist. That kind of implicit behavior requires a lot of knowledge about the implementation in order to use it correctly, IMO. Instead, I'd suggest that if there is to be a default host, it should be explicitly set by the code that is using the type.
@kjacque: It looks like you added this type in #11815 ... What do you think about reworking it a bit to make the behavior and usage clearer?
There was a problem hiding this comment.
I agree that currently the implementation is not very clean, and I'm happy to take another pass based on this feedback and anything @kjacque has to add.
There was a problem hiding this comment.
this isn't quite resolved by #15074 , updating singleHostCmd doesn't resolve the issue where hostlist can be pulled by the config in commands that don't explicitly require config usage
| if ctlCmd, ok := cmd.(ctlInvoker); ok { | ||
| ctlCmd.setInvoker(invoker) | ||
| } | ||
|
|
||
| // Handle the deprecated global hostlist flag |
There was a problem hiding this comment.
Seems like this is a bit out of order now -- should happen after the dealings with the config so it can override if necessary. Maybe it would be helpful to create a helper function that uses either of the command line flags, so they're processed together.
During dmg command initialisation, load ctl config from file to
preload cert data but in the case that the command does not embed
cfgCmd (and therefore doesn't intend to use config settings), strip
hostlist before setting config on invoker, this will prevent
inadvertent use of hostlist from a config file on commands that are
supposed not to.
Required-githooks: true
Before requesting gatekeeper:
Features:(orTest-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.Gatekeeper: