state: upgrade leases before starting manager

[axw]

Description of change

Since the lease managers are started when you
open a State object, they may be in use before
the lease upgrade step runs. This can lead to
errors, since the lease manager will choke upon
seeing the old lease documents.

To work around this, we upgrade the lease docs
just prior to starting the lease manager. When
we extract the lease managers from State, we
can drop this bit of code and rely on the upgrade
step. The upgrade step has been left in place
for this purpose.

QA steps

1. juju bootstrap (with a juju from before lease changes - 698a34c)
2. add a bunch of models
3. juju upgrade-juju -m controller (with juju from this branch)
4. confirm operational with "juju status"
5. confirm there are no "corrupt lease document" errors in the log

Documentation changes

None.

Bug reference

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

[jameinel]

another option might be to just allow reading docs that might be the old form rather than treating them as "corrupted".

also, we clearly have a bug where corrupted documents cause us to spin and leak file descriptors. we should try to figure out what is leaking so that general restarts don't leak

[axw]

another option might be to just allow reading docs that might be the old form rather than treating them as "corrupted".

I did consider that, but I'd rather keep this package clean, and put the crappy hack (upgrade before start) with the other crappy hacks (start worker from within State).

also, we clearly have a bug where corrupted documents cause us to spin and leak file descriptors. we should try to figure out what is leaking so that general restarts don't leak

Yup, looking into it now.

[jameinel]

as mentioned, there may be other ways but this seems ok. I have not done any manual testing of it yet to know if it really does solve the upgrade problem.

[jameinel]

why not leave this as an old type instead of being an embedded struct. type oldLeaseDoc or something like that?

[axw]

Why have a named type when you don't need one? I just cribbed from another upgrade. Doesn't really matter either way.

[jameinel]

I'm also concerned that we *need* This for anyone who might be testing upgrading to beta2. I guess maybe we don't usually ask people to test out the beta yet? John =:->

…

On Nov 2, 2017 06:45, "Andrew Wilkins" ***@***.***> wrote: Description of change Since the lease managers are started when you open a State object, they may be in use before the lease upgrade step runs. This can lead to errors, since the lease manager will choke upon seeing the old lease documents. To work around this, we upgrade the lease docs just prior to starting the lease manager. When we extract the lease managers from State, we can drop this bit of code and rely on the upgrade step. The upgrade step has been left in place for this purpose. QA steps 1. juju bootstrap (with a juju from before lease changes - 698a34c <698a34c> ) 2. add a bunch of models 3. juju upgrade-juju -m controller (with juju from this branch) 4. confirm operational with "juju status" 5. confirm there are no "corrupt lease document" errors in the log Documentation changes None. Bug reference Fixes https://bugs.launchpad.net/juju/+bug/1728972 ------------------------------ You can view, comment on, or merge this pull request online at: #8011 Commit Summary - state: upgrade leases before starting manager File Changes - *M* state/state.go <https://github.com/juju/juju/pull/8011/files#diff-0> (35) - *M* state/upgrades.go <https://github.com/juju/juju/pull/8011/files#diff-1> (83) Patch Links: - https://github.com/juju/juju/pull/8011.patch - https://github.com/juju/juju/pull/8011.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#8011>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMYfSjUweaykhMN_ZrF8KZr9gQ26qDqks5sySy-gaJpZM4QPF-D> .

[axw]

Landing this, will continue investigating the leak separately.

[axw]

$$merge$$

[jujubot]

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju

[jameinel]

Mostly because it allows a name and a context. It also seems pretty similar to the general philosophy that you shouldn't have inline defined constants. It also promotes it a bit to feel like something you should be careful about modifying arbitrarily. It certainly isn't necessary, we as long as you don't ever need to reference it in more than one place. John =:->

…

On Nov 2, 2017 07:22, "Andrew Wilkins" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In state/upgrades.go <#8011 (comment)>: > // Find all old lease/clock-skew documents, remove them // and create replacement lease docs in the new format. // // Replacement leases are created with a duration of a // minute, relative to the global time epoch. - var ops []txn.Op - iter := coll.Find(bson.D{{"type", bson.D{{"$exists", true}}}}).Iter() - for iter.Next(&doc) { - ops = append(ops, txn.Op{ - C: leasesC, - Id: st.localID(doc.DocID), - Assert: txn.DocExists, - Remove: true, - }) - if doc.Type != "lease" { - continue + err := st.db().Run(func(int) ([]txn.Op, error) { + var doc struct { + DocID string `bson:"_id"` Why have a named type when you don't need one? I just cribbed from another upgrade. Doesn't really matter either way. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8011 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMYfZpaApqNIQI6jrkUnh14X-XEcBltks5syTV0gaJpZM4QPF-D> .