Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Destroy model: watch better suited collection for changes. #7218
Conversation
| // ModelConfig retrieves the model configuration. | ||
| ModelConfig() (*config.Config, error) | ||
| + | ||
| + // WatchModelEntitiesReferences gets a watcher capable of monitoring |
anastasiamac
Apr 10, 2017
Member
Changed :D nice pick-up! I was, of course thinking of model entities as a concept.
| - watch := common.NewMultiNotifyWatcher(watchers...) | ||
| + // (anastasiamac 2017-04-10) I do not know what watcher is best suited here. | ||
| + // Keeping multi-notify watcher for historical reasons for now. | ||
| + watch := common.NewMultiNotifyWatcher(u.st.WatchModelEntitiesReferences(m.UUID())) |
axw
Apr 10, 2017
Member
Drop the multi and use the entity references watcher directly. There is now and for the foreseeable future should only be one.
| @@ -272,10 +272,6 @@ func (a *Application) removeOps(asserts bson.D) ([]txn.Op, error) { | ||
| Id: a.doc.DocID, | ||
| Assert: asserts, | ||
| Remove: true, | ||
| - }, { |
anastasiamac
Apr 10, 2017
Member
Removed this and the other related bits. Will propose separately :) I do not think that it is particularly hurtful but was very surprising to examine a transaction where several exact same operations appeared...
| @@ -290,7 +286,6 @@ func (a *Application) removeOps(asserts bson.D) ([]txn.Op, error) { | ||
| return nil, errors.Trace(err) | ||
| } | ||
| ops = append(ops, charmOps...) | ||
| - ops = append(ops, finalAppCharmRemoveOps(name, curl)...) |
| @@ -256,12 +256,10 @@ func (s *CharmSuite) TestDestroyFinalUnitReference(c *gc.C) { | ||
| app := s.Factory.MakeApplication(c, &factory.ApplicationParams{ | ||
| Charm: s.charm, | ||
| }) | ||
| - unit := s.Factory.MakeUnit(c, &factory.UnitParams{ |
| @@ -1040,16 +1046,22 @@ func (m *Model) checkEmpty() error { | ||
| } | ||
| return errors.Annotatef(err, "getting entity references for model %s", m.UUID()) | ||
| } | ||
| + // These errors could be potentially swallowed as we re-try to destroy model. | ||
| + // Let's, at least, log them for observations. |
anastasiamac
Apr 10, 2017
Member
Nice :) With my English, both plural and singular work here but I appreciate your feedback! Changed :)
| - // Yes, we ignore the error. See comment above. | ||
| + // Yes, we ignore the error. See comment above. But let's at least | ||
| + // surface it in status. | ||
| + u.setStatus(status.Destroying, fmt.Sprintf("%d attempt to destroy model: %v", attempt, err)) |
jameinel
Apr 10, 2017
Owner
2 attempt to destroy model: error
would probably read better as:
"attempt %d to destroy model failed (will retry): %v"
moving the counter helps readability, and some indication as to what we are doing about this failure.
These are going to be shown in "juju status", so making them legible is important.
anastasiamac
added some commits
Apr 10, 2017
| // ModelConfig retrieves the model configuration. | ||
| ModelConfig() (*config.Config, error) | ||
| + | ||
| + // WatchModelEntityReferences gets a watcher capable of monitoring | ||
| + // model entities references changes. |
| - } | ||
| - services, err := u.st.AllApplications() | ||
| + | ||
| + m, err := u.st.Model() |
axw
Apr 10, 2017
Member
just call state.ModelUUID instead? no need to hit the db just to get the UUID
anastasiamac
Apr 10, 2017
Member
oh k.. I was looking at other code in the area. I'll take a look to see what state.ModelUUID actually gives here :)
anastasiamac
added some commits
Apr 10, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
anastasiamac commentedApr 10, 2017
Description of change
When destroying models, we used to watch only applications and machines collections. However, when we were determining if model is ready to be destroyed, we'd check collection that stored model entities references.
This introduced 2 problems:
This PR changes undertaker behavior to watch for changes on collection that references model entities rather than said entities collections.
We are also now surfacing errors that we've encountered during the attempts to destroy model as well as log them.
Drive-by fixes:
QA steps
There are several scenarios where destroy-model would chase its own tail.
The most commonly encountered one that I have used:
[you would have an empty deployed app]
Model gets destroyed successfully.
Previously, model destruction would get into infinite loop and only eventual kill-controller will get rid of it.
Note that if you are running 'watch juju status' during model destruction, you may not observe failure to destroy.
Documentation changes
n/a - internal change
Bug reference
https://bugs.launchpad.net/juju/+bug/1672549
https://bugs.launchpad.net/juju/+bug/1661930