/juju

Allow enable-ha to work regardless of selected model #6421

Merged
merged 8 commits into from Oct 11, 2016

Conversation

Reviewers

@wallyworld wallyworld

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mjs @jujubot @wallyworld
@mjs
Contributor

mjs commented Oct 11, 2016 •
Edited 1 time

juju enable-ha now just works without requiring the controller model be selected.

There were a number of things done to make this work and some cleanup too:

  • The HighAvailability facade is now available for controller logins. It is also still available for model logins for compatibility with clients outside of Juju.
  • The unnecessarily exposed apiserver.highavailability.EnableHASingle function has been hidden.
  • The EnableHA API call will now fail if given more than one controller spec. Passing more than one makes no sense. This allowed EnableHA to be somewhat simplified.
  • Support has been removed for the model-tag field previously validated (but not used) by the EnableHA API. This will not break old clients if they send it, but it is no longer checked.
  • The client side highavailability facade no longer sends the model tag.
  • The juju enable-ha command is now a controller command instead of a model command. This means that -m controller is no longer necessary and the -c arg is now available if needed.

Fixes https://bugs.launchpad.net/juju/+bug/1607170

QA

Ran juju enable-ha with various arguments when the default model was selected.

Also confirmed that an rc3 client can enable HA as before using juju enable-ha -m controller.

mjs added some commits Oct 10, 2016

@mjs
apiserver: Test common facades with model login 
This closes a small testing gap.
46e5484
@mjs
apiserver: Expose HighAvailability correctly 
This facade should have always been for controller logins only, but it
has been left exposed for model only logins for compatibility reasons.
83007f2
@wallyworld

wallyworld approved these changes Oct 11, 2016
View changes

Did you also test with a rc3 client which still expects the facade to be a model facade? To simulate what externals clients would be calling.

apiserver/highavailability/highavailability.go
@@ -54,23 +54,32 @@ func NewHighAvailabilityAPI(st *state.State, resources facade.Resources, authori
}, nil
}
+// EnableHA adds controller machines as necessary to ensure the
+// controller has the number of machine specified.
@wallyworld

wallyworld Oct 11, 2016

Owner

machines

@mjs

mjs Oct 11, 2016

Contributor

Fixed.

apiserver/restrict_controller.go
@@ -27,6 +27,7 @@ var controllerFacadeNames = set.NewStrings(
var commonFacadeNames = set.NewStrings(
"Pinger",
"Bundle",
+ "HighAvailability", // Exposed for model logins for backwards compatibility.
@wallyworld

wallyworld Oct 11, 2016

Owner

Can you make this a bug TODO to fix once we very that no external clients use it. Target the bug to 2.0.1

@mjs

mjs Oct 11, 2016

Contributor

Done.

cmd/juju/commands/enableha.go
-to ensure that the specified number of controllers are made available.
+must itself be highly available. The enable-ha command will ensure
+that the specified number of controller machines are used to make up the
+controller.
@wallyworld

wallyworld Oct 11, 2016

Owner

... are used to make up the controller cluster.

Perhaps? TBH, "...the specified number of controllers...." reads ok to me.

@mjs

mjs Oct 11, 2016

Contributor

As per IRC, I'm pretty sure that "controller" is supposed to refer to the cluster (if any).

mjs added some commits Oct 11, 2016

@mjs
apiserver/highavailability: Hide EnableHASingle 
This function is no longer called outside the package. Removed the
misleading docstring too.
f798343
@mjs
apiserver/highavailability: Support one controller 
There is no way that EnableHA could support anything but one controller
so return an error if more than one controller spec is passed. Limiting
handling to just one spec also allowed signficant simplification of the
EnableHA function.
b262f63
@mjs
apiserver/highavailability: Don't check model tag 
The model tag field accepted by EnableHA was optional and validated but
not actually used for anything. It didn't make sense anyway and wasnt'
being tested.

Removing it won't break older clients. The field will just be ignored.
f3f22ef
@mjs
api/highavailability: Stop sending model tag 
It never made sense and is no longer checked server side. It wasn't
being tested here either.
e737cdd
@mjs
cmd/juju/commands: Make enable-ha controller based 
The enable-ha command is now a controller command instead of a model
command. This means it can be issued regardless of the currently
selected model.

Also improved the command help text slightly.

Fixes https://bugs.launchpad.net/juju/+bug/1607170
966e14c
@mjs
apiserver: Add TODO about HighAvailability
ba13a3d
@mjs
Contributor

mjs commented Oct 11, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 11, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 0a51043 into juju:master Oct 11, 2016

@mjs mjs deleted the mjs:1607170-enable-ha-args branch Oct 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment