Add machine instance metadata to state

Add machine instance metadata to state

When a machine is provisioned, it is assigned an
instance id and also has characteristics like 
mem, cores etc. This branch allows for this
metadata to be stored in state in a new
machineMetadata document. The machineMetadata
and machine document share the same primary key.

The InstanceId and Nonce attributes are pulled off
machine and stored with the rest of the instance
metadata. Backwards comnpatibility is retained until
schema upgrades are possible.

https://code.launchpad.net/~wallyworld/juju-core/instance-metadata-in-state/+merge/170740

Requires: https://code.launchpad.net/~wallyworld/juju-core/tweak-instance-metadata-api/+merge/170483

(do not edit description out of merge proposal)

[wallyworld, 2013-06-21 06:09:23]

Please take a look.

[fwereade, 2013-06-21 11:14:52]

Not there yet, I think -- let me know your thoughts on the below.

https://codereview.appspot.com/10447045/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode65
state/machine.go:65: metadata      *machineMetadata
I'm not sure we need it here as well. See ramblings below.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode68
state/machine.go:68: // TODO(wallyworld): remove these attributes when schema
upgrades are possible.
Shall we have a standard SCHEMACHANGE comment tag so we can find these easily?

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode111
state/machine.go:111: // machineMetadata holds attributes relevant to a
provisioned machine.
I think this is possibly a state.Instance actually? With MachineId and
MachineNonce?

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode120
state/machine.go:120: TxnRevno   int64       `bson:"txn-revno"`
Don't think this is needed, is it?

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode124
state/machine.go:124: func (m *Machine) Metadata() (*instance.Metadata, error) {
Not sure about the caching on the machine object -- do we use that a lot? If
not, I might argue it's complexity we can live without here.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode132
state/machine.go:132: }
metadata := *m.doc.metadata
return &metadata, nil

?

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode506
state/machine.go:506: // (we first check for InstanceId stored on the
machineDoc)
SCHEMACHANGE

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode512
state/machine.go:512: return "", false
Hmmm. This feels like it ought to be an actual error.

What if we were to keep InstanceId on the machine regardless (then we can have
an instances collection keyed on instance.Id) and use nonce to determine whether
or not there'll be an instance document actually stored?

Then we can avoid hitting the db in here, and keep the signature the same.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode541
state/machine.go:541: func (m *Machine) SetProvisioned(id instance.Id, nonce
string, metadata *instance.Metadata) (err error) {
metadata here is still feeling very much like HardwareCharacteristics. I'll try
to shut up about that now.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode563
state/machine.go:563: Assert: isAliveDoc,
Wait a mo, this isn't compatible. If a new client writes this, an old client
will get the wrong idea about the state, and this screws us both coming and
going I think. We may *have* to keep all existing info on the machine doc, for
now, and either check for metadata every time or add a HasMetadata field to
avoid the db hit.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode573
state/machine.go:573: m.doc.metadata = md
Yeah, this makes me slightly uncomfortable, but I guess it's reasonable -- the
metadata is immutable, right? If it's not it needs to either be updated in
Refresh(), or not cached at all, I think.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode589
state/machine.go:589: // (we first check for InstanceId and Nonce stored on the
machineDoc)
SCHEMACHANGE

https://codereview.appspot.com/10447045/diff/1/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/10447045/diff/1/state/megawatcher.go#newcode90
state/megawatcher.go:90: panic("cannot find mongo id from metadata document")
If we leave InstanceId on the machine doc, we can leave the megawatcher alone
(and only write this code when the GUI starts wanting to know more instance
info).

https://codereview.appspot.com/10447045/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10447045/diff/1/state/state_test.go#newcode776
state/state_test.go:776: err = dying.SetProvisioned(instance.Id("i-blah"),
"fake-nonce", nil)
Not sure how meaningful it is to allow a nil.

https://codereview.appspot.com/10447045/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10447045/diff/1/state/watcher.go#newcode1060
state/watcher.go:1060: txnRevno = m.doc.metadata.TxnRevno
TxnRevno on.metadata looks very wrong. What are we trying to do here? Watch
changes since the cached version?

[wallyworld, 2013-06-25 01:28:48]

I'd very much like to keep InstanceId and Nonce off machine and bundled with the
other instance metadata like mem, cores etc. I think of machineDoc as like a
class definition, and machineMetadata as akin to an instantiation of that class.
So machineMetadata rightly has attributes which are set when a machine instance
is actually created/provisioned, and whose lifecycle tied to that instance.

https://codereview.appspot.com/10447045/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode65
state/machine.go:65: metadata      *machineMetadata
On 2013/06/21 11:14:53, fwereade wrote:
> I'm not sure we need it here as well. See ramblings below.

If we don't use caching, I can remove it.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode68
state/machine.go:68: // TODO(wallyworld): remove these attributes when schema
upgrades are possible.
On 2013/06/21 11:14:53, fwereade wrote:
> Shall we have a standard SCHEMACHANGE comment tag so we can find these easily?

+1

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode120
state/machine.go:120: TxnRevno   int64       `bson:"txn-revno"`
On 2013/06/21 11:14:53, fwereade wrote:
> Don't think this is needed, is it?

I thought it was to allow a watcher to know from when to start notifying of
changes?

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode124
state/machine.go:124: func (m *Machine) Metadata() (*instance.Metadata, error) {
On 2013/06/21 11:14:53, fwereade wrote:
> Not sure about the caching on the machine object -- do we use that a lot? If
> not, I might argue it's complexity we can live without here.

I suspect it may not be used very often so I'll remove the caching.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode512
state/machine.go:512: return "", false
On 2013/06/21 11:14:53, fwereade wrote:
> Hmmm. This feels like it ought to be an actual error.
> 
> What if we were to keep InstanceId on the machine regardless (then we can have
> an instances collection keyed on instance.Id) and use nonce to determine
whether
> or not there'll be an instance document actually stored?
> 
> Then we can avoid hitting the db in here, and keep the signature the same.

m.Metadata()returning an error would be unexpected under normal circumstances. I
thought that returning false was a reasonable compromise in order to keep the
method signature the same.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode541
state/machine.go:541: func (m *Machine) SetProvisioned(id instance.Id, nonce
string, metadata *instance.Metadata) (err error) {
On 2013/06/21 11:14:53, fwereade wrote:
> metadata here is still feeling very much like HardwareCharacteristics. I'll
try
> to shut up about that now.

I changed the struct name to HardwareCharacteristics just for you

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode563
state/machine.go:563: Assert: isAliveDoc,
On 2013/06/21 11:14:53, fwereade wrote:
> Wait a mo, this isn't compatible. If a new client writes this, an old client
> will get the wrong idea about the state, and this screws us both coming and
> going I think. We may *have* to keep all existing info on the machine doc, for
> now, and either check for metadata every time or add a HasMetadata field to
> avoid the db hit.

When you say "client", you mean jujud and/or the API server right? Since these
are the only components that access machine.InstanceId() AFAICT. I thought/hoped
that for a given environment, our upgrade process would ensure all such backend
services would be upgraded together. I made it so that InstanceId() could read
old schemas, since we don't upgrade the schema yet. But that's all I was
expecting to have to do, especially prior to 2.0.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode573
state/machine.go:573: m.doc.metadata = md
On 2013/06/21 11:14:53, fwereade wrote:
> Yeah, this makes me slightly uncomfortable, but I guess it's reasonable -- the
> metadata is immutable, right? If it's not it needs to either be updated in
> Refresh(), or not cached at all, I think.

Yes, metadata/hardware characteristics are immutable right now.

https://codereview.appspot.com/10447045/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10447045/diff/1/state/state_test.go#newcode776
state/state_test.go:776: err = dying.SetProvisioned(instance.Id("i-blah"),
"fake-nonce", nil)
On 2013/06/21 11:14:53, fwereade wrote:
> Not sure how meaningful it is to allow a nil.

Nil is fine. If nil, a new empty doc is created to hold the instance id and
nonce and the hardware characteristics parts are unknown/nil.

https://codereview.appspot.com/10447045/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10447045/diff/1/state/watcher.go#newcode1060
state/watcher.go:1060: txnRevno = m.doc.metadata.TxnRevno
On 2013/06/21 11:14:53, fwereade wrote:
> TxnRevno on.metadata looks very wrong. What are we trying to do here? Watch
> changes since the cached version?

I thought that's what the watcher infrastructure required to ensure only new
changes since the document was last read were published. I re-used the pattern I
thought I saw used in watching machineDoc

[thumper, 2013-06-25 01:46:25]

https://codereview.appspot.com/10447045/diff/1/state/machine.go
File state/machine.go (left):

https://codereview.appspot.com/10447045/diff/1/state/machine.go#oldcode66
state/machine.go:66: Clean         bool
If we are moving Nonce and InstanceId off, isn't "Clean" really an attribute of
the instantiation of the machine?

https://codereview.appspot.com/10447045/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode111
state/machine.go:111: // machineMetadata holds attributes relevant to a
provisioned machine.
On 2013/06/21 11:14:53, fwereade wrote:
> I think this is possibly a state.Instance actually? With MachineId and
> MachineNonce?

and Clean?  This makes some sense.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode124
state/machine.go:124: func (m *Machine) Metadata() (*instance.Metadata, error) {
On 2013/06/25 01:28:48, wallyworld wrote:
> On 2013/06/21 11:14:53, fwereade wrote:
> > Not sure about the caching on the machine object -- do we use that a lot? If
> > not, I might argue it's complexity we can live without here.
> 
> I suspect it may not be used very often so I'll remove the caching.

+1

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode512
state/machine.go:512: return "", false
On 2013/06/25 01:28:48, wallyworld wrote:
> On 2013/06/21 11:14:53, fwereade wrote:
> > Hmmm. This feels like it ought to be an actual error.
> > 
> > What if we were to keep InstanceId on the machine regardless (then we can
have
> > an instances collection keyed on instance.Id) and use nonce to determine
> whether
> > or not there'll be an instance document actually stored?
> > 
> > Then we can avoid hitting the db in here, and keep the signature the same.
> 
> m.Metadata()returning an error would be unexpected under normal circumstances.
I
> thought that returning false was a reasonable compromise in order to keep the
> method signature the same.

Perhaps just check the err, and if it is a DocNotFound mgo error (for whatever
that is), then it isn't an error, but if it is a different type of error, then
it is a real error.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode512
state/machine.go:512: return "", false
On 2013/06/21 11:14:53, fwereade wrote:
> Hmmm. This feels like it ought to be an actual error.
> 
> What if we were to keep InstanceId on the machine regardless (then we can have
> an instances collection keyed on instance.Id) and use nonce to determine
whether
> or not there'll be an instance document actually stored?
> 
> Then we can avoid hitting the db in here, and keep the signature the same.

I'm -1 on keeping the attribute on the main machine doc.  If we are going to
have an instance metadata doc for whatever the name ends up being, then the
instance id should live on that.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode541
state/machine.go:541: func (m *Machine) SetProvisioned(id instance.Id, nonce
string, metadata *instance.Metadata) (err error) {
On 2013/06/25 01:28:48, wallyworld wrote:
> On 2013/06/21 11:14:53, fwereade wrote:
> > metadata here is still feeling very much like HardwareCharacteristics. I'll
> try
> > to shut up about that now.
> 
> I changed the struct name to HardwareCharacteristics just for you

I'm find with HardwareCharacteristics as long as that is all that it holds.

https://codereview.appspot.com/10447045/diff/1/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/10447045/diff/1/state/megawatcher.go#newcode90
state/megawatcher.go:90: panic("cannot find mongo id from metadata document")
On 2013/06/21 11:14:53, fwereade wrote:
> If we leave InstanceId on the machine doc, we can leave the megawatcher alone
> (and only write this code when the GUI starts wanting to know more instance
> info).

I have to confess to having NFI what is going on here.

[wallyworld, 2013-06-26 04:57:21]

Please take a look.

https://codereview.appspot.com/10447045/diff/1/state/machine.go
File state/machine.go (left):

https://codereview.appspot.com/10447045/diff/1/state/machine.go#oldcode66
state/machine.go:66: Clean         bool
On 2013/06/25 01:46:25, thumper wrote:
> If we are moving Nonce and InstanceId off, isn't "Clean" really an attribute
of
> the instantiation of the machine?

It's best to keep it directly on machineDoc otherwise it gets messy. An
unprovisioned machine will have no instanceData so we would need to use a not
found error loading the instanceData to mean the machine is clean. But after
provisioning, if the instanceData doc were removed for whatever reason, we would
assume the machine is clean when it may really be dirty. Think of Clean as being
similar to Life - both sort of are related and together communicate the
lifecycle/status of the machine.

[fwereade, 2013-06-26 16:56:31]

I'm still cutting up rough about the field move I'm afraid. Might be up lateish
tonight, ping me?

https://codereview.appspot.com/10447045/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode120
state/machine.go:120: TxnRevno   int64       `bson:"txn-revno"`
On 2013/06/25 01:28:48, wallyworld wrote:
> On 2013/06/21 11:14:53, fwereade wrote:
> > Don't think this is needed, is it?
> 
> I thought it was to allow a watcher to know from when to start notifying of
> changes?

The field is always written by mgo/txn, but we don't need to declare it unless
we're actually using it to start a document watch. We can add it if and when we
need an entity watcher on this doc.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode512
state/machine.go:512: return "", false
On 2013/06/25 01:46:25, thumper wrote:
> Perhaps just check the err, and if it is a DocNotFound mgo error (for whatever
> that is), then it isn't an error, but if it is a different type of error, then
> it is a real error.

Yeah, I think that becomes a NotFound error.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode512
state/machine.go:512: return "", false
On 2013/06/25 01:28:48, wallyworld wrote:
> On 2013/06/21 11:14:53, fwereade wrote:
> > Hmmm. This feels like it ought to be an actual error.
> > 
> > What if we were to keep InstanceId on the machine regardless (then we can
have
> > an instances collection keyed on instance.Id) and use nonce to determine
> whether
> > or not there'll be an instance document actually stored?
> > 
> > Then we can avoid hitting the db in here, and keep the signature the same.
> 
> m.Metadata()returning an error would be unexpected under normal circumstances.
I
> thought that returning false was a reasonable compromise in order to keep the
> method signature the same.

The convention is that ,bool returns are only for stuff that literally cannot go
wrong in any way except one, indicated by the bool status. More possible
problems imply a signature change.

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode512
state/machine.go:512: return "", false
On 2013/06/25 01:46:25, thumper wrote:
> I'm -1 on keeping the attribute on the main machine doc.  If we are going to
> have an instance metadata doc for whatever the name ends up being, then the
> instance id should live on that.

I *do* agree that it would be nice have the keys the other way round once we've
got the schema nicely hidden behind the API (modulo having an id for the
instance and an _id for the associated machine... err).

However, I'm not comfortable messing with compatibility here -- downgrades are
theoretically possible, apart from anything else, and so I think we must
continue to use the existing location for now.

And, FWIW, I also think that Nonce is definitely a machine property. It's stored
at provisioning time, but that doesn't make it instance data. Counterargument?

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode563
state/machine.go:563: Assert: isAliveDoc,
On 2013/06/25 01:28:48, wallyworld wrote:
> On 2013/06/21 11:14:53, fwereade wrote:
> > Wait a mo, this isn't compatible. If a new client writes this, an old client
> > will get the wrong idea about the state, and this screws us both coming and
> > going I think. We may *have* to keep all existing info on the machine doc,
for
> > now, and either check for metadata every time or add a HasMetadata field to
> > avoid the db hit.
> 
> When you say "client", you mean jujud and/or the API server right? Since these
> are the only components that access machine.InstanceId() AFAICT. I
thought/hoped
> that for a given environment, our upgrade process would ensure all such
backend
> services would be upgraded together. I made it so that InstanceId() could read
> old schemas, since we don't upgrade the schema yet. But that's all I was
> expecting to have to do, especially prior to 2.0.

We're just about to release something that has to work against an old CLI
client. It's true that all backend machines will (er, usually, near-enough) be
upgraded at near-enough the same time, but we're not currently protecting this
behind the API so I don't think it's safe to change until we hit 2.0 (or at
least have the client and relevant agent tasks talking exclusively over the API,
and have figured out how to *prevent* downgrading an environment... which I'm
not entirely +1 on).

[fwereade, 2013-06-26 16:58:36]

whoops, one more comment I forgot to make

https://codereview.appspot.com/10447045/diff/10001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10447045/diff/10001/state/watcher.go#newcode1062
state/watcher.go:1062: return newEntityWatcher(m.st, m.st.instanceData.Name,
m.doc.Id, txnRevNo), nil
I'm not sure this deserves a watcher at this stage.

[wallyworld, 2013-06-26 23:12:55]

https://codereview.appspot.com/10447045/diff/10001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10447045/diff/10001/state/watcher.go#newcode1062
state/watcher.go:1062: return newEntityWatcher(m.st, m.st.instanceData.Name,
m.doc.Id, txnRevNo), nil
On 2013/06/26 16:58:36, fwereade wrote:
> I'm not sure this deserves a watcher at this stage.

The watcher is so a provisioner test can work. The test provisions a machine and
watches to ensure the instance id etc are updated. If I remove the watcher, I'd
need to poll or something.

[wallyworld, 2013-06-27 04:02:39]

> 
> https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode120
> state/machine.go:120: TxnRevno   int64       `bson:"txn-revno"`
> On 2013/06/25 01:28:48, wallyworld wrote:
> > On 2013/06/21 11:14:53, fwereade wrote:
> > > Don't think this is needed, is it?
> > 
> > I thought it was to allow a watcher to know from when to start notifying of
> > changes?
> 
> The field is always written by mgo/txn, but we don't need to declare it unless
> we're actually using it to start a document watch. We can add it if and when
we
> need an entity watcher on this doc.
>

There is an entity watcher to allow the provisioner tests to work.
 
> 
> The convention is that ,bool returns are only for stuff that literally cannot
go
> wrong in any way except one, indicated by the bool status. More possible
> problems imply a signature change.
>

I've fixed it.
 
> https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode512
> state/machine.go:512: return "", false
> On 2013/06/25 01:46:25, thumper wrote:
> > I'm -1 on keeping the attribute on the main machine doc.  If we are going to
> > have an instance metadata doc for whatever the name ends up being, then the
> > instance id should live on that.
> 
> I *do* agree that it would be nice have the keys the other way round once
we've
> got the schema nicely hidden behind the API (modulo having an id for the
> instance and an _id for the associated machine... err).
> 
> However, I'm not comfortable messing with compatibility here -- downgrades are
> theoretically possible, apart from anything else, and so I think we must
> continue to use the existing location for now.
>

I write the InstanceId to both the machineDoc and instanceData doc. So we are
fully backwards and forwards compatible.
 
> And, FWIW, I also think that Nonce is definitely a machine property. It's
stored
> at provisioning time, but that doesn't make it instance data. Counterargument?
>

Done.

[wallyworld, 2013-06-27 04:04:51]

Something has screwed up the diffs and comments (some of my earlier comments are
missing). Anyway, I think I've addressed the major concerns.

InstanceId is written to both old and new places. We can remove it from the old
place once all existing deployments are upgraded.

Nonce is back on machineDoc.

machine.InstanceId() returns (string, error), where error may be a
NotProvisionedError

https://codereview.appspot.com/10447045/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/1/state/machine.go#newcode124
state/machine.go:124: func (m *Machine) Metadata() (*instance.Metadata, error) {
On 2013/06/25 01:46:25, thumper wrote:
> On 2013/06/25 01:28:48, wallyworld wrote:
> > On 2013/06/21 11:14:53, fwereade wrote:
> > > Not sure about the caching on the machine object -- do we use that a lot?
If
> > > not, I might argue it's complexity we can live without here.
> > 
> > I suspect it may not be used very often so I'll remove the caching.
> 
> +1

Done.

[wallyworld, 2013-06-27 04:16:24]

Please take a look.

[fwereade, 2013-06-27 10:25:25]

Very close, let me know what you think about these...

https://codereview.appspot.com/10447045/diff/19001/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/10447045/diff/19001/cmd/juju/ssh.go#newcode119
cmd/juju/ssh.go:119: // https://bugs.launchpad.net/juju-core/+bug/1130051
This is crazy. Since we've spotted it, would you do a driveby fix to refresh the
machine at the top of the loop please?

https://codereview.appspot.com/10447045/diff/19001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/19001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:250: c.Fail()
else {
    c.Check(err, X.Satisfies, state.IsNotProvisionedError)
}

?

https://codereview.appspot.com/10447045/diff/19001/doc/hacking-state.txt
File doc/hacking-state.txt (right):

https://codereview.appspot.com/10447045/diff/19001/doc/hacking-state.txt#newc...
doc/hacking-state.txt:152: Entities may exist in a relationship with another
entity. The _id of the
TYVM for this -- I have a slight quibble with the use of the word "entity"
though. Can we keep that term restricted to machine/unit/service/relation,
please?

(maybe this is a battle not worth fighting. I kinda think it remains a useful
and meaningful term, but maybe it's all in my own head at this point)

https://codereview.appspot.com/10447045/diff/19001/environs/jujutest/livetest...
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/10447045/diff/19001/environs/jujutest/livetest...
environs/jujutest/livetests.go:623: c.Assert(state.IsNotProvisionedError(err),
IsTrue)
I think there's that Satisfies checker now?

https://codereview.appspot.com/10447045/diff/19001/environs/jujutest/livetest...
environs/jujutest/livetests.go:662: c.Assert(state.IsNotProvisionedError(err),
IsTrue)
ditto

https://codereview.appspot.com/10447045/diff/19001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/machine.go#newcode612
state/machine.go:612: // TODO(wallyworld) - remove this backward compatibility
code when schema upgrades are possible
Honestly, I think this whole method should really just be "return nonce ==
m.doc.Nonce".

https://codereview.appspot.com/10447045/diff/19001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/machine_test.go#newc...
state/machine_test.go:417: func (s *MachineSuite) TestMachineRefresh(c *C) {
I think the InstanceId change seriously obscures this test... and possibly
renders it meaningless. What happens to it when we switch over, and how does
that have anything to do with a machine refresh?

https://codereview.appspot.com/10447045/diff/19001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/megawatcher.go#newco...
state/megawatcher.go:43: // Second the instance metadata.
Given that it's immutable, can't we simplify this whole thing by just using the
nonce's presence to infer the existence of the InstanceId doc, and not bother
with the extra backing at all? YAGNI ;).

https://codereview.appspot.com/10447045/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/state.go#newcode326
state/state.go:326: // has a value, else nil is returned.
A little note that it's to work around the lack of instance data that would
otherwise be passed into InjectMachine?

https://codereview.appspot.com/10447045/diff/19001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/state_test.go#newcod...
state/state_test.go:962: wc.AssertOneChange()
Maybe just add a change to the machine doc and an AssertNoChange?

https://codereview.appspot.com/10447045/diff/19001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/watcher.go#newcode1061
state/watcher.go:1061: }
I find this sort of structure often reads better in these situations. YMMV.

var txnRevno int64
if data, err := getInstanceData(...); errors.IsNotFoundError(err) {
    txnRevno = -1
} else if err == nil {
    txnRevno = data.TxnRevno
} else {
    return nil, err
}

https://codereview.appspot.com/10447045/diff/19001/worker/provisioner/provisi...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/10447045/diff/19001/worker/provisioner/provisi...
worker/provisioner/provisioner_task.go:202: if _, err := machine.InstanceId();
err == nil {
You know, this change isn't really looking like such an improvement at the
moment. I'll reserve judgment for now, though.

[wallyworld, 2013-06-27 13:43:07]

Please take a look.

https://codereview.appspot.com/10447045/diff/19001/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/10447045/diff/19001/cmd/juju/ssh.go#newcode119
cmd/juju/ssh.go:119: // https://bugs.launchpad.net/juju-core/+bug/1130051
On 2013/06/27 10:25:26, fwereade wrote:
> This is crazy. Since we've spotted it, would you do a driveby fix to refresh
the
> machine at the top of the loop please?

The bug is fixed by the new implementation. I'll remove the comment and resolve
the bug.

https://codereview.appspot.com/10447045/diff/19001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/19001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:250: c.Fail()
On 2013/06/27 10:25:26, fwereade wrote:
> else {
>     c.Check(err, X.Satisfies, state.IsNotProvisionedError)
> }
> 
> ?

I used FitsTypeOf checker

https://codereview.appspot.com/10447045/diff/19001/doc/hacking-state.txt
File doc/hacking-state.txt (right):

https://codereview.appspot.com/10447045/diff/19001/doc/hacking-state.txt#newc...
doc/hacking-state.txt:152: Entities may exist in a relationship with another
entity. The _id of the
On 2013/06/27 10:25:26, fwereade wrote:
> TYVM for this -- I have a slight quibble with the use of the word "entity"
> though. Can we keep that term restricted to machine/unit/service/relation,
> please?
> 
> (maybe this is a battle not worth fighting. I kinda think it remains a useful
> and meaningful term, but maybe it's all in my own head at this point)

Entity is a well accepted term to describe a significant domain object. I think
it's use here is ok

https://codereview.appspot.com/10447045/diff/19001/environs/jujutest/livetest...
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/10447045/diff/19001/environs/jujutest/livetest...
environs/jujutest/livetests.go:623: c.Assert(state.IsNotProvisionedError(err),
IsTrue)
On 2013/06/27 10:25:26, fwereade wrote:
> I think there's that Satisfies checker now?

I know of a FitsTypeOf checker. But here there's a helper method which I think
it's better to use. And FitsTypeOf required two lines, one to declare a variable
of the type being checked.

https://codereview.appspot.com/10447045/diff/19001/environs/jujutest/livetest...
environs/jujutest/livetests.go:662: c.Assert(state.IsNotProvisionedError(err),
IsTrue)
On 2013/06/27 10:25:26, fwereade wrote:
> ditto

ditto :-)

https://codereview.appspot.com/10447045/diff/19001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/machine.go#newcode612
state/machine.go:612: // TODO(wallyworld) - remove this backward compatibility
code when schema upgrades are possible
On 2013/06/27 10:25:26, fwereade wrote:
> Honestly, I think this whole method should really just be "return nonce ==
> m.doc.Nonce".

Done.

https://codereview.appspot.com/10447045/diff/19001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/machine_test.go#newc...
state/machine_test.go:417: func (s *MachineSuite) TestMachineRefresh(c *C) {
On 2013/06/27 10:25:26, fwereade wrote:
> I think the InstanceId change seriously obscures this test... and possibly
> renders it meaningless. What happens to it when we switch over, and how does
> that have anything to do with a machine refresh?

Agreed. Test removed.

https://codereview.appspot.com/10447045/diff/19001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/megawatcher.go#newco...
state/megawatcher.go:43: // Second the instance metadata.
On 2013/06/27 10:25:26, fwereade wrote:
> Given that it's immutable, can't we simplify this whole thing by just using
the
> nonce's presence to infer the existence of the InstanceId doc, and not bother
> with the extra backing at all? YAGNI ;).

Done.

https://codereview.appspot.com/10447045/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/state.go#newcode326
state/state.go:326: // has a value, else nil is returned.
On 2013/06/27 10:25:26, fwereade wrote:
> A little note that it's to work around the lack of instance data that would
> otherwise be passed into InjectMachine?

Done.

https://codereview.appspot.com/10447045/diff/19001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/state_test.go#newcod...
state/state_test.go:962: wc.AssertOneChange()
On 2013/06/27 10:25:26, fwereade wrote:
> Maybe just add a change to the machine doc and an AssertNoChange?

Done.

https://codereview.appspot.com/10447045/diff/19001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10447045/diff/19001/state/watcher.go#newcode1061
state/watcher.go:1061: }
On 2013/06/27 10:25:26, fwereade wrote:
> I find this sort of structure often reads better in these situations. YMMV.
> 
> var txnRevno int64
> if data, err := getInstanceData(...); errors.IsNotFoundError(err) {
>     txnRevno = -1
> } else if err == nil {
>     txnRevno = data.TxnRevno
> } else {
>     return nil, err
> }

Done.

[fwereade, 2013-06-27 23:14:56]

Sorry, more nitpicking; thank you for putting up with all this. The only really
critical stuff is about compatibility.

https://codereview.appspot.com/10447045/diff/19001/doc/hacking-state.txt
File doc/hacking-state.txt (right):

https://codereview.appspot.com/10447045/diff/19001/doc/hacking-state.txt#newc...
doc/hacking-state.txt:152: Entities may exist in a relationship with another
entity. The _id of the
On 2013/06/27 13:43:08, wallyworld wrote:
> On 2013/06/27 10:25:26, fwereade wrote:
> > TYVM for this -- I have a slight quibble with the use of the word "entity"
> > though. Can we keep that term restricted to machine/unit/service/relation,
> > please?
> > 
> > (maybe this is a battle not worth fighting. I kinda think it remains a
useful
> > and meaningful term, but maybe it's all in my own head at this point)
> 
> Entity is a well accepted term to describe a significant domain object. I
think
> it's use here is ok

Quibble quibble grumble grumble waffle waffle. OK, fair enough, it probably is
all in my head.

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:251: c.Check(err, FitsTypeOf, notProvisionedError)
(*notProvisionedError)(nil)

I think

https://codereview.appspot.com/10447045/diff/27001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode113
state/machine.go:113: Id         string      `bson:"_id"`
If this becomes mutable you may find it convenient to drop this field. No read
need now...

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode565
state/machine.go:565: Update: D{{"$set", D{{"nonce", nonce}}}},
[wait a mo, didn't we agree we'd still write it for compatibility's sake? the
default mode of operation for a user who isn't paying attention (ie bootstrap
with old client and immediately get upgraded tools) will see errors if we don't
keep writing it. right? ...but then... are we inferring anything from its
absence anywhere? if so this is a real problem, if not we can just add the
{"instanceid", id} and be done.]

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode568
state/machine.go:568: Id:     hc.Id,
...although, fwiw, I'd find it clearer here if you'd used m.doc.Id directly --
and I think that's the only reason it's ever used in the code?

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode638
state/machine.go:638: Assert: txn.DocMissing,
Wondering whether nonce presence would be a better check here too, so we only
cause potential contention one doc rather than two?

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go
File state/machine_test.go (left):

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go#oldc...
state/machine_test.go:385: func (s *MachineSuite) TestMachineRefresh(c *C) {
Sorry, I think this may be a coverage loss -- it's an explicit test that Refresh
picks up db state. Maybe just assign/unassign a principal or something instead
of setting instance id?

Or even just tag it with SCHEMACHANGE and point out it'll fail, and should be
replaced, when we stop writing to the old InstanceId field?

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go#newc...
state/machine_test.go:375: }
Maybe a quick race test or two using the transaction hooks?

https://codereview.appspot.com/10447045/diff/27001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/state.go#newcode307
state/state.go:307: Insert: *metadata,
Don't think you actually need to copy here.

https://codereview.appspot.com/10447045/diff/27001/state/state.go#newcode361
state/state.go:361: Series: params.Series,
Again, I think we still need to keep writing it.

I'm totally fine removing it once we can guarantee nobody's going to try to read
it and infer meaning therefrom, and that hopefully won't be too long. In the
meantime, it might be better to assume that, for now, the InstanceId branch is
basically always going to be hit; and just test against an export function that
checks that, yes, the correct instanceData doc was written; and also that a doc
as written in the future (ie with InstanceId deleted) correctly goes and looks
in the instanceData.

https://codereview.appspot.com/10447045/diff/27001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/watcher.go#newcode1058
state/watcher.go:1058: txnRevNo = instData.TxnRevno
I was earlier whining that we didn't really need to watch this, but its
existence won't hurt anything.

[thumper, 2013-06-28 05:07:33]

All niggles, but nothing that will block landing once William is happy. LGTM

https://codereview.appspot.com/10447045/diff/27001/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/10447045/diff/27001/cmd/juju/ssh.go#newcode114
cmd/juju/ssh.go:114: } else if !state.IsNotProvisionedError(err) {
Why have this error in state instead of our more general errors package?

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/bootstrap_test.g...
cmd/jujud/bootstrap_test.go:81: instid, ok, err := machines[0].InstanceId()
This will fail right?  we only return instid and err now right?

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:251: c.Check(err, FitsTypeOf, notProvisionedError)
On 2013/06/27 23:14:56, fwereade wrote:
> (*notProvisionedError)(nil)
> 
> I think

What about Roger's Satisfies,

c.Check(err, Satisfies, IsNotProvisionedError)

https://codereview.appspot.com/10447045/diff/27001/doc/hacking-state.txt
File doc/hacking-state.txt (right):

https://codereview.appspot.com/10447045/diff/27001/doc/hacking-state.txt#newc...
doc/hacking-state.txt:161: retrieved. The globalKey can also sensibly be used in
transaction asserts, as well
Wrap text at 72 chars?  Remember you work at pedantical :-)The 

Elements of Typographic Style" by Robert Bringhurst, mentions the following more
specific measurements:
  * 45 to 75 characters line length (measure); specifically 66 including spaces;
Single Column
  * and more not relevant here.

https://codereview.appspot.com/10447045/diff/27001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/state.go#newcode240
state/state.go:240: //TODO(wallyworld) - figure out how to determine the
existing machine's characteristics so they can be recorded in state
How about collecting the information in the machiner task?

[wallyworld, 2013-06-28 05:51:21]

Please take a look.

https://codereview.appspot.com/10447045/diff/27001/cmd/juju/ssh.go
File cmd/juju/ssh.go (right):

https://codereview.appspot.com/10447045/diff/27001/cmd/juju/ssh.go#newcode114
cmd/juju/ssh.go:114: } else if !state.IsNotProvisionedError(err) {
On 2013/06/28 05:07:34, thumper wrote:
> Why have this error in state instead of our more general errors package?

It really is specific to the state package, not general purpose like NotFound.
The state package already has other errors like HasAssignedUnitsError

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/bootstrap_test.go
File cmd/jujud/bootstrap_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/bootstrap_test.g...
cmd/jujud/bootstrap_test.go:81: instid, ok, err := machines[0].InstanceId()
On 2013/06/28 05:07:34, thumper wrote:
> This will fail right?  we only return instid and err now right?

Bah, missed that. Thanks.

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:251: c.Check(err, FitsTypeOf, notProvisionedError)
On 2013/06/27 23:14:56, fwereade wrote:
> (*notProvisionedError)(nil)
> 
> I think

Done.

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:251: c.Check(err, FitsTypeOf, notProvisionedError)
On 2013/06/28 05:07:34, thumper wrote:
> On 2013/06/27 23:14:56, fwereade wrote:
> > (*notProvisionedError)(nil)
> > 
> > I think
> 
> What about Roger's Satisfies,
> 
> c.Check(err, Satisfies, IsNotProvisionedError)

What is this Satisfies of which you speak. I can't find it anywhere? Is it in a
3rd party package or something?

https://codereview.appspot.com/10447045/diff/27001/doc/hacking-state.txt
File doc/hacking-state.txt (right):

https://codereview.appspot.com/10447045/diff/27001/doc/hacking-state.txt#newc...
doc/hacking-state.txt:161: retrieved. The globalKey can also sensibly be used in
transaction asserts, as well
On 2013/06/28 05:07:34, thumper wrote:
> Wrap text at 72 chars?  Remember you work at pedantical :-)The 
> 
> Elements of Typographic Style" by Robert Bringhurst, mentions the following
more
> specific measurements:
>   * 45 to 75 characters line length (measure); specifically 66 including
spaces;
> Single Column
>   * and more not relevant here.

There's so real max width coding standard we are following for Go AFAIR. You can
easily see this looking at the source code. We discussed it a while ago and I
think it was decided 132 chars was ok, since we are no longer in the 20th
century. Perhaps it could be brought up again. I would consider 80 the absolute
minimum with a preference for 100 or more.

https://codereview.appspot.com/10447045/diff/27001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode565
state/machine.go:565: Update: D{{"$set", D{{"nonce", nonce}}}},
On 2013/06/27 23:14:56, fwereade wrote:
> [wait a mo, didn't we agree we'd still write it for compatibility's sake? the
> default mode of operation for a user who isn't paying attention (ie bootstrap
> with old client and immediately get upgraded tools) will see errors if we
don't
> keep writing it. right? ...but then... are we inferring anything from its
> absence anywhere? if so this is a real problem, if not we can just add the
> {"instanceid", id} and be done.]

I thought about it and it seems to me that the code which uses this attribute is
all server side, especially since machineDoc is unexported and the actual
instanceId attribute is behind a machine getter which does the right thing. I've
just searched the code base and it seems that only the tools call
machine.InstanceId(). The only thing client related is apiserver.client which I
think is just the backend for the api client. So unless I'm on crack, I think
the code as it is ok? I just can't find a place where local juju binaries on
someone's machine can access InstanceId without going via the tools. Sorry if
I'm being dumb or just forgotten the use case which proves me wrong. I'll ping
you to discuss.

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode568
state/machine.go:568: Id:     hc.Id,
On 2013/06/27 23:14:56, fwereade wrote:
> ...although, fwiw, I'd find it clearer here if you'd used m.doc.Id directly --
> and I think that's the only reason it's ever used in the code?

Done.

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode638
state/machine.go:638: Assert: txn.DocMissing,
On 2013/06/27 23:14:56, fwereade wrote:
> Wondering whether nonce presence would be a better check here too, so we only
> cause potential contention one doc rather than two?

I did it this way to be "safe". But if we are sure that an instanceData doc is
always created only when nonce it set then I see your point. Something still
says to me though that it's better to make the explicit check, even if for
"clarity". Perhaps we can leave it as is till/if it becomes an issue? I don't
imagine SetConstraints is called that often to matter?

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go
File state/machine_test.go (left):

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go#oldc...
state/machine_test.go:385: func (s *MachineSuite) TestMachineRefresh(c *C) {
On 2013/06/27 23:14:56, fwereade wrote:
> Sorry, I think this may be a coverage loss -- it's an explicit test that
Refresh
> picks up db state. Maybe just assign/unassign a principal or something instead
> of setting instance id?
> 
> Or even just tag it with SCHEMACHANGE and point out it'll fail, and should be
> replaced, when we stop writing to the old InstanceId field?

Done.

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go#newc...
state/machine_test.go:375: }
On 2013/06/27 23:14:56, fwereade wrote:
> Maybe a quick race test or two using the transaction hooks?

I don't see the need. SetProvisioned doesn't read the database, do some logic
based on the state, and then run transactions. It checks passed in args and then
runs 2 fixed transaction ops which have asserts.

https://codereview.appspot.com/10447045/diff/27001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/state.go#newcode240
state/state.go:240: //TODO(wallyworld) - figure out how to determine the
existing machine's characteristics so they can be recorded in state
On 2013/06/28 05:07:34, thumper wrote:
> How about collecting the information in the machiner task?

Could do. I'd like to do anything like that in a separate branch though.
InjectMachine is only used to put into state the machine doc for the bootstrap
node which was started separately. So having its characteristics recorded is not
really important since we don't deploy to it.

https://codereview.appspot.com/10447045/diff/27001/state/state.go#newcode307
state/state.go:307: Insert: *metadata,
On 2013/06/27 23:14:56, fwereade wrote:
> Don't think you actually need to copy here.

True, Insert accepts both. But a few lines above Insert: *mdoc is used so I
stayed consistent.

https://codereview.appspot.com/10447045/diff/27001/state/state.go#newcode361
state/state.go:361: Series: params.Series,
On 2013/06/27 23:14:56, fwereade wrote:
> Again, I think we still need to keep writing it.
> 
> I'm totally fine removing it once we can guarantee nobody's going to try to
read
> it and infer meaning therefrom, and that hopefully won't be too long. In the
> meantime, it might be better to assume that, for now, the InstanceId branch is
> basically always going to be hit; and just test against an export function
that
> checks that, yes, the correct instanceData doc was written; and also that a
doc
> as written in the future (ie with InstanceId deleted) correctly goes and looks
> in the instanceData.

See my previous rambling. I'll ping you later just to be sure.

https://codereview.appspot.com/10447045/diff/27001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/watcher.go#newcode1058
state/watcher.go:1058: txnRevNo = instData.TxnRevno
On 2013/06/27 23:14:56, fwereade wrote:
> I was earlier whining that we didn't really need to watch this, but its
> existence won't hurt anything.

I had to make an entity watcher for the provisioning tests and I thought
txnRevNo is required for entity watchers. I think I may have mentioned it in a
previous comment but there's been a bit of back and forth so it may have got
lost in the noise.

[fwereade, 2013-06-28 08:07:50]

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:251: c.Check(err, FitsTypeOf, notProvisionedError)
On 2013/06/28 05:51:21, wallyworld wrote:
> On 2013/06/28 05:07:34, thumper wrote:
> > On 2013/06/27 23:14:56, fwereade wrote:
> > > (*notProvisionedError)(nil)
> > > 
> > > I think
> > 
> > What about Roger's Satisfies,
> > 
> > c.Check(err, Satisfies, IsNotProvisionedError)
> 
> What is this Satisfies of which you speak. I can't find it anywhere? Is it in
a
> 3rd party package or something?

testing/checkers/bool.go

https://codereview.appspot.com/10447045/diff/27001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode565
state/machine.go:565: Update: D{{"$set", D{{"nonce", nonce}}}},
On 2013/06/28 05:51:21, wallyworld wrote:
> On 2013/06/27 23:14:56, fwereade wrote:
> > [wait a mo, didn't we agree we'd still write it for compatibility's sake?
the
> > default mode of operation for a user who isn't paying attention (ie
bootstrap
> > with old client and immediately get upgraded tools) will see errors if we
> don't
> > keep writing it. right? ...but then... are we inferring anything from its
> > absence anywhere? if so this is a real problem, if not we can just add the
> > {"instanceid", id} and be done.]
> 
> I thought about it and it seems to me that the code which uses this attribute
is
> all server side, especially since machineDoc is unexported and the actual
> instanceId attribute is behind a machine getter which does the right thing.
I've
> just searched the code base and it seems that only the tools call
> machine.InstanceId(). The only thing client related is apiserver.client which
I
> think is just the backend for the api client. So unless I'm on crack, I think
> the code as it is ok? I just can't find a place where local juju binaries on
> someone's machine can access InstanceId without going via the tools. Sorry if
> I'm being dumb or just forgotten the use case which proves me wrong. I'll ping
> you to discuss.

`juju ssh` and `juju status` both need InstanceId, and won't work right if we
run new tools with an old CLI.

(I think we're probably safe on the server side, although we're still carrying
the original upgrade logic DNA to some degree, which I think is crazier every
time I think about it -- we *don't* guarantee that a whole env will all be
running the same tools at all, we just ...kinda... hope that'll be the case.
Which it probably will. Most of the time. /sigh)

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode638
state/machine.go:638: Assert: txn.DocMissing,
On 2013/06/28 05:51:21, wallyworld wrote:
> On 2013/06/27 23:14:56, fwereade wrote:
> > Wondering whether nonce presence would be a better check here too, so we
only
> > cause potential contention one doc rather than two?
> 
> I did it this way to be "safe". But if we are sure that an instanceData doc is
> always created only when nonce it set then I see your point. Something still
> says to me though that it's better to make the explicit check, even if for
> "clarity". Perhaps we can leave it as is till/if it becomes an issue? I don't
> imagine SetConstraints is called that often to matter? 

AFAICT SetConstraints is only used by tests. Not sure why we added it in the
first place... possibly as a path for manual tweaking of failing constraints
before `juju resolved $machine`. (which doesn't exist yet)

Regardless, compatibility's still an issue here. This will not fail when run
against a machine provisioned by old code, and it should. The major factor
that's pushing me to recommend use of nonce in various situations is that *that*
field doesn't suffer these sorts of problems, and it's tedious hard work
thinking through compatibility issues, so I'd rather do an end run around the
whole problem.

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/machine_test.go#newc...
state/machine_test.go:375: }
On 2013/06/28 05:51:21, wallyworld wrote:
> On 2013/06/27 23:14:56, fwereade wrote:
> > Maybe a quick race test or two using the transaction hooks?
> 
> I don't see the need. SetProvisioned doesn't read the database, do some logic
> based on the state, and then run transactions. It checks passed in args and
then
> runs 2 fixed transaction ops which have asserts.

Fair enough.

https://codereview.appspot.com/10447045/diff/27001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/state.go#newcode240
state/state.go:240: //TODO(wallyworld) - figure out how to determine the
existing machine's characteristics so they can be recorded in state
On 2013/06/28 05:51:21, wallyworld wrote:
> On 2013/06/28 05:07:34, thumper wrote:
> > How about collecting the information in the machiner task?
> 
> Could do. I'd like to do anything like that in a separate branch though.
> InjectMachine is only used to put into state the machine doc for the bootstrap
> node which was started separately. So having its characteristics recorded is
not
> really important since we don't deploy to it.

Medium-term that sounds like a decent idea, probably, because it would be quite
nice to be able to fill in missing info from old-code-provisioned instances.

Might be interesting figuring out exactly which component has that
responsibility, though. Not this CL for sure :).

https://codereview.appspot.com/10447045/diff/39001/doc/hacking-state.txt
File doc/hacking-state.txt (right):

https://codereview.appspot.com/10447045/diff/39001/doc/hacking-state.txt#newc...
doc/hacking-state.txt:171: no need for the disambiguation afforded by using a
global key.
Sorry, I'm having difficulty being as phlegmatic about this today as I was
yesterday. "Entity" already has a meaning in juju, and it's not "mongodb
document"; and "subordinate" also has a meaning in juju, and it's not "mongodb
document" either. (oh, and: relation membership node ids follow the global key
scheme, but involve a more nuanced story -- it's really not about "parent"
documents even, let alone parent entities.

Can we put this particular change to one side and discuss it separately please?

[wallyworld, 2013-06-28 12:49:05]

Please take a look.

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/10447045/diff/27001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:251: c.Check(err, FitsTypeOf, notProvisionedError)
On 2013/06/28 08:07:51, fwereade wrote:
> On 2013/06/28 05:51:21, wallyworld wrote:
> > On 2013/06/28 05:07:34, thumper wrote:
> > > On 2013/06/27 23:14:56, fwereade wrote:
> > > > (*notProvisionedError)(nil)
> > > > 
> > > > I think
> > > 
> > > What about Roger's Satisfies,
> > > 
> > > c.Check(err, Satisfies, IsNotProvisionedError)
> > 
> > What is this Satisfies of which you speak. I can't find it anywhere? Is it
in
> a
> > 3rd party package or something?
> 
> testing/checkers/bool.go

Ah ok. My trunk was only a few days out of date and when I updated it was there.
Thanks.

https://codereview.appspot.com/10447045/diff/27001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode565
state/machine.go:565: Update: D{{"$set", D{{"nonce", nonce}}}},
On 2013/06/28 08:07:51, fwereade wrote:
> On 2013/06/28 05:51:21, wallyworld wrote:
> > On 2013/06/27 23:14:56, fwereade wrote:
> > > [wait a mo, didn't we agree we'd still write it for compatibility's sake?
> the
> > > default mode of operation for a user who isn't paying attention (ie
> bootstrap
> > > with old client and immediately get upgraded tools) will see errors if we
> > don't
> > > keep writing it. right? ...but then... are we inferring anything from its
> > > absence anywhere? if so this is a real problem, if not we can just add the
> > > {"instanceid", id} and be done.]
> > 
> > I thought about it and it seems to me that the code which uses this
attribute
> is
> > all server side, especially since machineDoc is unexported and the actual
> > instanceId attribute is behind a machine getter which does the right thing.
> I've
> > just searched the code base and it seems that only the tools call
> > machine.InstanceId(). The only thing client related is apiserver.client
which
> I
> > think is just the backend for the api client. So unless I'm on crack, I
think
> > the code as it is ok? I just can't find a place where local juju binaries on
> > someone's machine can access InstanceId without going via the tools. Sorry
if
> > I'm being dumb or just forgotten the use case which proves me wrong. I'll
ping
> > you to discuss.
> 
> `juju ssh` and `juju status` both need InstanceId, and won't work right if we
> run new tools with an old CLI.
> 
> (I think we're probably safe on the server side, although we're still carrying
> the original upgrade logic DNA to some degree, which I think is crazier every
> time I think about it -- we *don't* guarantee that a whole env will all be
> running the same tools at all, we just ...kinda... hope that'll be the case.
> Which it probably will. Most of the time. /sigh)

