state: trivial Constraints

state: trivial Constraints

This does not necessarily reflect the current consensus re what we should
provide, but at least it's something concrete to argue about.

https://code.launchpad.net/~fwereade/juju-core/trivial-initial-constraints/+merge/148248

(do not edit description out of merge proposal)

[fwereade, 2013-02-13 17:35:25]

Please take a look.

[dave_cheney.net, 2013-02-14 00:21:05]

On 2013/02/13 17:35:25, fwereade wrote:
> Please take a look.

To the best of my ability, I oppose the inclusion of cpu-power as a constraint.
It is, IMO, meaningless in a world where you share CPU time with other VMs.

[dave_cheney.net, 2013-02-14 00:25:23]

LGTM. I have some concerns about the choice pointer values for the Constraint
struct.

https://codereview.appspot.com/7324049/diff/1/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode14
state/constraints.go:14: CpuCores *int
Could this be a uint, it makes it (slightly) harder to work with, but as this
struct already admits a nil number of cores, a negative number of cores is not
sensible, even as a sentinel for unknown.

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode16
state/constraints.go:16: Mem      *float64
Mem should be an uint64 (or an int64)

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode23
state/constraints.go:23: strs = append(strs, "cpu-cores="+intStr(*c.CpuCores))
Will panic if CpuCores is nil

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode26
state/constraints.go:26: strs = append(strs, "cpu-power="+floatStr(*c.CpuPower))
same

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode29
state/constraints.go:29: s := floatStr(*c.Mem)
same

[TheMue, 2013-02-14 07:32:04]

LGTM, only one question.

https://codereview.appspot.com/7324049/diff/1/cmd/juju/constraints.go
File cmd/juju/constraints.go (right):

