Keyboard Shortcuts

File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(26)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 10959043: Azure provider: map instances onto Azure services.

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 6 months ago by rvb
Modified:
12 years, 6 months ago
Reviewers:
mp+173140, gz , jameinel
Visibility:
Public.
Azure provider: map instances onto Azure services. We had to change our strategy w.r.t. how the Azure provider maps Juju instances onto Azure's objects. Our original plan was to use a Service as a container for all the Deployments created by the provider, each of these deployments corresponding to a Juju node. This is graphical representation of the model we had in mind: Account > - Hosted Service (container configured in the juju config) - Deployment 1 (juju instance 1) - VM - Deployment 2 (juju instance 2) - VM - Deployment 3 (juju instance 3) - VM We thought we could use Service as a container of Deployments (and thus, VMs). But we can't! It turns out a Service can only be deployed to "staging" and to "production"... and those are its two Deployments. You can't add more. Here is the Azure model as we now know it: Account > - Hosted Service 1 > - Deployment, production slot > - VM - VM - Deployment, staging slot > - VM - VM - Hosted Service 2 > We discussed our options with the Red Squad and mgz and reached the conclusion that having one hosted service per Juju instance is the only reasonnable solution to accomodate Juju's model with how Azure's structure. We're going with: one Juju Instance is one Azure Service, containing one Deployment, containing one VM. Account > (container configured in the juju config) - Hosted Service 1 (juju instance 1) - Deployment 1 - VM - Hosted Service 2 (juju instance 2) - deployment 2 - VM - Hosted Service 3 (juju instance 3) - deployment 3 - VM In this branch, I change the code from the old model to the new. The changes are surprinsingly straighforward: - In Environ.Instances() and Environ.AllInstances(), use the gwacl method to list the hosted services instead of the method used to list deployments. - Use the gwacl object representing a Service instead of the one representing a Deployment inside the instance.go:azureInstance object. Note that the hostname of the Azure instance, which is stored by Azure on the Deployment object, will be copied over (by the Azure provider, when the hosted service and the deployment will be created) in the Hosted Service's base64-encoded 'Label' field. This is mainly a convenient shortcut to avoid having to fetch the Deployment inside each Service when we want a list of the instances' hostnames. - Get rid of the management hosted service name config item. https://code.launchpad.net/~rvb/juju-core/use-hosted-services/+merge/173140 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 1
Created: 12 years, 6 months ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+119 lines, -114 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M environs/azure/config.go View 5 chunks +13 lines, -26 lines 0 comments Download
M environs/azure/config_test.go View 5 chunks +15 lines, -31 lines 0 comments Download
M environs/azure/environ.go View 4 chunks +19 lines, -18 lines 1 comment Download
M environs/azure/environ_test.go View 8 chunks +26 lines, -22 lines 0 comments Download
M environs/azure/instance.go View 2 chunks +15 lines, -4 lines 0 comments Download
M environs/azure/instance_test.go View 2 chunks +29 lines, -13 lines 0 comments Download
Total messages: 4
|
rvb
Please take a look.
12 years, 6 months ago (2013年07月05日 09:04:43 UTC) #1
Please take a look.
Sign in to reply to this message.
jameinel
LGTM I think you are losing the environment grouping you used to have (because one ...
12 years, 6 months ago (2013年07月05日 10:23:54 UTC) #2
LGTM
I think you are losing the environment grouping you used to have (because one
storage name was intended to be used for all instances in an env). So we'll need
to consider how we get that back.
Otherwise, looks good.
https://codereview.appspot.com/10959043/diff/1/environs/azure/environ.go
File environs/azure/environ.go (right):
https://codereview.appspot.com/10959043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:190: hostedServices, err :=
context.ListHostedServices()
I'm not seeing where you are filtering the list of available services by
something that restricts it to just the current environment.
You should be able to have 2 juju environments running with the same user
account, and not have them step on eachother. (And not have 'juju
destroy-environment' end up destroying both environments.)
I believe in the Openstack provider we set the instance name according to a
pattern that includes the environment name.
Something like "juju $ENV 0". Then when we query for all instances, we can
filter based on that prefix.
It doesn't have to be done in this patch, but you'll want to consider how you
are modelling multiple environments in the same Azure account.
Sign in to reply to this message.
rvb
On 2013年07月05日 10:23:54, jameinel wrote: > LGTM > > I think you are losing the ...
12 years, 6 months ago (2013年07月05日 14:59:14 UTC) #3
On 2013年07月05日 10:23:54, jameinel wrote:
> LGTM
> 
> I think you are losing the environment grouping you used to have (because one
> storage name was intended to be used for all instances in an env). So we'll
need
> to consider how we get that back.
> 
> Otherwise, looks good.
> 
> https://codereview.appspot.com/10959043/diff/1/environs/azure/environ.go
> File environs/azure/environ.go (right):
> 
>
https://codereview.appspot.com/10959043/diff/1/environs/azure/environ.go#newc...
> environs/azure/environ.go:190: hostedServices, err :=
> context.ListHostedServices()
> I'm not seeing where you are filtering the list of available services by
> something that restricts it to just the current environment.
> 
> You should be able to have 2 juju environments running with the same user
> account, and not have them step on eachother. (And not have 'juju
> destroy-environment' end up destroying both environments.)
> 
> I believe in the Openstack provider we set the instance name according to a
> pattern that includes the environment name.
> 
> Something like "juju $ENV 0". Then when we query for all instances, we can
> filter based on that prefix.
> 
> It doesn't have to be done in this patch, but you'll want to consider how you
> are modelling multiple environments in the same Azure account.
Thanks for the review jam!
I think you're right, we should support having 2 juju environments running with
the same user account. I'm adding something similar to what the Openstack
provider does.
Sign in to reply to this message.
gz
LGTM, pending StartInstance etc implementation. I'll echo John's comment that including the environment name and ...
12 years, 6 months ago (2013年07月05日 15:12:12 UTC) #4
LGTM, pending StartInstance etc implementation.
I'll echo John's comment that including the environment name and machine tag in
the service name seems like a good idea, compare environs/openstack/provider.go
machineFullName and machinesFilter.
Sign in to reply to this message.
|
Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /