Annotation documents and Environment Entity

Annotation documents and Environment Entity

Annotations were moved to separate documents in order to prevent the act of
setting an annotation from affecting watchers on the entities.  Annotations are
stored, one document per entity, in a separate collection which are removed when
the entities are removed.  In addition, an environment-level Entity was created
in order to allow storing annotations on the environment, and potentially for
expansion in the future.

https://code.launchpad.net/~frankban/juju-core/ann-doc/+merge/152280

Requires: https://code.launchpad.net/~frankban/juju-core/bug-1147138-annotations-api/+merge/152037

(do not edit description out of merge proposal)

[frankban, 2013-03-07 21:18:09]

Please take a look.

[matthew.scott, 2013-03-07 21:21:30]

Note that this diff contains all of the API changes as well, not just the
documents.  If needed, we can repropose as a separate branch once the API branch
lands, or we can land them together.

[dimitern, 2013-03-07 21:44:52]

LGTM, but I have one question.

https://codereview.appspot.com/7598043/diff/1/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7598043/diff/1/state/annotator.go#newcode90
state/annotator.go:90: func AnnotationRemoveOps(st *State, id string) txn.Op {
should this be exported?

[matthew.scott, 2013-03-07 21:49:45]

Thanks for the review.

https://codereview.appspot.com/7598043/diff/1/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7598043/diff/1/state/annotator.go#newcode90
state/annotator.go:90: func AnnotationRemoveOps(st *State, id string) txn.Op {
On 2013/03/07 21:44:52, dimitern wrote:
> should this be exported?

Done.

[frankban, 2013-03-07 21:51:14]

Please take a look.

[frankban, 2013-03-07 22:15:57]

Please take a look.

[frankban, 2013-03-08 22:04:08]

Please take a look.

[fwereade, 2013-03-09 15:13:36]

Sorry, but NOT LGTM until someone can explain what on earth has happened to the
concept of an Entity.

Once it just meant machine/service/unit/relation. Then users were added. Then
annotations were added. Now the environment has been added... and each of these
additions has resulted in more special cases.

At this point, it's completely incoherent. At an *absolute* minimum, we have to
separate the concept of "state entity" from that of "something that can log in".

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go
File state/annotator.go (left):

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#oldcode11
state/annotator.go:11: annotations *map[string]string
Why are we dropping the local data storage? It may be a reasonable thing to do
but it's pretty inconsistent and deserves a comment.

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode11
state/annotator.go:11: type annDoc struct {
Please call this annotatorDoc.

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode12
state/annotator.go:12: EntityName  string `bson:"_id"`
I don't think it's a good idea to invent a new id scheme just for these
documents. If there's a problem with the usual globalKey-style lookups we should
discuss and resolve it.

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode61
state/annotator.go:61: return fmt.Errorf("cannot set annotation %q = %q on %s:
%v", key, value, id, onAbort(err, errNotAlive))
This is wrong. We can abort because the doc already exists, but we always report
that as a "not alive" error. Why aren't we retrying on state change?

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode72
state/annotator.go:72: return make(map[string]string), nil
What does a non-nil dict give us here? Are we expecting people to modify it?

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode75
state/annotator.go:75: return make(map[string]string), err
return nil, err

...surely?

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode86
state/annotator.go:86: return ann[key], nil
So, a missing annotation is never an error? The reasoning should probably be
explained in the doc comment.

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode90
state/annotator.go:90: func annotationRemoveOps(st *State, id string) txn.Op {
Nice, thank you.

https://codereview.appspot.com/7598043/diff/18001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/7598043/diff/18001/state/environ.go#newcode12
state/environ.go:12: func (st *State) GetEnvironment() *Environment {
Why not Environment() for consistency?

https://codereview.appspot.com/7598043/diff/18001/state/environ.go#newcode24
state/environ.go:24: func (e Environment) EntityName() string {
This is probably one for roger... please explain the concept of an Entity, and
why you consider an environment to be one. It's been twisted way too far and no
longer represents a coherent concept AFAICT.

https://codereview.appspot.com/7598043/diff/18001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/7598043/diff/18001/state/machine_test.go#newco...
state/machine_test.go:629: s.machine.Remove()
Check errors.

https://codereview.appspot.com/7598043/diff/18001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7598043/diff/18001/state/service.go#newcode197
state/service.go:197: return append(ops, annotationRemoveOps(s.st,
s.EntityName()))
When did service get an EntityName?

https://codereview.appspot.com/7598043/diff/18001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/7598043/diff/18001/state/service_test.go#newco...
state/service_test.go:1103: s.mysql.Destroy()
Errors.

https://codereview.appspot.com/7598043/diff/18001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/7598043/diff/18001/state/unit_test.go#newcode709
state/unit_test.go:709: s.unit.Remove()
Errors.

[fwereade, 2013-03-09 15:19:31]

On 2013/03/09 15:13:36, fwereade wrote:
> At this point, it's completely incoherent. At an *absolute* minimum, we have
to
> separate the concept of "state entity" from that of "something that can log
in".

And "EntityName" is not sane. There are entities, and they have names, but those
names are not the same as their EntityName()s? And environments have names, but
have EntityNames that follow a different scheme to every other entity and
doesn't include the environment's actual name? I was prepared to let sleeping
dogs lie, so long as the situation was not actively decaying, but ISTM that it
definitely is decaying and that this needs to be stopped.

[frankban, 2013-03-11 16:02:26]

On 2013/03/09 15:13:36, fwereade wrote:
> Sorry, but NOT LGTM until someone can explain what on earth has happened to
the
> concept of an Entity.
> 
> Once it just meant machine/service/unit/relation. Then users were added. Then
> annotations were added. Now the environment has been added... and each of
these
> additions has resulted in more special cases.
> 
> At this point, it's completely incoherent. At an *absolute* minimum, we have
to
> separate the concept of "state entity" from that of "something that can log
in".

We extended the concept of an entity as suggested. We can restore the old
AuthEntity and work on a new concept, e.g. AnnotationsEntity. 
We assume that the entity name can no longer be used as a primary key for
the annotations document. IWUC, we should use the GlobalKey of objects as
already
done in settings and constraints.
Please consider this proposal and confirm it makes sense:
- restore the AuthEntity;
- create a new AnnotationsEntity interface, implemented by units, services,
machines and environments;
- create a new AnnotationsEntity function taking some name, maybe the entity
name as required by the GUI, and returning an AnnotationsEntity instance.
- switch to using the GlobalKey() as _id for the annotations mongodb doc;
- query the database using the GlobalKey() of the entity.

Questions:
Currently the GUI does not have any knowledge of what's the name of the
environment.
This means that all consumers (e.g. Landscape) of annotations must retrieve the
environment name before setting or getting annotations on the environment.
We could extend the EnvironmentInfo API call so that the environment name
(possibly obtained for settings) is included in the response.
At that point, the EntityName for the environment can be something like
"environment-" + [env name].

What do you think?

> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go
> File state/annotator.go (left):
> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#oldcode11
> state/annotator.go:11: annotations *map[string]string
> Why are we dropping the local data storage? It may be a reasonable thing to do
> but it's pretty inconsistent and deserves a comment.

We no longer make use of the local data storage, because we don't store
annotations
inside the entity's documents anymore and we don't require annotations to be
accessed directly. If, for consistency or for other reasons, you want us to
maintain a local data storage, we can just restore it.
For now, added a comment.

> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go
> File state/annotator.go (right):
> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode11
> state/annotator.go:11: type annDoc struct {
> Please call this annotatorDoc.

Done.

> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode12
> state/annotator.go:12: EntityName  string `bson:"_id"`
> I don't think it's a good idea to invent a new id scheme just for these
> documents. If there's a problem with the usual globalKey-style lookups we
should
> discuss and resolve it.

See our proposal above.

> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode61
> state/annotator.go:61: return fmt.Errorf("cannot set annotation %q = %q on %s:
> %v", key, value, id, onAbort(err, errNotAlive))
> This is wrong. We can abort because the doc already exists, but we always
report
> that as a "not alive" error. Why aren't we retrying on state change?

Done. Now we just report the original error. Could you please expand on state
change?

> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode72
> state/annotator.go:72: return make(map[string]string), nil
> What does a non-nil dict give us here? Are we expecting people to modify it?

No, you are right. Fixed.

> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode75
> state/annotator.go:75: return make(map[string]string), err
> return nil, err
> 
> ...surely?

Done.

> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode86
> state/annotator.go:86: return ann[key], nil
> So, a missing annotation is never an error? The reasoning should probably be
> explained in the doc comment.

Added a comment to the method.

> 
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode90
> state/annotator.go:90: func annotationRemoveOps(st *State, id string) txn.Op {
> Nice, thank you.
> 
> https://codereview.appspot.com/7598043/diff/18001/state/environ.go
> File state/environ.go (right):
> 
> https://codereview.appspot.com/7598043/diff/18001/state/environ.go#newcode12
> state/environ.go:12: func (st *State) GetEnvironment() *Environment {
> Why not Environment() for consistency?

Done.

> https://codereview.appspot.com/7598043/diff/18001/state/environ.go#newcode24
> state/environ.go:24: func (e Environment) EntityName() string {
> This is probably one for roger... please explain the concept of an Entity, and
> why you consider an environment to be one. It's been twisted way too far and
no
> longer represents a coherent concept AFAICT.

See our proposal above.

> https://codereview.appspot.com/7598043/diff/18001/state/machine_test.go
> File state/machine_test.go (right):
> 
>
https://codereview.appspot.com/7598043/diff/18001/state/machine_test.go#newco...
> state/machine_test.go:629: s.machine.Remove()
> Check errors.

Done. Also added error checks in the similar test for units and services.

> https://codereview.appspot.com/7598043/diff/18001/state/service.go
> File state/service.go (right):
> 
> https://codereview.appspot.com/7598043/diff/18001/state/service.go#newcode197
> state/service.go:197: return append(ops, annotationRemoveOps(s.st,
> s.EntityName()))
> When did service get an EntityName?

We need an entity name for the service, see proposal above.
Another idea could be adding something like AnnotationsName to each
AnnotationsEntity/AnnotableObject/the name you like.

> 
> https://codereview.appspot.com/7598043/diff/18001/state/service_test.go
> File state/service_test.go (right):
> 
>
https://codereview.appspot.com/7598043/diff/18001/state/service_test.go#newco...
> state/service_test.go:1103: s.mysql.Destroy()
> Errors.

Done.

> 
> https://codereview.appspot.com/7598043/diff/18001/state/unit_test.go
> File state/unit_test.go (right):
> 
>
https://codereview.appspot.com/7598043/diff/18001/state/unit_test.go#newcode709
> state/unit_test.go:709: s.unit.Remove()
> Errors.

Done.

[frankban, 2013-03-11 16:04:22]

Please take a look.

[fwereade, 2013-03-13 10:38:51]

On 2013/03/11 16:02:26, frankban wrote:
> We extended the concept of an entity as suggested. We can restore the old
> AuthEntity and work on a new concept, e.g. AnnotationsEntity. 
> We assume that the entity name can no longer be used as a primary key for
> the annotations document. IWUC, we should use the GlobalKey of objects as
> already
> done in settings and constraints.
> Please consider this proposal and confirm it makes sense:
> - restore the AuthEntity;

+1

> - create a new AnnotationsEntity interface, implemented by units, services,
> machines and environments;

Should this include relations? If it should, I'm fine calling that one Entity.

> - create a new AnnotationsEntity function taking some name, maybe the entity
> name as required by the GUI, and returning an AnnotationsEntity instance.

+1

> - switch to using the GlobalKey() as _id for the annotations mongodb doc;

Note that you might not even need to explicitly mention the _id in the document;
might (or might not) simplify some code.

> - query the database using the GlobalKey() of the entity.

+1 (note that not everything has a globalKey at the moment, but the convention
is hopefully clear by inspection).

> Questions:
> Currently the GUI does not have any knowledge of what's the name of the
> environment.
> This means that all consumers (e.g. Landscape) of annotations must retrieve
the
> environment name before setting or getting annotations on the environment.
> We could extend the EnvironmentInfo API call so that the environment name
> (possibly obtained for settings) is included in the response.
> At that point, the EntityName for the environment can be something like
> "environment-" + [env name].
> 
> What do you think?

That SGTM.

> > 
> > https://codereview.appspot.com/7598043/diff/18001/state/annotator.go
> > File state/annotator.go (left):
> > 
> >
https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#oldcode11
> > state/annotator.go:11: annotations *map[string]string
> > Why are we dropping the local data storage? It may be a reasonable thing to
do
> > but it's pretty inconsistent and deserves a comment.
> 
> We no longer make use of the local data storage, because we don't store
> annotations
> inside the entity's documents anymore and we don't require annotations to be
> accessed directly. If, for consistency or for other reasons, you want us to
> maintain a local data storage, we can just restore it.
> For now, added a comment.

I'm not really bothered either way -- just curious, and desirous of
documentation :).

> >
https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode61
> > state/annotator.go:61: return fmt.Errorf("cannot set annotation %q = %q on
%s:
> > %v", key, value, id, onAbort(err, errNotAlive))
> > This is wrong. We can abort because the doc already exists, but we always
> report
> > that as a "not alive" error. Why aren't we retrying on state change?
> 
> Done. Now we just report the original error. Could you please expand on state
> change?

This probably demands a g+ chat (and also probably requires that I write some
developer docs describing what I have hitherto figured out about effective use
of mgo/txn.)

> > https://codereview.appspot.com/7598043/diff/18001/state/service.go
> > File state/service.go (right):
> > 
> >
https://codereview.appspot.com/7598043/diff/18001/state/service.go#newcode197
> > state/service.go:197: return append(ops, annotationRemoveOps(s.st,
> > s.EntityName()))
> > When did service get an EntityName?
> 
> We need an entity name for the service, see proposal above.
> Another idea could be adding something like AnnotationsName to each
> AnnotationsEntity/AnnotableObject/the name you like.

I still have some issues with EntityName, as described previously... I don't
love AnnotationsName either, but it's definitely clearer, so maybe go with that
for now.

[fwereade, 2013-03-13 10:41:42]

Couple more notes

https://codereview.appspot.com/7598043/diff/31001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7598043/diff/31001/state/annotator.go#newcode90
state/annotator.go:90: // Returning an empty string if the requested annotation
is not found.
Sorry, I don't think I was clear-- I was suggesting that the behaviour is
important enough to be mentioned in the doc comment above. I don't think we need
to document what `return ann[key], nil` means inside the method.

https://codereview.appspot.com/7598043/diff/31001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/7598043/diff/31001/state/environ.go#newcode12
state/environ.go:12: func (st *State) GetEnvironment() *Environment {
This doesn't seem to have been changed?

https://codereview.appspot.com/7598043/diff/31001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/7598043/diff/31001/state/machine_test.go#newco...
state/machine_test.go:627: s.machine.SetAnnotation("mykey", "myvalue")
Error here as well.

https://codereview.appspot.com/7598043/diff/31001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/7598043/diff/31001/state/service_test.go#newco...
state/service_test.go:1102: s.mysql.SetAnnotation("mykey", "myvalue")
Error check here.

https://codereview.appspot.com/7598043/diff/31001/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/7598043/diff/31001/state/unit_test.go#newcode707
state/unit_test.go:707: s.unit.SetAnnotation("mykey", "myvalue")
And here

[rog, 2013-03-13 11:02:58]

On 2013/03/13 10:38:51, fwereade wrote:
> On 2013/03/11 16:02:26, frankban wrote:
> > We extended the concept of an entity as suggested. We can restore the old
> > AuthEntity and work on a new concept, e.g. AnnotationsEntity. 
> > We assume that the entity name can no longer be used as a primary key for
> > the annotations document. IWUC, we should use the GlobalKey of objects as
> > already
> > done in settings and constraints.
> > Please consider this proposal and confirm it makes sense:
> > - restore the AuthEntity;
> 
> +1
> 
> > - create a new AnnotationsEntity interface, implemented by units, services,
> > machines and environments;
> 
> Should this include relations? If it should, I'm fine calling that one Entity.
> 
> > - create a new AnnotationsEntity function taking some name, maybe the entity
> > name as required by the GUI, and returning an AnnotationsEntity instance.
> 
> +1
> 
> > - switch to using the GlobalKey() as _id for the annotations mongodb doc;
> 
> Note that you might not even need to explicitly mention the _id in the
document;
> might (or might not) simplify some code.
> 
> > - query the database using the GlobalKey() of the entity.
> 
> +1 (note that not everything has a globalKey at the moment, but the convention
> is hopefully clear by inspection).
> 
> > Questions:
> > Currently the GUI does not have any knowledge of what's the name of the
> > environment.
> > This means that all consumers (e.g. Landscape) of annotations must retrieve
> the
> > environment name before setting or getting annotations on the environment.
> > We could extend the EnvironmentInfo API call so that the environment name
> > (possibly obtained for settings) is included in the response.
> > At that point, the EntityName for the environment can be something like
> > "environment-" + [env name].
> > 
> > What do you think?
> 
> That SGTM.
> 
> > > 
> > > https://codereview.appspot.com/7598043/diff/18001/state/annotator.go
> > > File state/annotator.go (left):
> > > 
> > >
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#oldcode11
> > > state/annotator.go:11: annotations *map[string]string
> > > Why are we dropping the local data storage? It may be a reasonable thing
to
> do
> > > but it's pretty inconsistent and deserves a comment.
> > 
> > We no longer make use of the local data storage, because we don't store
> > annotations
> > inside the entity's documents anymore and we don't require annotations to be
> > accessed directly. If, for consistency or for other reasons, you want us to
> > maintain a local data storage, we can just restore it.
> > For now, added a comment.
> 
> I'm not really bothered either way -- just curious, and desirous of
> documentation :).
> 
> > >
> https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#newcode61
> > > state/annotator.go:61: return fmt.Errorf("cannot set annotation %q = %q on
> %s:
> > > %v", key, value, id, onAbort(err, errNotAlive))
> > > This is wrong. We can abort because the doc already exists, but we always
> > report
> > > that as a "not alive" error. Why aren't we retrying on state change?
> > 
> > Done. Now we just report the original error. Could you please expand on
state
> > change?
> 
> This probably demands a g+ chat (and also probably requires that I write some
> developer docs describing what I have hitherto figured out about effective use
> of mgo/txn.)
> 
> > > https://codereview.appspot.com/7598043/diff/18001/state/service.go
> > > File state/service.go (right):
> > > 
> > >
> https://codereview.appspot.com/7598043/diff/18001/state/service.go#newcode197
> > > state/service.go:197: return append(ops, annotationRemoveOps(s.st,
> > > s.EntityName()))
> > > When did service get an EntityName?
> > 
> > We need an entity name for the service, see proposal above.
> > Another idea could be adding something like AnnotationsName to each
> > AnnotationsEntity/AnnotableObject/the name you like.
> 
> I still have some issues with EntityName, as described previously... I don't
> love AnnotationsName either, but it's definitely clearer, so maybe go with
that
> for now.

I thought we'd worked out that EntityName was OK - it's a name for an entity in
the state. Given that we already have the concept of a global name for such an
entity, what value does creating AnnotationsName add? Will we ever want to have
AnnotationsName != EntityName?

[fwereade, 2013-03-13 11:26:11]

On 2013/03/13 11:02:58, rog wrote:
> I thought we'd worked out that EntityName was OK - it's a name for an entity
in
> the state. Given that we already have the concept of a global name for such an
> entity, what value does creating AnnotationsName add? Will we ever want to
have
> AnnotationsName != EntityName?

Sorry, that wasn't what I took away from the conversation -- I don't think I've
changed my views on the fundamental problem that (1) we have entities, (2)
entities have names, (3) entity names don't match their EntityName()s. I'm a lot
more comfortable with AuthName and AnnotationsName methods that coincidentally
always match than I am with having an intrinsically confusing EntityName method
anywhere; but I could probably be swayed with an argument that included a better
common name for the two methods, and might even be happiest with that solution.
Suggestions?

[frankban, 2013-03-13 12:20:37]

On 2013/03/13 11:26:11, fwereade wrote:
> On 2013/03/13 11:02:58, rog wrote:
> > I thought we'd worked out that EntityName was OK - it's a name for an entity
> in
> > the state. Given that we already have the concept of a global name for such
an
> > entity, what value does creating AnnotationsName add? Will we ever want to
> have
> > AnnotationsName != EntityName?
> 
> Sorry, that wasn't what I took away from the conversation -- I don't think
I've
> changed my views on the fundamental problem that (1) we have entities, (2)
> entities have names, (3) entity names don't match their EntityName()s. I'm a
lot
> more comfortable with AuthName and AnnotationsName methods that coincidentally
> always match than I am with having an intrinsically confusing EntityName
method
> anywhere; but I could probably be swayed with an argument that included a
better
> common name for the two methods, and might even be happiest with that
solution.
> Suggestions?

GlobalName()? And then make both AnnotationEntity and AuthEntity include that?
I don't know, and I can move forward in both cases, and maybe it does not make
sense, but here it is: an entity is something that has a globalKey() for Mongo
and a GlobalName() for the outside world.

[rog, 2013-03-13 12:43:05]

On 13 March 2013 11:26,  <fwereade@gmail.com> wrote:
> On 2013/03/13 11:02:58, rog wrote:
>>
>> I thought we'd worked out that EntityName was OK - it's a name for an
>
> entity in
>>
>> the state. Given that we already have the concept of a global name for
>
> such an
>>
>> entity, what value does creating AnnotationsName add? Will we ever
>
> want to have
>>
>> AnnotationsName != EntityName?
>
>
> Sorry, that wasn't what I took away from the conversation -- I don't
> think I've changed my views on the fundamental problem that (1) we have
> entities, (2) entities have names, (3) entity names don't match their
> EntityName()s.

Yes, I see that issue. I don't think I have too much of a problem with
x.Name() being distinct from x.EntityName(), but there are alternatives.
We could rename all Name methods to Id. We could think of another
spelling for EntityName (StateName? GlobalName? - I don't like either
of those tbh, but... suggestions?).

>  I'm a lot more comfortable with AuthName and
> AnnotationsName methods that coincidentally always match

It's not just coincidental - it's important from the point of view
of the API that there be a coherent way of naming objects
in the state, because those names are used for both authentication
and notifications.

Perhaps we should have a G+ meeting about this issue - there are
too many degrees of freedom and I fear that anything I suggest
will not be acceptable.

[rog, 2013-03-13 12:45:18]

On 2013/03/09 15:13:36, fwereade wrote:
> Sorry, but NOT LGTM until someone can explain what on earth has happened to
the
> concept of an Entity.
> 
> Once it just meant machine/service/unit/relation. Then users were added. Then
> annotations were added. Now the environment has been added... and each of
these
> additions has resulted in more special cases.

Presumably the special cases you're thinking of here are places
where entities stub out the Entity methods they don't implement?

I actually think that these additions have resulted in less special cases -
external code can check passwords and add annotations without needing to have
different special cases for different kinds of entity, and there's a single way
to retrieve and operate on a given state entity. I don't have a problem with
having an interface that some entities may not implement completely - there's no
*inherent* problem, AFAICS with having the possibility of relations having
working annotations or services a password, for example.

I think that having a single Entity interface that all state entities implement
is
actually a simplification. Including "environment" in those entities is perhaps
a *little* stretch but maps well to the way that the API (which is in the end
the only client for the state package) would like to handle things.

> At this point, it's completely incoherent. At an *absolute* minimum, we have
to
> separate the concept of "state entity" from that of "something that can log
in".

I think of logging in as an agent identifying itself on behalf of an entity in
the state, and annotation as annotating the same thing, so I'd actually be happy
if there was no such separation, but I *think* I see where you're coming from.

Let's chat about this.

[fwereade, 2013-03-15 14:55:39]

On 2013/03/13 12:45:18, rog wrote:
> Let's chat about this.

Chat was had a couple of days ago, but I believe Francesco is on holiday; WIP
until he returns.

[frankban, 2013-03-18 17:31:11]

Please take a look.

[frankban, 2013-03-18 17:38:44]

The branch was changed as suggested in this MP discussion.
Filed bugs https://bugs.launchpad.net/juju-core/+bug/1156714 (fix race
conditions) and https://bugs.launchpad.net/juju-core/+bug/1156715 (replace
EntityName with something better). We will take care of those issues later, once
the annotations branches are merged.
Also updated the EnvironmentInfo API call: now the env name is included in the
returned data. This way the GUI will be able to fetch the env name and use it
later when setting or getting annotations on the environment.

[frankban, 2013-03-18 17:40:52]

Please take a look.

[dimitern, 2013-03-19 09:06:12]

Much better, thanks! LGTM modulo some trivials below.

https://codereview.appspot.com/7598043/diff/39004/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/annotator.go#newcode100
state/annotator.go:100: // annotationRemoveOps returns an operation to remove a
given annotation document from MongoDB.
please rename this to annotationRemoveOp, since it returns a single op only (and
see the comment below about appending).

https://codereview.appspot.com/7598043/diff/39004/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/apiserver/api_test.go...
state/apiserver/api_test.go:573: // PasswordValid which is implemented by state
entities but not by api
s/but/, /

https://codereview.appspot.com/7598043/diff/39004/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/apiserver/apiserver.g...
state/apiserver/apiserver.go:515: _entity    state.Authenticator // logged-in
entity (access only when mu is locked)
why the underscore here? please remove.

https://codereview.appspot.com/7598043/diff/39004/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/service.go#newcode193
state/service.go:193: return append(ops, annotationRemoveOps(s.st,
s.globalKey()))
"Ops" is confusing when only a single op is returned, moreover if there are
really more than one, here and in other append calls, you should append "..."
after the call. See for example state/service:removeUnitOps and how it gets used
with append().

https://codereview.appspot.com/7598043/diff/39004/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/state.go#newcode359
state/state.go:359: return st.Environment()
shouldn't this be: return st.Environment(id) ?

https://codereview.appspot.com/7598043/diff/39004/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/state_test.go#newcode...
state/state_test.go:1361: c.Assert(err, ErrorMatches, `settings not found`)
"" instead of `` here is enough

https://codereview.appspot.com/7598043/diff/39004/state/state_test.go#newcode...
state/state_test.go:1433: c.Assert(err, ErrorMatches, `entity ".*" does not
support authentication`)
why .*? don't you know the name by now, so you can put it in the literal regexp?

https://codereview.appspot.com/7598043/diff/39004/state/state_test.go#newcode...
state/state_test.go:1463: c.Assert(err, ErrorMatches, `entity ".*" does not
support annotations`)
ditto

[frankban, 2013-03-19 10:17:57]

Please take a look.

https://codereview.appspot.com/7598043/diff/39004/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/annotator.go#newcode100
state/annotator.go:100: // annotationRemoveOps returns an operation to remove a
given annotation document from MongoDB.
On 2013/03/19 09:06:12, dimitern wrote:
> please rename this to annotationRemoveOp, since it returns a single op only
(and
> see the comment below about appending).

Good catch, thanks, fixed.

https://codereview.appspot.com/7598043/diff/39004/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/apiserver/api_test.go...
state/apiserver/api_test.go:573: // PasswordValid which is implemented by state
entities but not by api
On 2013/03/19 09:06:12, dimitern wrote:
> s/but/, /

Done.

https://codereview.appspot.com/7598043/diff/39004/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/apiserver/apiserver.g...
state/apiserver/apiserver.go:515: _entity    state.Authenticator // logged-in
entity (access only when mu is locked)
On 2013/03/19 09:06:12, dimitern wrote:
> why the underscore here? please remove.

I don't know why, _entity preexisted.
Anyway, renamed.

Also renamed the entity method to "authenticator".

https://codereview.appspot.com/7598043/diff/39004/state/service.go
File state/service.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/service.go#newcode193
state/service.go:193: return append(ops, annotationRemoveOps(s.st,
s.globalKey()))
On 2013/03/19 09:06:12, dimitern wrote:
> "Ops" is confusing when only a single op is returned, moreover if there are
> really more than one, here and in other append calls, you should append "..."
> after the call. See for example state/service:removeUnitOps and how it gets
used
> with append().

Thanks for your suggestion. Renamed the function so that it is clear that we are
appending a single op.

https://codereview.appspot.com/7598043/diff/39004/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/state.go#newcode359
state/state.go:359: return st.Environment()
On 2013/03/19 09:06:12, dimitern wrote:
> shouldn't this be: return st.Environment(id) ?

No, the Environment method takes no arguments. AFAICT, we always have one
environment for each bootstrapped... environment. The env name/id is included in
the annotator name for consistency: all the other annotator names follow the
"type-name" schema. That id is used here to ensure the requested id equals the
current environment name. However, I don't think we should require the
environment name when we instantiate a state environment.

https://codereview.appspot.com/7598043/diff/39004/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/7598043/diff/39004/state/state_test.go#newcode...
state/state_test.go:1361: c.Assert(err, ErrorMatches, `settings not found`)
On 2013/03/19 09:06:12, dimitern wrote:
> "" instead of `` here is enough

Done.

https://codereview.appspot.com/7598043/diff/39004/state/state_test.go#newcode...
state/state_test.go:1433: c.Assert(err, ErrorMatches, `entity ".*" does not
support authentication`)
On 2013/03/19 09:06:12, dimitern wrote:
> why .*? don't you know the name by now, so you can put it in the literal
regexp?

Done.

https://codereview.appspot.com/7598043/diff/39004/state/state_test.go#newcode...
state/state_test.go:1463: c.Assert(err, ErrorMatches, `entity ".*" does not
support annotations`)
On 2013/03/19 09:06:12, dimitern wrote:
> ditto

Done.

[rog, 2013-03-19 18:08:38]

This is looking great, with a few trivials, and one thing that needs fixing.
LGTM with all the below done.

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go
File state/annotator.go (left):

https://codereview.appspot.com/7598043/diff/18001/state/annotator.go#oldcode11
state/annotator.go:11: annotations *map[string]string
On 2013/03/09 15:13:36, fwereade wrote:
> Why are we dropping the local data storage? It may be a reasonable thing to do
> but it's pretty inconsistent and deserves a comment.

if we store the annotations locally in the entity doc of the thing that's being
annotated, we lose much of the benefit of keeping the annotations in a separate
collection IMHO.

we *could* have a separate Annotations type holding the annotations in local
storage, but i think simply returning a map is just as good.

https://codereview.appspot.com/7598043/diff/80001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7598043/diff/80001/state/annotator.go#newcode20
state/annotator.go:20: // annotator stores information required to query
MongoDB.
// annotator implements annotation-related methods
// for any entity that wishes to use it.
?

https://codereview.appspot.com/7598043/diff/80001/state/annotator.go#newcode77
state/annotator.go:77: func (a annotator) Annotations() (map[string]string,
error) {
unless there's a particularly good reason, it's conventional to make methods on
a type be either all pointer-receiver or all value-receiver.

i'd go for
func (a *annotator) Annotations() (map[string]string, error) {

(and for Annotation below)

https://codereview.appspot.com/7598043/diff/80001/state/annotator.go#newcode81
state/annotator.go:81: // Returning an empty map if there are no annotations.
s/Returning/Return/

and i'm afraid, despite an earlier review comment, i think it's best to return a
made map here - the caller may well retrieve the map, then try to modify it.

https://codereview.appspot.com/7598043/diff/80001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7598043/diff/80001/state/apiserver/apiserver.g...
state/apiserver/apiserver.go:100: e := r.user.authenticator()
this reads well.

https://codereview.appspot.com/7598043/diff/80001/state/apiserver/apiserver.g...
state/apiserver/apiserver.go:168: if r.user.entityName != name {
this isn't right - we should only be accessing the entity name within when
holding the r.user mutex, because it can be modified concurrently.

perhaps we could change the authenticator method to return the entity name too.
perhaps "authInfo" might work as a name for that method.

https://codereview.appspot.com/7598043/diff/80001/state/apiserver/apiserver.g...
state/apiserver/apiserver.go:516: entityName string
this needs a comment.
perhaps a comment before mu:

// mu guards access to the following fields.
mu sync.Mutex
// entity holds the logged-in entity
...
// entityName holds the name of the logged-in entity.


but actually... all the entities we can log in as in still have EntityName
methods, don't they?

so we could do:

type namedAuthenticator interface {
     state.Authenticator
     EntityName() string
}

and just use the logic we had previously (no need to add an entityName field).

you'd need to type-assert the entity result of st.Authenticator first.

or else change state.Authenticator to have an AuthName method too, which may
well be a better solution (and means no type assert would be necessary, although
you'd need to implement AuthName on the various state entities (it would just
return the same as EntityName)

https://codereview.appspot.com/7598043/diff/80001/state/apiserver/apiserver.g...
state/apiserver/apiserver.go:548: func (u *authUser) authenticator()
state.Authenticator {
nice name.

https://codereview.appspot.com/7598043/diff/80001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7598043/diff/80001/state/state.go#newcode282
state/state.go:282: type Authenticator interface {
as mentioned earlier, perhaps add AuthName to this interface.

https://codereview.appspot.com/7598043/diff/80001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7598043/diff/80001/state/unit.go#newcode103
state/unit.go:103: unit.annotator = annotator{unit.globalKey(),
unit.EntityName(), st}
please name the fields here.

[frankban, 2013-03-20 11:17:31]

Please take a look.

https://codereview.appspot.com/7598043/diff/80001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7598043/diff/80001/state/annotator.go#newcode20
state/annotator.go:20: // annotator stores information required to query
MongoDB.
On 2013/03/19 18:08:39, rog wrote:
> // annotator implements annotation-related methods
> // for any entity that wishes to use it.
> ?

Done.

https://codereview.appspot.com/7598043/diff/80001/state/annotator.go#newcode77
state/annotator.go:77: func (a annotator) Annotations() (map[string]string,
error) {
On 2013/03/19 18:08:39, rog wrote:
> unless there's a particularly good reason, it's conventional to make methods
on
> a type be either all pointer-receiver or all value-receiver.
> 
> i'd go for
> func (a *annotator) Annotations() (map[string]string, error) {
> 
> (and for Annotation below)

Done.

https://codereview.appspot.com/7598043/diff/80001/state/annotator.go#newcode81
state/annotator.go:81: // Returning an empty map if there are no annotations.
On 2013/03/19 18:08:39, rog wrote:
> s/Returning/Return/
> 
> and i'm afraid, despite an earlier review comment, i think it's best to return
a
> made map here - the caller may well retrieve the map, then try to modify it.

Done.

https://codereview.appspot.com/7598043/diff/80001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7598043/diff/80001/state/apiserver/apiserver.g...
state/apiserver/apiserver.go:516: entityName string
On 2013/03/19 18:08:39, rog wrote:

> or else change state.Authenticator to have an AuthName method too, which may
> well be a better solution (and means no type assert would be necessary,
although
> you'd need to implement AuthName on the various state entities (it would just
> return the same as EntityName)

As discussed on IRC, for now I temporarily added the EntityName() to the
Authenticator. We will soon introduce the Tag, Tagger and TaggedAuthenticator.

https://codereview.appspot.com/7598043/diff/80001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7598043/diff/80001/state/unit.go#newcode103
state/unit.go:103: unit.annotator = annotator{unit.globalKey(),
unit.EntityName(), st}
On 2013/03/19 18:08:39, rog wrote:
> please name the fields here.

Done. Also fixed in machine, service and environment.
For the future, when are named fields preferred over positional fields, and
vice-versa?

[fwereade, 2013-03-20 12:58:49]

LGTM, thank you for bearing with me on this.

https://codereview.appspot.com/7598043/diff/88001/state/annotator.go
File state/annotator.go (right):

https://codereview.appspot.com/7598043/diff/88001/state/annotator.go#newcode71
state/annotator.go:71: return fmt.Errorf("cannot set annotation %q = %q on %s:
%v", key, value, id, err)
My failure to write the transactions doc is probably to blame here, so I'm not
going to block this, on the understanding that this work will be imminently
revisited when we come to set multiple annotations atomically.

https://codereview.appspot.com/7598043/diff/88001/state/environ.go
File state/environ.go (right):

https://codereview.appspot.com/7598043/diff/88001/state/environ.go#newcode11
state/environ.go:11: func (st *State) Environment() (*Environment, error) {
I think that this type should capture and expose via methods the environ's
default-series, agent-version, and provider-type, for use in the various bits of
agent code that currently rely on unacceptable access to the environ config; but
that's not something for this branch.

[frankban, 2013-03-20 13:35:13]

On 2013/03/20 12:58:49, fwereade wrote:
> LGTM, thank you for bearing with me on this.

Thanks for your reviews, Roger and William.

> https://codereview.appspot.com/7598043/diff/88001/state/annotator.go#newcode71
> state/annotator.go:71: return fmt.Errorf("cannot set annotation %q = %q on %s:
> %v", key, value, id, err)
> My failure to write the transactions doc is probably to blame here, so I'm not
> going to block this, on the understanding that this work will be imminently
> revisited when we come to set multiple annotations atomically.

As mentioned, I filed bug https://bugs.launchpad.net/juju-gui/+bug/1156714
specifically for addressing this issue. As discussed, my plan is to 1) update
and propose the setAnnotations branch (at that point the GUI can communicate
with juju-core), and then 2) have a branch that fixes the race condition (with
the help of your draft/document), and then 3) tackle the Tag/Tagger/
refactoring.
Does it sound reasonable?

[rog, 2013-03-20 15:54:16]

LGTM
i'm glad i noticed the below - it needs fixing, but not in this branch.

https://codereview.appspot.com/7598043/diff/88001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/7598043/diff/88001/state/state_test.go#newcode...
state/state_test.go:1353: st, err := state.Initialize(state.TestingStateInfo(),
cfg)
calling Initialize on an already-initialized state seems wrong to me, but i see
that it's done elsewhere too. we can leave it for now and then change all of the
occurrences later.

[frankban, 2013-03-20 16:17:03]

*** Submitted:

Annotation documents and Environment Entity

Annotations were moved to separate documents in order to prevent the act of
setting an annotation from affecting watchers on the entities.  Annotations are
stored, one document per entity, in a separate collection which are removed when
the entities are removed.  In addition, an environment-level Entity was created
in order to allow storing annotations on the environment, and potentially for
expansion in the future.

R=matthew.scott, dimitern, fwereade, rog
CC=roger.peppe
https://codereview.appspot.com/7598043