Yes, you are right. I just didn't want to believe that client code could
possibly access the state directly. Double sigh.

https://codereview.appspot.com/10447045/diff/27001/state/machine.go#newcode638
state/machine.go:638: Assert: txn.DocMissing,
On 2013/06/28 08:07:51, fwereade wrote:
> On 2013/06/28 05:51:21, wallyworld wrote:
> > On 2013/06/27 23:14:56, fwereade wrote:
> > > Wondering whether nonce presence would be a better check here too, so we
> only
> > > cause potential contention one doc rather than two?
> > 
> > I did it this way to be "safe". But if we are sure that an instanceData doc
is
> > always created only when nonce it set then I see your point. Something still
> > says to me though that it's better to make the explicit check, even if for
> > "clarity". Perhaps we can leave it as is till/if it becomes an issue? I
don't
> > imagine SetConstraints is called that often to matter? 
> 
> AFAICT SetConstraints is only used by tests. Not sure why we added it in the
> first place... possibly as a path for manual tweaking of failing constraints
> before `juju resolved $machine`. (which doesn't exist yet)
> 
> Regardless, compatibility's still an issue here. This will not fail when run
> against a machine provisioned by old code, and it should. The major factor
> that's pushing me to recommend use of nonce in various situations is that
*that*
> field doesn't suffer these sorts of problems, and it's tedious hard work
> thinking through compatibility issues, so I'd rather do an end run around the
> whole problem.

Done.

https://codereview.appspot.com/10447045/diff/39001/doc/hacking-state.txt
File doc/hacking-state.txt (right):

https://codereview.appspot.com/10447045/diff/39001/doc/hacking-state.txt#newc...
doc/hacking-state.txt:171: no need for the disambiguation afforded by using a
global key.
On 2013/06/28 08:07:51, fwereade wrote:
> Sorry, I'm having difficulty being as phlegmatic about this today as I was
> yesterday. "Entity" already has a meaning in juju, and it's not "mongodb
> document"; and "subordinate" also has a meaning in juju, and it's not "mongodb
> document" either. (oh, and: relation membership node ids follow the global key
> scheme, but involve a more nuanced story -- it's really not about "parent"
> documents even, let alone parent entities.
> 
> Can we put this particular change to one side and discuss it separately
please?

Done.

[fwereade, 2013-06-28 13:08:58]

LGTM with the below fixes

https://codereview.appspot.com/10447045/diff/37002/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10447045/diff/37002/state/machine.go#newcode610
state/machine.go:610: return m.doc.Nonce == nonce
hmm, sorry: maybe nonce == m.doc.Nonce && nonce != ""

https://codereview.appspot.com/10447045/diff/37002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10447045/diff/37002/state/state.go#newcode362
state/state.go:362: Series: params.Series,
This needs InstanceId as well for compatibility, I think.

[wallyworld, 2013-06-30 23:34:59]

https://codereview.appspot.com/10447045/diff/37002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10447045/diff/37002/state/state.go#newcode362
state/state.go:362: Series: params.Series,
On 2013/06/28 13:08:58, fwereade wrote:
> This needs InstanceId as well for compatibility, I think.

I realise looking at this code that despite what was done when it was first
written, instanceId isn't needed here. InstanceId is used by InjectMachine when
setting up the bootstrap node in state, but isn't needed when creating a new
parent machine for a new container. Similarly, instanceId isn't needed when
creating a new container machine doc below. The above is true for current use
cases. If this ever changes, we will already be at the point where the new
instance metadata is in use.