Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(377)

Issue 75250043: Add DistributionGroup to StartInstanceParams

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 11 months ago by axw
Modified:
5 years, 10 months ago
Reviewers:
mp+210746, wallyworld, fwereade, hduran
Visibility:
Public.

Description

Add DistributionGroup to StartInstanceParams DistributionGroup returns instances that StartInstance should consider when provisioning for high availability (i.e. the new instance + those in DistributionGroup should be distributed for high availability.) DistributionGroup is implemented in terms of a new provisioner API method of the same name. This API returns different instances depending on the machine passed in. For an environ manager, all other environ manager instances are returned; for a non-environ manager, all other instances with services in common are returned. This is in preparation for modifying the Azure provider to group instances into the same cloud service when in availability-sets-enabled mode. (Note: the previous patch included a change to PrecheckInstance; this has been reverted, and will be implemented differently in another CL.) https://code.launchpad.net/~axwalk/juju-core/startinstance-principalunit/+merge/210746 Requires: https://code.launchpad.net/~axwalk/juju-core/startinstance-param-struct/+merge/210738 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : Add PrincipalUnits to StartInstanceParams #

Patch Set 3 : Add DistributionInstances to StartInstanceParams #

Total comments: 2

Patch Set 4 : Add DistributionInstances to StartInstanceParams #

Patch Set 5 : Add DistributionGroup to StartInstanceParams #

Total comments: 6

Patch Set 6 : Add DistributionGroup to StartInstanceParams #

Total comments: 6

Patch Set 7 : Add DistributionGroup to StartInstanceParams #

Patch Set 8 : Add DistributionGroup to StartInstanceParams #

Unified diffs Side-by-side diffs Delta from patch set Stats (+327 lines, -4 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M environs/broker.go View 1 2 3 4 5 6 7 2 chunks +9 lines, -1 line 0 comments Download
M provider/manual/environ.go View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M state/api/params/params.go View 1 2 3 4 5 6 7 1 chunk +13 lines, -0 lines 0 comments Download
M state/api/provisioner/machine.go View 1 2 3 4 5 6 7 1 chunk +23 lines, -0 lines 0 comments Download
M state/api/provisioner/provisioner_test.go View 1 2 3 4 5 6 7 1 chunk +46 lines, -0 lines 0 comments Download
M state/apiserver/provisioner/provisioner.go View 1 2 3 4 5 6 7 2 chunks +102 lines, -0 lines 0 comments Download
M state/apiserver/provisioner/provisioner_test.go View 1 2 3 4 5 6 7 1 chunk +127 lines, -0 lines 0 comments Download
M worker/provisioner/provisioner_task.go View 1 2 3 4 5 6 7 1 chunk +4 lines, -3 lines 0 comments Download

Messages

Total messages: 13
axw
Please take a look.
5 years, 11 months ago (2014-03-13 06:40:19 UTC) #1
wallyworld
Looks ok pending William's +1. But there's a missing test for the client PrincipalUnits API ...
5 years, 11 months ago (2014-03-14 02:12:53 UTC) #2
axw
Please take a look.
5 years, 11 months ago (2014-03-14 02:31:19 UTC) #3
axw
Please take a look.
5 years, 11 months ago (2014-03-19 08:10:38 UTC) #4
hduran
https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/provisioner_test.go File state/api/provisioner/provisioner_test.go (right): https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/provisioner_test.go#newcode235 state/api/provisioner/provisioner_test.go:235: In CommonServiceInstances there are four return statements, three of ...
5 years, 11 months ago (2014-03-19 17:31:41 UTC) #5
axw
Please take a look. https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/provisioner_test.go File state/api/provisioner/provisioner_test.go (right): https://codereview.appspot.com/75250043/diff/40001/state/api/provisioner/provisioner_test.go#newcode235 state/api/provisioner/provisioner_test.go:235: On 2014/03/19 17:31:42, hduran wrote: ...
5 years, 11 months ago (2014-03-20 03:58:36 UTC) #6
axw
Please take a look.
5 years, 11 months ago (2014-03-20 06:40:50 UTC) #7
fwereade
Looks really good. I'm open to arguments re notfound/unauth; the only thing I'm not sure ...
5 years, 11 months ago (2014-03-20 09:23:53 UTC) #8
axw
Please take a look. https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisioner/provisioner_test.go File state/apiserver/provisioner/provisioner_test.go (right): https://codereview.appspot.com/75250043/diff/80001/state/apiserver/provisioner/provisioner_test.go#newcode568 state/apiserver/provisioner/provisioner_test.go:568: // show up in the ...
5 years, 11 months ago (2014-03-21 07:55:56 UTC) #9
fwereade
LGTM. Taking DG into account when choosing pre-existing instances is not in scope for this ...
5 years, 11 months ago (2014-03-24 10:12:20 UTC) #10
axw
Please take a look. https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provisioner/provisioner.go File state/apiserver/provisioner/provisioner.go (right): https://codereview.appspot.com/75250043/diff/100001/state/apiserver/provisioner/provisioner.go#newcode334 state/apiserver/provisioner/provisioner.go:334: // Sort values to simplify ...
5 years, 11 months ago (2014-03-24 14:12:20 UTC) #11
fwereade
LGTM
5 years, 11 months ago (2014-03-27 12:39:25 UTC) #12
axw
5 years, 10 months ago (2014-04-03 03:01:59 UTC) #13
Please take a look.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b