Support constraints vocabs

Support constraints vocabs

Add support to constraints validation for
vocabs for each attribute. eg arch and
instance-type can only have well defined
values for each provider. The vocab check
is part of constraints validation.

https://code.launchpad.net/~wallyworld/juju-core/constraints-vocab/+merge/216968

(do not edit description out of merge proposal)

[wallyworld, 2014-04-24 02:27:16]

Please take a look.

[jameinel, 2014-04-24 04:58:16]

I had a couple comments about the RegisterVocabulary function, but otherwise I
think this LGTM.

I'm slightly concerned that this is more round-trips to the environ API when
provisioning machines, but I think they are worthy ones and hopefully it is O(1)
when you are using -n25

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

https://codereview.appspot.com/96730043/diff/1/constraints/constraints.go#new...
constraints/constraints.go:226: if attrTag == value {
If values is a map, why would we iterate it in order to do the comparison?

Shouldn't we just do:

_, ok := values[attrTag]
return ok

https://codereview.appspot.com/96730043/diff/1/constraints/validation.go
File constraints/validation.go (right):

https://codereview.appspot.com/96730043/diff/1/constraints/validation.go#newc...
constraints/validation.go:87: valuesSlice = []interface{}{v}
This seems really too loose that we allow:

RegisterVocabulary("attr", "foo")
as well as
RegisterVocabulary("attr", []string{"foo"})

Shouldn't we just error if TypeOf(v) != Slice or Array?
Actually, a better syntax might be

RegisterVocabulary(attributeName string, allowedValues ...interface{})

That lets you do:
RegisterVocabulary("attr", "foo")
RegisterVocabulary("attr", "foo", "bar")
and
attrs = []string{"foo", "bar"} // read from somewhere else
RegisterVocabulary("attr", attrs...)

I think that gives us the best flexibility and it still ensures that
allowedValues is always a slice.
Actually, I think it eliminates your need for all the reflection code
completely, since all that is doing is turning a typed slice into a
[]interface{}, and I'm pretty sure the ... notation is going to be doing the
work for us.

I might be wrong, but if it works it would save a fair amount of code.

https://codereview.appspot.com/96730043/diff/1/constraints/validation.go#newc...
constraints/validation.go:165: return int64(reflect.ValueOf(v).Uint())
This is a little bit dangerous for Uint64. Can you actually do a range check so
we don't overflow? I realize there is a problem that you don't have a way to
signal an error, but even a panic() is probably better than silently turning
2**64-1 (0xFFFFFFFFFFFFFFFF) into -1.

Actually, we could potentially cast it into a Float64 on overflow, though that
won't let you do exact comparison (I think).

[wallyworld, 2014-04-24 07:16:12]

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

https://codereview.appspot.com/96730043/diff/1/constraints/constraints.go#new...
constraints/constraints.go:226: if attrTag == value {
On 2014/04/24 04:58:16, jameinel wrote:
> If values is a map, why would we iterate it in order to do the comparison?
> 
> Shouldn't we just do:
> 
> _, ok := values[attrTag]
> return ok

bah, it used to be a slice. bad refactoring. fixed.

https://codereview.appspot.com/96730043/diff/1/constraints/validation.go
File constraints/validation.go (right):

https://codereview.appspot.com/96730043/diff/1/constraints/validation.go#newc...
constraints/validation.go:87: valuesSlice = []interface{}{v}
On 2014/04/24 04:58:16, jameinel wrote:
> This seems really too loose that we allow:
> 
> RegisterVocabulary("attr", "foo")
> as well as
> RegisterVocabulary("attr", []string{"foo"})
> 
> Shouldn't we just error if TypeOf(v) != Slice or Array?
> Actually, a better syntax might be
> 
> RegisterVocabulary(attributeName string, allowedValues ...interface{})
> 
> That lets you do:
> RegisterVocabulary("attr", "foo")
> RegisterVocabulary("attr", "foo", "bar")
> and
> attrs = []string{"foo", "bar"} // read from somewhere else
> RegisterVocabulary("attr", attrs...)

Sadly this doesn't work because Go still treats on as []interface{} and the
other as []string, and so it won't compile:

eg

validator.RegisterVocabulary(constraints.Arch, supportedArches...)

cannot use supportedArches (type []string) as type []interface {} in function
argument

So I'll raise an error if the values param is not a slice/array.

> 
> I think that gives us the best flexibility and it still ensures that
> allowedValues is always a slice.
> Actually, I think it eliminates your need for all the reflection code
> completely, since all that is doing is turning a typed slice into a
> []interface{}, and I'm pretty sure the ... notation is going to be doing the
> work for us.
> 

I still need some reflection code to handle that the constraints values
themselves might be a slice eg mass tags. But I've reworked it on my local copy
to be cleaner.

https://codereview.appspot.com/96730043/diff/1/constraints/validation.go#newc...
constraints/validation.go:165: return int64(reflect.ValueOf(v).Uint())
On 2014/04/24 04:58:16, jameinel wrote:
> This is a little bit dangerous for Uint64. Can you actually do a range check
so
> we don't overflow? I realize there is a problem that you don't have a way to
> signal an error, but even a panic() is probably better than silently turning
> 2**64-1 (0xFFFFFFFFFFFFFFFF) into -1.
> 
> Actually, we could potentially cast it into a Float64 on overflow, though that
> won't let you do exact comparison (I think).

I thought about it but didn't think any of our constraints values could or would
be that big. Do we really think we'll encounter an overflow setting cpu-power or
cpu-cores or mem in MB etc?