state: Really delete status history in Unit.Destroy

[babbageclunk]

Description of change

The deletion code expected the key to be in the statusid field, but
the status history docs were changed to have the global key stored as
globalkey, so the RemoveAll calls were never deleting anything and
always doing a full scan.

Add a test that the status history is gone and correct the deletion. Also move the eraseHistory function into status. Keeping everything that needs to know about the fields of the underlying collection in one place reduces the likelihood of them getting out of sync (although obviously tests are the final check).

QA steps

• Bootstrap a controller
• Deploy an application with multiple units and wait for the units to be idle
• Check the status history for one unit contains entries
• Remove that unit
• Check that the status history records for that unit are gone (by looking in Mongo).

Bug reference

Fixes https://bugs.launchpad.net/juju/+bug/1696491

[babbageclunk]

I'm going through the QA steps for this now. Also I need to check that it will use the existing indexes. I'm also checking that other entities don't have the same problem (so far it looks like they don't).

[jameinel]

Some comments, but mostly LGTM

[jameinel]

Sometimes I really dislike the "inject model-uuid" abstraction we put in place. Certainly is hard when you're jumping between Juju code and trying to run it live.
Having a "globalKey" string constant here feels easy to get wrong.
Maybe if we put the string next to the struct definition? It would be great if we could do something like:
type historyDoc struct {
globalKey json:$THEKEY
}
...
history.RemoveAll($THEKEY)

I believe you can do
reflect.Type(historyDoc).FieldByName("globalKey").Tag().Get("bson")
but that certainly doesn't fit the "make it easy to keep them in sync" and you still have that "globalKey" constant (though its naming the attribute rather than a database field).

Anyway, this seems ok, maybe slightly better with a string const next to the struct definition.

[babbageclunk]

Yeah, that makes sense - essentially the same thinking that prompted me to move eraseHistory in here, but more so. The reflection code would at least have the advantage of failing noisily if the field changed, rather than the situation here where everything seemed fine except for all of the table scans. I'll add a constant for this anyway.

[jameinel]

Just making sure this test failed before the change.

[babbageclunk]

Yup.

[babbageclunk]

$$merge$$

[jujubot]

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

[jujubot]

Build failed: Generating tarball failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/11098

[babbageclunk]

I noticed a cut-n-pasto in a string, aborted the build and fixed.

$$merge$$

[jujubot]

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju