Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd: Add check-connection to jujud #7712
Conversation
wallyworld
approved these changes
Aug 8, 2017
I reckon we need to change the cmd constructor to just take a data dir
| + "github.com/juju/juju/worker/apicaller" | ||
| +) | ||
| + | ||
| +// ConnectFunc describes what we need to check whether the connection |
babbageclunk
Aug 8, 2017
•
Member
Yeah, I couldn't think of better wording - I've tweaked it to "ConnectFunc connects to the API as the given agent.".
| + | ||
| +// ReallyConnect really connects to the API specified in the agent | ||
| +// config. It's extracted so tests can pass something else in. | ||
| +func ReallyConnect(a agent.Agent) (io.Closer, error) { |
wallyworld
Aug 8, 2017
Owner
I'd prefer this be given a name like ConnectToAgent or something and the testing version be munged to be MockConnectToAgent.
"ReallyConnect" is what we'd expect business logic to do without adding that aspect to the method name. Best to keep the real logic / naming pure and rename things for tests.
babbageclunk
Aug 8, 2017
Member
I've changed it to ConnectAsAgent. ConnectToAgent doesn't seem right?
| + return &cmd.Info{ | ||
| + Name: "check-connection", | ||
| + Args: "<agent-name>", | ||
| + Purpose: "check connection to the API server", |
| + if tag.Kind() != "machine" && tag.Kind() != "unit" { | ||
| + return &util.FatalError{"agent-name must be a machine or unit tag"} | ||
| + } | ||
| + if err := cmd.CheckEmpty(args); err != nil { |
wallyworld
Aug 8, 2017
Owner
Move this to just above the tag, err := .... line
No need to go to the trouble of checking tags etc if we are only going to bail out anyway
| + if err := cmd.CheckEmpty(args); err != nil { | ||
| + return err | ||
| + } | ||
| + err = c.config.ReadConfig(agentName) |
wallyworld
Aug 8, 2017
Owner
I'm confused - if we are reading in the agent conf from a file, why pass an agent conf in at all? Why not just pass in the data dir and construct an empty conf from that?
babbageclunk
Aug 8, 2017
•
Member
I might be misreading the code, but an AgentConf is just a wrapper around a directory, right? If I passed in a data dir as a string wouldn't the tests have to create the dir(s) and a config file in it? If I take the AgentConf then I can pass in a mock conf and avoid filesystem access from the tests.
babbageclunk
Aug 8, 2017
Member
I think I might be missing your point though, so I'll check in with you about it anyway.
| @@ -181,6 +181,7 @@ func jujuDMain(args []string, ctx *cmd.Context) (code int, err error) { | ||
| jujud.Register(unitAgent) | ||
| jujud.Register(NewUpgradeMongoCommand()) | ||
| + jujud.Register(agentcmd.NewCheckConnectionCommand(agentConf, agentcmd.ReallyConnect)) |
wallyworld
Aug 8, 2017
Owner
So all we need here is the data dir from the current conf right? Let's pass that in instead of the whole conf which we are just going to overwrite anyway.
babbageclunk
Aug 8, 2017
Member
The AgentConf that gets passed in is empty - it doesn't have a current conf until something calls ReadConfig(agentName) on it. Or are you concerned about the conf being shared between the different commands? I can change that, anyway.
|
Not sure about the test failure here:
From reading the code, I thought maybe we were hitting the read deadline, but it's using endpointVersion 0, so that's 6 hours! So I don't know what's happening here. The websocket dies for some reason, and that in turn calls Close on the writeCloser. :( |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
babbageclunk commentedAug 8, 2017
Description of change
This takes an agent name and checks whether a connection can be made
to the API server from that agent's config. It will be used in the 1.25
upgrade process to confirm that the controller is accessible from all
of the agent machines once the config files have been rewritten (analogous
to the migration minions checking connectivity in a model migration).
QA steps
/var/lib/juju):apipasswordorapiservers, for example), you should get sensible error messages.Documentation changes
This isn't really intended for users - it will be called from the upgrade tool, so there's no need for documentation.