Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add instance types #6564
Conversation
|
!!build!! |
|
!!try!! |
|
!!try!! |
axw
requested changes
Nov 16, 2016
Mostly looking good. I think we should get input from @urosj or someone on GUI about cost/currency details.
| +package provider | ||
| + | ||
| +import ( | ||
| + names "gopkg.in/juju/names.v2" |
perrito666
Nov 16, 2016
Contributor
yes, I just realized that, mm, Ill have to check what is going on with goimports so I can disable it
| + | ||
| +// NewAPI returns an API pointer. | ||
| +func NewAPI(backend Backend, authorizer facade.Authorizer, envNew environs.NewEnvironFunc) (*API, error) { | ||
| + if !authorizer.AuthModelManager() { |
| + }, nil | ||
| +} | ||
| + | ||
| +type modelConfigAble interface { |
| + return params.InstanceTypesResults{}, errors.Trace(err) | ||
| + } | ||
| + | ||
| + for i, c := range cons.Constraints { |
axw
Nov 16, 2016
Member
Please add a TODO to cache the results. We need to avoid repeatedly querying the cloud, but cannot poll the cloud for reasons described in the spec.
| + | ||
| +var _ = gc.Suite(&providerTypesSuite{}) | ||
| + | ||
| +var over9kCPUCores uint64 = 9001 |
| + m := mockEnviron{ | ||
| + results: map[constraints.Value]instances.InstanceTypesWithCostMetadata{ | ||
| + itCons: instances.InstanceTypesWithCostMetadata{ | ||
| + CostUnit: "USD/h", |
axw
Nov 16, 2016
Member
It looks weird having both USD/h and USD, but let's come back to it after seeing exactly how the GUI wants to format things. Easy enough to massage into shape.
| +// InstanceTypesWithCostMetadata holds an array of InstanceType and metadata | ||
| +// about their cost. | ||
| +type InstanceTypesWithCostMetadata struct { | ||
| + InstanceTypes []InstanceType |
| @@ -0,0 +1,33 @@ | ||
| +package azure |
| +package azure | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| + } | ||
| + result := make([]instances.InstanceType, len(types)) | ||
| + i := 0 | ||
| + for _, iType := range types { |
rogpeppe
Nov 16, 2016
•
Edited 1 time
-
rogpeppe
Nov 16, 2016
Owner
-
Nov 16, 2016
My preferred idiom for that is:
result := make([]instances.InstanceType, 0, len(types))
for _, iType := range types {
result = append(result, iType)
}
perrito666
Nov 16, 2016
Contributor
@rogpeppe cool, I guess I know why but could you expand on the underlying of that?
axw
Nov 20, 2016
Member
@perrito666 like rogpeppe, I do the same when going from map to slice. Only because it's more compact and reads better (to me).
| + result[i] = iType | ||
| + i++ | ||
| + } | ||
| + result, err = instances.MatchingInstanceTypes(result, "", c) |
axw
Nov 16, 2016
Member
I wasn't suggesting you move this into each provider. You can/should do this outside of each provider, but you also need the constraints in the provider-level API because some providers will need to generate results based on those constraints. i.e. the constraints are required for generic filtering, but also for generating custom instance types.
perrito666
Nov 16, 2016
Contributor
Well, do we really want double filtering? I believe that its saner if each provider filters its own types but am open to be convinced of the contrary.
axw
Nov 16, 2016
Member
I'm not too fussed, I'd just prefer not to pass the buck to the provider if we can avoid it, and also avoid repeating code in each provider. Maybe it's better to have the responsibility of the provider clear, and require that it honour the constraints.
| @@ -0,0 +1,16 @@ | ||
| +package cloudsigma |
| +package cloudsigma | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| @@ -0,0 +1,16 @@ | ||
| +package dummy |
| +package dummy | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| + return instances.InstanceTypesWithCostMetadata{}, errors.Trace(err) | ||
| + } | ||
| + for i, t := range iTypes { | ||
| + iTypes[i].Cost = t.Cost / 1000 |
axw
Nov 16, 2016
Member
Nope. I must have been unclear, sorry.
What I meant was to include a divisor in the result struct, not to truncate/drop precision in the cost field. i.e. the results should include:
- the cost (e.g. in deci-cents)
- a divisor (e.g. of 1000) to get to dollars, or whatever the unit is
- the currency/unit (e.g. $USD/h)
e.g. with cost=1234, divisor=1000, currency/unit=$USD/h, the GUI could display "1.234 $USD/h".
| + } | ||
| + | ||
| + result := make([]instances.InstanceType, len(resultUnique)) | ||
| + i := 0 |
axw
approved these changes
Nov 16, 2016
LGTM, but let's get feedback from @urosj before committing to this.
| + InstanceTypes []InstanceType `json:"instance-types,omitempty"` | ||
| + CostUnit string `json:"cost-unit,omitempty"` | ||
| + CostCurrency string `json:"cost-currency,omitempty"` | ||
| + Divisor uint64 `json:"divisor,omitempty"` |
| + CostCurrency string | ||
| + // Divisor indicates a number that must be applied to InstanceType.Cost to obtain | ||
| + // a number that is in CostUnit. | ||
| + Divisor uint64 |
axw
Nov 16, 2016
Member
I think we should document that setting (Cost)Divisor to zero will mean that the cost is treated as already being in specified unit, as the (cost-)divisor field will be omitted from the API response. i.e. the zero value is equivalent to 1.
| @@ -0,0 +1,60 @@ | ||
| +package gce |
| +import ( | ||
| + "strconv" | ||
| + | ||
| + "github.com/juju/errors" |
| + "github.com/juju/juju/constraints" | ||
| + "github.com/juju/juju/environs" | ||
| + "github.com/juju/juju/environs/instances" | ||
| + "github.com/juju/utils/arch" |
| @@ -0,0 +1,23 @@ | ||
| +package joyent |
| @@ -0,0 +1,15 @@ | ||
| +package lxd |
| +package lxd | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| @@ -0,0 +1,15 @@ | ||
| +package maas |
| +package maas | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| @@ -0,0 +1,16 @@ | ||
| +package manual |
| +package manual | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| @@ -0,0 +1,15 @@ | ||
| +package openstack |
| +package openstack | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| @@ -0,0 +1,15 @@ | ||
| +package rackspace |
| +package rackspace | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| + | ||
| +var _ environs.InstanceTypesFetcher = (*environ)(nil) | ||
| + | ||
| +func (e environ) InstanceTypes(c constraints.Value) (instances.InstanceTypesWithCostMetadata, error) { |
| @@ -0,0 +1,16 @@ | ||
| +package vsphere |
| +package vsphere | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| + costUnit := instanceTypes.CostUnit | ||
| + costCurrency := instanceTypes.CostCurrency | ||
| + divisor := instanceTypes.CostDivisor | ||
| + if err != nil { |
| + | ||
| + res[i] = params.InstanceTypesResult{ | ||
| + InstanceTypes: toParamsInstanceTypeResult(allTypes), | ||
| + CostUnit: costUnit, |
rogpeppe
Nov 16, 2016
Owner
Inline? i.e. use instanceTypes.CostUnit instead of using a temp variable.
| + | ||
| +// InstanceTypesFetcher is an interface that allows for instance information from | ||
| +// a provider to be obtained. | ||
| +type InstanceTypesFetcher interface { |
rogpeppe
Nov 16, 2016
Owner
Is it really worth having a separate interface type for this single method? Why not just make it part of Environ?
| + | ||
| +// ModelInstanceTypesConstraint contains a constraint applied when filtering instance types. | ||
| +type ModelInstanceTypesConstraint struct { | ||
| + // Value, if specified, contains the constraints to filter |
axw
Nov 20, 2016
Member
Can you please change the name to Constraints, and tag to "constraints", to match the field in CloudInstanceTypesConstraints? That way one is a superset of the other.
| + InstanceTypes []InstanceType `json:"instance-types,omitempty"` | ||
| + CostUnit string `json:"cost-unit,omitempty"` | ||
| + CostCurrency string `json:"cost-currency,omitempty"` | ||
| + // CostDivisor Will be present only when the Cost is not expressed in CostUnit. |
| + } | ||
| + result := make([]instances.InstanceType, len(types)) | ||
| + i := 0 | ||
| + for _, iType := range types { |
rogpeppe
Nov 16, 2016
•
Edited 1 time
-
rogpeppe
Nov 16, 2016
Owner
-
Nov 16, 2016
My preferred idiom for that is:
result := make([]instances.InstanceType, 0, len(types))
for _, iType := range types {
result = append(result, iType)
}
perrito666
Nov 16, 2016
Contributor
@rogpeppe cool, I guess I know why but could you expand on the underlying of that?
axw
Nov 20, 2016
Member
@perrito666 like rogpeppe, I do the same when going from map to slice. Only because it's more compact and reads better (to me).
| + }, | ||
| + } | ||
| + return &machineType, nil | ||
| + |
| +) | ||
| + | ||
| +var _ environs.InstanceTypesFetcher = (*environ)(nil) | ||
| +var virtType = "kvm" |
perrito666
Nov 23, 2016
Contributor
if I use a const I cannot pass its addres afterwards (instances.InstanceType.VirtType is a pointer)
| +package cloud | ||
| + | ||
| +import ( | ||
| + "github.com/juju/errors" |
| + "github.com/juju/juju/environs" | ||
| + "github.com/juju/juju/state" | ||
| + "github.com/juju/juju/state/stateenvirons" | ||
| + names "gopkg.in/juju/names.v2" |
| +// EnvironConfigGetter implements environs.EnvironConfigGetter | ||
| +// in terms of a *state.State. | ||
| +type cloudEnvironConfigGetter struct { | ||
| + *state.State |
axw
Nov 20, 2016
Member
You only need state.CloudAccessor and the GetModel function. Backend (apiserver/cloud/backend.go) already embeds state.CloudAccessor, so you just need to add the GetModel method, and you can use Backend instead of State.
You could otherwise just ignore the ModelTag, and use the existing Backend.ControllerModel method.
I think it might be simpler though if you just pass the CloudSpec (or perhaps Environ?) into the common.InstanceTypes function? i.e. have the facades pull apart the args, get the appropriate model, cloud, and region, and construct a CloudSpec from them. You may as well then get the Environ then too, and avoid passing the backend in just to get ModelConfig?
| + | ||
| +// InstanceTypes returns instance type information for the cloud and region | ||
| +// in which the current model is deployed. | ||
| +func (api *CloudAPI) InstanceTypes(cons params.CloudInstanceTypesConstraints) (params.InstanceTypesResults, error) { |
| + if cons.Constraints != nil { | ||
| + value = *cons.Constraints | ||
| + } | ||
| + backend := cloudEnvironConfigGetter{ |
axw
Nov 20, 2016
Member
Please parse the cons.CloudTag here, and check that it is the same as the controller model's cloud. We may relax this later, but it's better to be strict to avoid having to deal with sloppily written clients later.
| + itCons := common.NewInstanceTypeConstraints(backend, value, modelTag) | ||
| + it, err := common.InstanceTypes(itCons) | ||
| + if err != nil { | ||
| + return params.InstanceTypesResults{}, errors.Trace(err) |
axw
Nov 20, 2016
Member
An error due to one of them failing should not cause all of them to fail.
Either assume that common.InstanceTypes never fills in the Error field, and if it returns an error, have the caller (Cloud or MachineManager facade) fill it in; or change common.InstanceTypes to not have an error result.
| + environs.EnvironConfigGetter | ||
| + ModelTag() names.ModelTag | ||
| + Close() error | ||
| + GetModel(names.ModelTag) (*state.Model, error) |
axw
Nov 21, 2016
Member
Don't be so defeatist ;)
Look at ControllerModel in apiserver/cloud/backend.go for an example of how to do this. Anyway, I think this will be moot if you pass the Environ in?
perrito666
Nov 21, 2016
Contributor
I realized that seconds after writing that and fixed it with ashim, and am now writing the tests
| +// InstanceTypes returns a list of the available instance types in the provider according | ||
| +// to the passed constraints. | ||
| +func InstanceTypes(cons instanceTypeConstraints) (params.InstanceTypesResult, error) { | ||
| + m, err := cons.backend.GetModel(cons.modelTag) |
axw
Nov 20, 2016
Member
The fact that we're calling GetModel multiple times for what's always going to be the same model is a bit of a smell I think. As mentioned before, I think we should pass the CloudSpec (perhaps Environ?) in.
If this were split out, then it would be reasonable to fail the entire API call if we fail to get the CloudSpec in the case of the MachineManager facade. For the Cloud facade, it would be reasonable to fail the entire API call if we fail to get the Model; and then fail individual results if we can't get the CloudSpec (e.g. if an invalid CloudTag or region is specified).
| +// information. | ||
| +type InstanceTypeBackend interface { | ||
| + environs.EnvironConfigGetter | ||
| + Close() error |
axw
Nov 22, 2016
Member
Unused, drop me. I don't think it's appropriate to hand off ownership/responsibility to InstanceTypes anyway.
| + Config() (*config.Config, error) | ||
| +} | ||
| + | ||
| +// NewModelEnvironConfigGetter can create |
axw
Nov 22, 2016
Member
Can you please move this, modelEnvironConfigGetter, and InstanceTypeBackend to a separate file? It's only tangentially related to instance types. Also, InstanceTypeBackend should be renamed to be something to do with model/environ config.
It might be simpler just to have this as a struct of two functions? e.g.
type EnvironConfigGetterFuncs struct {
ModelConfigFunc func() (*config.Config, error)
CloudSpecFunc func(names.ModelTag) (environs.CloudSpec, error)
}
func (f EnvironConfigGetterFuncs) ModelConfig() (*config.Config, error) {
return f.ModelConfigFunc()
}
...Just a thought, feel free to ignore that.
The EnvironConfigGetter interface is kinda weird. Both methods should probably be taking a ModelTag...
| + | ||
| +// InstanceTypes returns a list of the available instance types in the provider according | ||
| +// to the passed constraints. | ||
| +func InstanceTypes(cons instanceTypeConstraints) (params.InstanceTypesResult, error) { |
| +// InstanceTypes returns instance type information for the cloud and region | ||
| +// in which the current model is deployed. | ||
| +func (mm *MachineManagerAPI) InstanceTypes(cons params.ModelInstanceTypesConstraints) (params.InstanceTypesResults, error) { | ||
| + st := mm.st.(*state.State) |
axw
Nov 22, 2016
Member
Please do the same for this method as you did for the other one, and use interfaces (stateInterface).
| jujutesting.Stub | ||
| results map[constraints.Value]instances.InstanceTypesWithCostMetadata | ||
| } | ||
| func (m *mockEnviron) InstanceTypes(c constraints.Value) (instances.InstanceTypesWithCostMetadata, error) { | ||
| + fmt.Println(c) |
| + return m, nil | ||
| +} | ||
| + | ||
| +func (s stateShim) Cloud(cloudName string) (cloud.Cloud, error) { |
axw
Nov 22, 2016
Member
these Cloud* methods aren't needed? stateShim embeds State, and State already has those methods
axw
approved these changes
Nov 23, 2016
LGTM once remaining comments are addressed (those from this review and ones from before, unless we've already talked about them.)
| + "github.com/juju/juju/state/stateenvirons" | ||
| +) | ||
| + | ||
| +// EnvironConfigGetter implements environs.EnvironConfigGetter |
| + return params.InstanceTypesResults{}, errors.Trace(err) | ||
| + } | ||
| + modelTag := c.ModelTag() | ||
| + m, err := api.backend.GetModel(modelTag) |
axw
Nov 23, 2016
Member
you end up with the same model as returned by ControllerModel
just do m, err := api.backend.ControllerModel()
then you can get rid of the ModelTag method from Backend
| + "github.com/juju/juju/environs/config" | ||
| +) | ||
| + | ||
| +// EnvironConfigGetterFuncs holds implements environs.EnvironConfigGetter |
| + | ||
| +// ModelInstanceTypesConstraint contains a constraint applied when filtering instance types. | ||
| +type ModelInstanceTypesConstraint struct { | ||
| + // Value, if specified, contains the constraints to filter |
axw
Nov 20, 2016
Member
Can you please change the name to Constraints, and tag to "constraints", to match the field in CloudInstanceTypesConstraints? That way one is a superset of the other.
frankban
approved these changes
Nov 23, 2016
Looks good and works well. Just a couple of minor issues. Did you talk about opening the Cloud call up to anonymous users?
| + if m.Cloud() != cloudTag.Id() { | ||
| + result[i] = params.InstanceTypesResult{Error: common.ServerError(errors.NotValidf("asking %s cloud information to %s cloud", cloudTag.Id(), m.Cloud()))} | ||
| + continue | ||
| + } |
frankban
Nov 23, 2016
Member
I'd check also that cons.CloudRegion is not empty here. otherwise I guess the subsequent error would be ugly and not really informative. This case should also be tested.
| + ) (environs.Environ, error) { | ||
| + return &env, nil | ||
| + } | ||
| + r, err := machinemanager.InstanceTypes(&api, fakeEnvironGet, cons) |
frankban
Nov 23, 2016
Member
Should we also test the case no constraints are passed? In this case I'd expect an empty result.
| @@ -75,6 +75,10 @@ func instanceTypes(api *CloudAPI, | ||
| result[i] = params.InstanceTypesResult{Error: common.ServerError(errors.NotValidf("asking %s cloud information to %s cloud", cloudTag.Id(), m.Cloud()))} | ||
| continue | ||
| } | ||
| + if cons.CloudRegion == "" { |
axw
Nov 23, 2016
Member
Not all clouds have regions. I'm not sure if the validation belongs here or not, but if we do validate, we need to check that the cloud supports regions before requiring that it's empty.
Yes we have talked about it, but as we agreed we'll defer this bit. It's not feasible to make that change for 2.1. |
|
!!try!! |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
perrito666 commentedNov 14, 2016
Added InstanceTypes API endpoint that allows to learn
the available instance types in the model region/zone
to make better deployment decisions.
Tests are missing for 3 out of 4 providers implemented but I am keen to get the rest of the code reviewed as this needs to be merged soon.
This follows: https://docs.google.com/document/d/1m6iNWOMYyGwHbDbl8-u_VSZozeZQklxUOgM5t_0wbMQ/edit#heading=h.kc1rayquamxb
###QA
Run Tests.
Create an API client call that points to the controller and Perform the call for the different providers to obtain lists of available types. (This is intended to be used by the GUI)