state: Split logs collection into a collection per model #7441

Merged
merged 7 commits into from Jun 4, 2017

Conversation

Projects
None yet
4 participants
Member

babbageclunk commented Jun 1, 2017

Description of change

When destroying a model on a controller with lots of models, deleting the logs for that model is by far the most expensive part of the operation. Splitting the log collection so we have one log collection per model will mean that we can drop the collection instead, and save space by not storing the model UUID on each row and in the indexes.

QA steps

Member

babbageclunk commented Jun 2, 2017

Job got killed for some reason.

!!chittychitty!!

@babbageclunk babbageclunk changed the title from WIP state: Split logs collection into a collection per model to state: Split logs collection into a collection per model Jun 2, 2017

Member

babbageclunk commented Jun 2, 2017

!!buildplease!!

A few things to consider

state/logs.go
@@ -29,7 +29,7 @@ import (
// TODO(wallyworld) - lp:1602508 - collections need to be defined in collections.go
const (
logsDB = "logs"
- logsC = "logs"
+ logsPrefix = "logs."
state/logs.go
@@ -745,15 +746,15 @@ func PruneLogs(st MongoSessioner, minLogTime time.Time, maxLogsMB int) error {
// Do further pruning if the logs collection is over the maximum size.
@wallyworld

wallyworld Jun 2, 2017

Owner

Need to update this comment - logs collections

state/model.go
@@ -395,6 +395,9 @@ func (st *State) NewModel(args ModelArgs) (_ *Model, _ *State, err error) {
return nil, nil, errors.Annotate(err, "granting admin permission to the owner")
}
+ if err := InitDbLogs(session, uuid); err != nil {
+ return nil, nil, errors.Trace(err)
@wallyworld

wallyworld Jun 2, 2017

Owner

can we annotate the error like the others above

state/upgrades.go
@@ -235,7 +235,7 @@ func DropOldLogIndex(st *State) error {
// If the log collection still has the old e,t index, remove it.
key := []string{"e", "t"}
db := st.MongoSession().DB(logsDB)
- collection := db.C(logsC)
+ collection := db.C("logs")
@wallyworld

wallyworld Jun 2, 2017

Owner

The "logs" collection won't necessarily be there will it? Once we upgrade to 2.2 it will be gone. So this DropOldLogIndex step can be deleted.

@babbageclunk

babbageclunk Jun 3, 2017

Member

It's used in steps_21 - although I guess if someone is upgrading to 2.1 they aren't running this binary, are they? So then the only use for this is if someone is upgrading from 2.0 to 2.2? You're right - I'll delete the step. It feels slightly weird to be deleting something from the 2.1 steps because of something we're changing in 2.2 though.

state/upgrades.go
+func SplitLogCollections(st *State) error {
+ session := st.MongoSession()
+ db := session.DB(logsDB)
+ oldLogs := db.C("logs")
@wallyworld

wallyworld Jun 2, 2017

Owner

This collection won't necessarily be there once we drop it. I guess we drop it below so even though we create it here, it will be gone later. But if there's an error in the upgrade step, and logs had already been dropped before, we will now leave it behind when we shouldn't

@babbageclunk

babbageclunk Jun 3, 2017

Member

I'll add a check that exits if the collection doesn't exist - I have a feeling that the remove all might raise an error if the collection doesn't exist. (I'll add a test.)

@babbageclunk

babbageclunk Jun 3, 2017

Member

It turned out @howbazaar already had handling for the remove failing if the collection didn't exist - calling db.C("logs") doesn't create the collection, it is created lazily on insert. I added a test for that case and verified that it failed if the dropping-nonexistent-collection error handling wasn't there.

state/upgrades.go
+ db := session.DB(logsDB)
+ oldLogs := db.C("logs")
+
+ // If we haven't seen any particular environment, we need to initialise
@wallyworld

wallyworld Jun 2, 2017

Owner

s/environment/model

state/upgrades.go
+ seen.Add(newCollName)
+ }
+
+ delete(doc, "e") // old env uuid
state/upgrades.go
+ delete(doc, "e") // old env uuid
+
+ if err := newLogs.Insert(doc); err != nil {
+ return errors.Annotate(err, "failed to insert log record")
@wallyworld

wallyworld Jun 2, 2017

Owner

need to ignore dupes

@babbageclunk

babbageclunk Jun 3, 2017

Member

Yup - I've got @howbazaar's patch, I'll add that in.

Member

babbageclunk commented Jun 2, 2017

Thanks for the comments @wallyworld, they all make sense. I'll address them when I can later on.

Check build failed because the windows build crapped out.

!!trytryagain!!

Review tweaks, handle duplicate log records
This could happen if a previous run had been interrupted - in that case,
skip duplicate records and continue inserting the rest.

Remove the old 2.1 step to drop an obsolete log index - this isn't needed
since we're removing the whole collection in a later step.
Member

babbageclunk commented Jun 4, 2017

$$merge$$

Contributor

jujubot commented Jun 4, 2017

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

@jujubot jujubot merged commit 46d13fb into juju:develop Jun 4, 2017

1 check passed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@babbageclunk babbageclunk deleted the babbageclunk:split-log-collection branch Jun 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment