Skip to content
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

DAOS-16487 control: Only use hostlist from cfg on cfg-aware dmg cmds #15087

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

DAOS-16487 control: Only use hostlist from cfg on cfg-aware dmg cmds #15087

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 15 additions & 11 deletions src/control/cmd/dmg/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,11 +284,6 @@ and access control settings, along with system wide operations.`
return errors.Wrap(err, "Unable to load Certificate Data")
}

invoker.SetConfig(ctlCfg)
if ctlCmd, ok := cmd.(ctlInvoker); ok {
ctlCmd.setInvoker(invoker)
}

// Handle the deprecated global hostlist flag
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if !opts.HostList.Empty() {
if hlCmd, ok := cmd.(hostListSetter); ok {
Expand All @@ -301,15 +296,24 @@ and access control settings, along with system wide operations.`
}
}

if hlCmd, ok := cmd.(hostListGetter); ok {
hl := hlCmd.getHostList()
if len(hl) > 0 {
ctlCfg.HostList = hl
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
}
Comment on lines +299 to +305
Copy link
Contributor

@mjmac mjmac Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
cfgCmd.setConfig(ctlCfg)
} else {
// Before setting config on invoker strip hostlist as in the case that the
// command doesn't support config, the host list should be derived from cmd.
ctlCfg.HostList = nil
}

if cfgCmd, ok := cmd.(cmdConfigSetter); ok {
cfgCmd.setConfig(ctlCfg)
invoker.SetConfig(ctlCfg)
if ctlCmd, ok := cmd.(ctlInvoker); ok {
ctlCmd.setInvoker(invoker)
}

if argsCmd, ok := cmd.(cmdutil.ArgsHandler); ok {
Expand Down
Loading