Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
storageprovisioner: fix watching of volume-backed filesystems #7346
Conversation
wallyworld
approved these changes
May 16, 2017
I think this seems ok, but I might be misunderstanding the semantics of the machineFilesystemsWatcher as I don't get why all 3 contained watchers need to fire before an event is omitted.
| + // The tests all operate on machine "0", and the watchers | ||
| + // should ignore attachments for other machines, so we should | ||
| + // never get here. | ||
| + panic("should not get here") |
wallyworld
May 16, 2017
Owner
Instead of panicing, why not return an error and let the caller's c.Assert(err, jc.ErrorIsNil) fail the test. IMO since the panic would result from a logic error ("code should do such and such"), any such violation is a test failure.
| @@ -0,0 +1,453 @@ | ||
| +package filesystemwatcher |
| +// results from machine- and model-scoped watchers, to conform to the behaviour | ||
| +// of the storageprovisioner worker. The model-level storageprovisioner watches | ||
| +// model-scoped filesystems that have no backing volume. The machine-level worker | ||
| +// watches both machine-scoped filesytems, and model-scoped filesystems attached |
| +// WatchModelFilesystems returns a strings watcher that reports model-scoped | ||
| +// filesystems that have no backing volume. Volume-backed filesystems are | ||
| +// always managed by the machine to which they are attached. | ||
| +func (fw Watchers) WatchModelFilesystems() state.StringsWatcher { |
wallyworld
May 16, 2017
Owner
might be verbose, but could it be WatchOrphanedModelFilesystems() or WatchUnbackedModelFileSystems() or something? the method name doesn't imply that there's a condition on which model filesystems are watched
axw
May 17, 2017
Member
I'll change it to "ModelManaged" and "MachineManaged", to make it clear that we're interested in machines that are managed by the model-level or machine-level storage provisioner.
| + w.changes = make(set.Strings) | ||
| + out = nil | ||
| + } | ||
| + if machineFilesystemsReceived && |
wallyworld
May 16, 2017
Owner
Is that right? All 3 underlying watchers have to fire before we send any events?
axw
May 17, 2017
Member
Yes. The way all watchers work is to first give the initial state of the world in a single event, and then later changes can report on individual aspects. If we were to report the machine-scoped filesystems without also reporting the model-scoped ones, then the worker would think that the model-scoped ones were not part of the model any more.
| + C chan []string | ||
| +} | ||
| + | ||
| +func NewStringsWatcher(ch chan []string) *StringsWatcher { |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
axw
added some commits
May 14, 2017
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
axw commentedMay 16, 2017
Description of change
A recent change was made to the state package's filesystem and filesystem attachment watchers. The change was to filter the results based on the state of volume attachments. This doesn't work without also watching the volume attachment collection for changes, so that change has been reverted.
In its place, we introduce composite watchers in the apiserver/storageprovisioner/internal/filesystemwatcher package. These watchers compose watchers for model and machine filesystems, and volume attachments, to produce answers that the storage provisioner worker expects.
There are some incidental bug fixes, found while verifying this change:
QA steps
$ juju bootstrap google
$ juju deploy postgresql --storage pgdata=gce,10G
(wait)
$ juju detach-storage pgdata/0
(wait)
$ juju attach-storage postgresql/0 pgdata/0
(storage should show up as attached)
Documentation changes
None.
Bug reference
None.