https://codereview.appspot.com/7324049/diff/1/cmd/juju/constraints.go#newcode45
cmd/juju/constraints.go:45: func (v *constraintsValue) setCpuCores(str string)
error {
Is an empty string or the value 0 intended to say "I don't care."? As well es
for cpu-power and mem?

[rog, 2013-02-14 07:40:09]

a good start! a few minor suggestions below.

https://codereview.appspot.com/7324049/diff/1/cmd/juju/constraints.go
File cmd/juju/constraints.go (right):

https://codereview.appspot.com/7324049/diff/1/cmd/juju/constraints.go#newcode20
cmd/juju/constraints.go:20: args := strings.Split(raw, " ")
i'd put this code into state and name it ParseConstraints

https://codereview.appspot.com/7324049/diff/1/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode14
state/constraints.go:14: CpuCores *int
On 2013/02/14 00:25:23, dfc wrote:
> Could this be a uint, it makes it (slightly) harder to work with, but as this
> struct already admits a nil number of cores, a negative number of cores is not
> sensible, even as a sentinel for unknown. 

i don't mind int actually - if the parsing code doesn't allow an int then you're
not going to see negative numbers.

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode15
state/constraints.go:15: CpuPower *float64
i think we could choose a measure that made sense as an integer here. i'd prefer
to avoid floats if possible (it can make round-tripping awkward, and we're going
to be doing quite a bit of that).

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode16
state/constraints.go:16: Mem      *float64
On 2013/02/14 00:25:23, dfc wrote:
> Mem should be an uint64 (or an int64)

+1. int64 as MB would be fine i think.

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode23
state/constraints.go:23: strs = append(strs, "cpu-cores="+intStr(*c.CpuCores))
On 2013/02/14 00:25:23, dfc wrote:
> Will panic if CpuCores is nil

only if that's changed since the previous line :-)

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode26
state/constraints.go:26: strs = append(strs, "cpu-power="+floatStr(*c.CpuPower))
On 2013/02/14 00:25:23, dfc wrote:
> same

ditto

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode29
state/constraints.go:29: s := floatStr(*c.Mem)
On 2013/02/14 00:25:23, dfc wrote:
> same

ditto

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode39
state/constraints.go:39: if i == 0 {
i <= 0 ?

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode46
state/constraints.go:46: if f == 0 {
i <= 0 ?

https://codereview.appspot.com/7324049/diff/1/state/constraints_test.go
File state/constraints_test.go (right):

https://codereview.appspot.com/7324049/diff/1/state/constraints_test.go#newco...
state/constraints_test.go:13: cpuCores interface{}
why aren't these the correct types for the fields?

[dave_cheney.net, 2013-02-14 08:13:29]

https://codereview.appspot.com/7324049/diff/1/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode23
state/constraints.go:23: strs = append(strs, "cpu-cores="+intStr(*c.CpuCores))
My apologies, I must not have had my caffeine

[fwereade, 2013-02-15 10:51:20]

Please take a look.

https://codereview.appspot.com/7324049/diff/1/cmd/juju/constraints.go
File cmd/juju/constraints.go (right):

https://codereview.appspot.com/7324049/diff/1/cmd/juju/constraints.go#newcode20
cmd/juju/constraints.go:20: args := strings.Split(raw, " ")
On 2013/02/14 07:40:09, rog wrote:
> i'd put this code into state and name it ParseConstraints

I'm not sure it's really state's job, and it's very specific to command-line
usage. I'm not really wild about changing conventions without good reason...

https://codereview.appspot.com/7324049/diff/1/cmd/juju/constraints.go#newcode45
cmd/juju/constraints.go:45: func (v *constraintsValue) setCpuCores(str string)
error {
On 2013/02/14 07:32:04, TheMue wrote:
> Is an empty string or the value 0 intended to say "I don't care."? As well es
> for cpu-power and mem?

Yes.

https://codereview.appspot.com/7324049/diff/1/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode14
state/constraints.go:14: CpuCores *int
On 2013/02/14 00:25:23, dfc wrote:
> Could this be a uint, it makes it (slightly) harder to work with, but as this
> struct already admits a nil number of cores, a negative number of cores is not
> sensible, even as a sentinel for unknown. 

SGTM, thanks.

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode15
state/constraints.go:15: CpuPower *float64
On 2013/02/14 07:40:09, rog wrote:
> i think we could choose a measure that made sense as an integer here. i'd
prefer
> to avoid floats if possible (it can make round-tripping awkward, and we're
going
> to be doing quite a bit of that).

Done (although I don't expect us to ever actually round-trip it, except for test
convenience..?)

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode23
state/constraints.go:23: strs = append(strs, "cpu-cores="+intStr(*c.CpuCores))
On 2013/02/14 00:25:23, dfc wrote:
> Will panic if CpuCores is nil

Guarded by `if c.CpuCores != nil`, surely?

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode26
state/constraints.go:26: strs = append(strs, "cpu-power="+floatStr(*c.CpuPower))
On 2013/02/14 00:25:23, dfc wrote:
> same

same

https://codereview.appspot.com/7324049/diff/1/state/constraints.go#newcode29
state/constraints.go:29: s := floatStr(*c.Mem)
On 2013/02/14 00:25:23, dfc wrote:
> same

same

https://codereview.appspot.com/7324049/diff/1/state/constraints_test.go
File state/constraints_test.go (right):

https://codereview.appspot.com/7324049/diff/1/state/constraints_test.go#newco...
state/constraints_test.go:13: cpuCores interface{}
On 2013/02/14 07:40:09, rog wrote:
> why aren't these the correct types for the fields?

Because I want to express nil sometimes, and I can't do `&123` in the cases
where I don't. intp/floatp funcs would do the trick though, as discussed online.

[jameinel, 2013-02-15 12:59:10]

LGTM

[dimitern, 2013-02-15 16:30:37]

LGTM, with a few comments.

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints.go
File cmd/juju/constraints.go (right):

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints.go#newc...
cmd/juju/constraints.go:21: args := strings.Split(raw, " ")
How about stripping all spaces except the ones between constrains, including
possible spaces around "=". Then the parsing can be simplified and a little
laxer on there user?
We can have "  something  =  val   thing= val2 ", and end up with "something=val
thing=val2" and then parse it.

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints.go#newc...
cmd/juju/constraints.go:96: var mbSuffixes = map[string]float64{
Aliasing M to MB, etc. ?

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints_test.go
File cmd/juju/constraints_test.go (right):

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints_test.go...
cmd/juju/constraints_test.go:12: var constraintsValueTests = []struct {
Nice tests!

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints_test.go...
cmd/juju/constraints_test.go:104: summary: "set mem without suffix",
Is this "bytes"?

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints_test.go...
cmd/juju/constraints_test.go:128: args:    []string{"mem=32Y"},
Arguably, yoctobytes are a valid figure, so maybe test 32X or something?

https://codereview.appspot.com/7324049/diff/9001/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7324049/diff/9001/state/constraints.go#newcode20
state/constraints.go:20: // equivalent to 1 Amazon ECU (or, roughly, a single
2007-era Xeon).
Will there be a formula in juju to convert cpu-power to cpu-cores?

[fwereade, 2013-02-17 09:31:44]

*** Submitted:

state: trivial Constraints

This does not necessarily reflect the current consensus re what we should
provide, but at least it's something concrete to argue about.

R=dfc, TheMue, rog, jameinel, dimitern
CC=
https://codereview.appspot.com/7324049

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints.go
File cmd/juju/constraints.go (right):

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints.go#newc...
cmd/juju/constraints.go:21: args := strings.Split(raw, " ")
On 2013/02/15 16:30:37, dimitern wrote:
> How about stripping all spaces except the ones between constrains, including
> possible spaces around "=". Then the parsing can be simplified and a little
> laxer on there user?
> We can have "  something  =  val   thing= val2 ", and end up with
"something=val
> thing=val2" and then parse it.

Feels like it makes it harder to me (especially considering "mem=" to clear
constraints).

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints.go#newc...
cmd/juju/constraints.go:96: var mbSuffixes = map[string]float64{
On 2013/02/15 16:30:37, dimitern wrote:
> Aliasing M to MB, etc. ?

That's what we did in python, and nobody has yet complained. Shouldn't be hard
to add them later if it turns out somebody's really bothered by this ;p.

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints_test.go
File cmd/juju/constraints_test.go (right):

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints_test.go...
cmd/juju/constraints_test.go:12: var constraintsValueTests = []struct {
On 2013/02/15 16:30:37, dimitern wrote:
> Nice tests!

Thanks :D

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints_test.go...
cmd/juju/constraints_test.go:104: summary: "set mem without suffix",
On 2013/02/15 16:30:37, dimitern wrote:
> Is this "bytes"?

It's always measured in MB.

https://codereview.appspot.com/7324049/diff/9001/cmd/juju/constraints_test.go...
cmd/juju/constraints_test.go:128: args:    []string{"mem=32Y"},
On 2013/02/15 16:30:37, dimitern wrote:
> Arguably, yoctobytes are a valid figure, so maybe test 32X or something?

Heh, I thought it was "yotta". Anyway, it's intended as an explicit check that
I'm not *that* crazy... petabytes is bad enough ;p.

https://codereview.appspot.com/7324049/diff/9001/state/constraints.go
File state/constraints.go (right):

https://codereview.appspot.com/7324049/diff/9001/state/constraints.go#newcode20
state/constraints.go:20: // equivalent to 1 Amazon ECU (or, roughly, a single
2007-era Xeon).
On 2013/02/15 16:30:37, dimitern wrote:
> Will there be a formula in juju to convert cpu-power to cpu-cores?

I don't think there's any intrinsic correlation. Openstack will be free to
ignore cpu-power constraints (but may want to handle them for certain
clouds...).