migrations: Track resource revisions per unit #6534

Merged
merged 5 commits into from Nov 3, 2016

Conversation

Projects
None yet
3 participants
Contributor

mjs commented Nov 3, 2016

This extends the model description format so that application resource revisions are tracked on a per-unit basis. It is possible for a unit to have a different resource revision to the application when it has not yet retrieved a resource or when a resource is being upgraded.

Handling of resource revisions is somewhat awkward due to the denormalised way they are stored in the database.

QA

Deployed a resource using charm and watched the output of juju dump-model. There is a point where the application has been created but the unit has pulled the resource yet and this was correctly reflected. Once the unit agent had retrieved the resource the dump-model output was reflected

mjs added some commits Oct 27, 2016

core/description: Track resources per unit
Infrastructure to allow description of resource revisions in use on a
per-unit basis.
core/description: Revision merging
Resource.AddRevision will now merge revision data when a revision is
given multiple times. This simplifies handling when revision data comes
from multiple sources (application, charmstore & units).

A couple of questions.

core/description/resource.go
@@ -15,7 +15,7 @@ type Resource interface {
Name() string
Revision() int
CharmStoreRevision() int
- AddRevision(ResourceRevisionArgs) ResourceRevision
+ AddRevision(ResourceRevisionArgs) (ResourceRevision, error)
@wallyworld

wallyworld Nov 3, 2016

Owner

would love to see comments on these at some point, especially to explain that AddRevision can accept a rev that already exists in which case it will be merged etc

@mjs

mjs Nov 3, 2016

Contributor

Done.

+ return errors.Errorf("%s mismatch for revision %d", field, args.Revision)
+ }
+ checkStr := func(field, a, b string) error {
+ if a != "" && b != "" && a != b {
@wallyworld

wallyworld Nov 3, 2016

Owner

If a is not empty and b is empty, doesn't that count as a mismatch? As in we are trying to overwrite a previously set value with empty?

@mjs

mjs Nov 3, 2016

Contributor

No, it's ok for b to be empty. It won't get used in that case. The value could come from either side but if it's set on both sides then they have to match.

+
+type unitResources struct {
+ Version int `yaml:"version"`
+ Resources_ []*unitResource `yaml:"resources"`
@wallyworld

wallyworld Nov 3, 2016

Owner

omitempty?

@mjs

mjs Nov 3, 2016

Contributor

Good catch. Done.

@mjs

mjs Nov 3, 2016

Contributor

Actually, Tim hasn't done similar structs with omitempty so I might leave this as-is for consistency.

Contributor

mjs commented Nov 3, 2016

$$merge$$

Contributor

jujubot commented Nov 3, 2016

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

@jujubot jujubot merged commit e794127 into juju:develop Nov 3, 2016

@mjs mjs deleted the mjs:MM-resources-unit-revs branch Dec 11, 2016

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