Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Introduce environ/provider versioning #7599
Conversation
axw
referenced this pull request
Jul 6, 2017
Merged
Separate model environ upgrades from controller upgrades #7602
babbageclunk
approved these changes
Jul 7, 2017
This looks good to me - just a couple of questions and things I didn't quite follow.
| @@ -436,5 +437,12 @@ type UpgradeStep interface { | ||
| Description() string | ||
| // Run executes the upgrade business logic. | ||
| - Run() error | ||
| + Run(UpgradeStepParams) error |
axw
Jul 7, 2017
Member
I did that because I was going to use the steps to determine the current best version. Then I added EnvironProvider.Version() instead. I'll revert this change.
| + if attempt > 0 { | ||
| + if attempt == 1 { | ||
| + mCopy := *m | ||
| + m = &mCopy |
babbageclunk
Jul 7, 2017
Member
Wouldn't it be much simpler to just copy the model outside buildTxn? It doesn't seem like SetEnvironVersion would be called often enough that the extra efficiency is worth the awkwardness here.
| - return upgradeToVersion{op.TargetVersion, steps} | ||
| + // NOTE(axw) all two of the current environ upgrade steps will happily | ||
| + // run idempotently; there are no post-upgrade steps that will render | ||
| + // them non-runabble. This is here as a backstop, to ensure the upgrades |
babbageclunk
Jul 7, 2017
Member
typo - runnable.
Also, I don't really understand this comment. Are you referring to the use of jujuversion.Current instead of op.TargetVersion? Why does that make it a backstop?
axw
Jul 7, 2017
Member
it's not super important, because it's gone in the next PR :)
yes, I'm referring to the use of jujuversion.Current rather than op.TargetVersion. the point I was trying to make is that this will force the upgrade steps to continue to run, until we move the logic out of controller upgrades
babbageclunk
Jul 7, 2017
Member
Cool, thanks - yes, I see what you mean about the target handling in the next PR.
| return &upgradeStep{ | ||
| step.Description(), | ||
| []Target{DatabaseMaster}, | ||
| func(Context) error { | ||
| - return step.Run() | ||
| + return step.Run(environs.UpgradeStepParams{controllerUUID}) |
babbageclunk
Jul 7, 2017
Member
Ah - is it more convenient to write the upgrade steps if you don't need to thread this information down to the steps yourself?
axw
added some commits
Jul 5, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
axw commentedJul 5, 2017
•
Edited 1 time
-
axw
Jul 5, 2017
Description of change
Environs (i.e. the cloud representation of
a model) are now versioned, so that we can
upgrade them as necessary. Previously this
has been done as part of controller upgrades,
but this poses a problem when the environs
cannot be opened; thus we seek to decouple
the upgrading of a model's environ from the
upgrading of the controller.
EnvironProvider now has a Version method,
which returns the current/most up-to-date
environ version. Model docs now record the
version of the model's associated environ.
Environ upgrade operations specify an
environ/provider version, rather than
controller/agent version.
Once this lands, we will introduce a new
worker that will run under the model
dependency engine to upgrade the model's
environ, and which will gate access to the
model until that has happened.
Requires juju/description#16
(There is a drive-by fix in state/upgrade.go, to stop checking for env-uuid upgrades. That all happened before 2.0, so we don't need to carry that code anymore.)
QA steps
No functional changes. Bootstrap azure with 2.1.x, juju upgrade-juju to this branch.
Documentation changes
None.
Bug reference
Part of the fix for https://bugs.launchpad.net/juju/+bug/1700451