Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
state: check constraints when destroying storage #7096
Conversation
| @@ -554,6 +559,8 @@ func (s *FilesystemStateSuite) TestDestroyFilesystemStorageAssigned(c *gc.C) { | ||
| err = s.State.DestroyFilesystem(filesystem.FilesystemTag()) | ||
| c.Assert(err, gc.ErrorMatches, "destroying filesystem 0/0: filesystem is assigned to storage data/0") | ||
| + err = u.Destroy() |
| + // machine, since the only other option is to destroy | ||
| + // them. The user can remove them explicitly, or else | ||
| + // leave them to be removed along with the machine. | ||
| + return nil, nil |
| + | ||
| + count := uint64(current + n) | ||
| + if charmStorage.CountMin == 1 && charmStorage.CountMax == 1 && count != 1 { | ||
| + return errors.Errorf("cannot %s, storage is singular", action) |
wallyworld
Mar 14, 2017
Owner
For most users, not sure if "singular" is better than "there can be at most 1" type thing. It's certainly more concise. Let's make sure we get user feedback on the wording
axw
Mar 14, 2017
Member
yeah, it's a bit clunky. I'll leave it for now, may have another go at it in a follow-up -- otherwise based on feedback
| @@ -1150,6 +1150,31 @@ func (u *Unit) SetCharmURL(curl *charm.URL) error { | ||
| return err | ||
| } | ||
| +// charm returns the charm for the unit, and txn.Ops to ensure that | ||
| +// it does not change. | ||
| +func (u *Unit) charm() (*Charm, []txn.Op, error) { |
wallyworld
Mar 14, 2017
Owner
Would this be better named "assertCharmOps"?
Or "ensureCharmOps" or something.
But then I look at the usage and half the time we just use the charm and ignore the ops. So the above rename is a bit off. It's convenient to combine the 2 operations in one method, but seems a little muddy. Maybe we should just have 2 methods? One for ops and one for charm? Then the 2 places that need the ops can just make an extra call at that point.
axw
Mar 14, 2017
Member
I've split it so we have:
func (*Unit) charm() (*Charm, error) {...}
func (*Unit) assertCharmOps(ch *Charm) []txn.Ops {...}| wc.AssertNoChange() | ||
| } | ||
| func (s *VolumeStateSuite) TestWatchMachineVolumeAttachments(c *gc.C) { | ||
| - service := s.setupMixedScopeStorageService(c, "block") | ||
| + service := s.setupMixedScopeStorageService( |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: cancele |
|
$$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 commentedMar 14, 2017
•
Edited 1 time
-
axw
Mar 15, 2017
Description of change
When detaching or destroying storage, ensure that
doing so does not violate the minimum storage
requirements of the owner (unit or application).
The minimum storage requirements are ignored when
the owner is dying, to allow storage to be cleanly
detached during unit destruction.
Also: rename DestroyStorageAttachment to
DetachStorage, to better match what we have with
volumes and filesystems. An AttachStorage method
will be added in a future PR.
QA steps
(wait for unit to become idle)
(should fail, saying that the storage is singular)
(should fail, saying that the storage is singular)
[0] The storagetest charm's metadata.yaml is as follows:
Documentation changes
None.
Bug reference
None.