state: add AttachStorage to AddApplicationArgs #7322
Conversation
state/state.go
Outdated
| @@ -993,6 +994,9 @@ func (st *State) AddApplication(args AddApplicationArgs) (_ *Application, err er | |||
| if args.Charm == nil { | |||
| return nil, errors.Errorf("charm is nil") | |||
| } | |||
| if len(args.AttachStorage) > 0 && args.NumUnits != 1 { | |||
| return nil, errors.Errorf("AttachStorage is non-empty, but NumUnits is %d", args.NumUnits) | |||
There was a problem hiding this comment.
Better to word the message to tell the user what the allowed value is, eg
...., is non-empty, but NumUnits is 23, should be 1
There was a problem hiding this comment.
Nobody should ever see this (apiserver and api client, and cmd, will all check this, and the cmd provides a nicer error message). Nevertheless I'll do it, doesn't hurt.
state/storage_test.go
Outdated
| c.Assert(err, jc.ErrorIsNil) | ||
| c.Assert(app2Units, gc.HasLen, 1) | ||
|
|
||
| // Now attach the storage to the second unit. |
| Name: "secondwind", | ||
| Series: app.Series(), | ||
| Charm: ch, | ||
| Storage: map[string]state.StorageConstraints{ |
There was a problem hiding this comment.
Is this needed here - AttachStorge contains the storage to add.
Actually, what happens when there is a conflict between Storage and AttachStorage
There was a problem hiding this comment.
Storage defines how to create storage for new units; each storage constraint contains a Count. We'll create Count less the number of storage instances in AttachStorage (for the corresponding charm storage name) for the first unit, and the full Count for subsequent units.
e.g. if I do juju deploy ceph-osd --storage osd-devices=3,100G --attach-storage osd-devices/0, we'll create and attach two new osd-devices storage instances, and attach the existing osd-devices/0, to the first unit. If I then juju add-unit ceph-osd, three new storage instances will be created and attached to the new unit.
| @@ -1890,93 +1890,105 @@ func machineStorageParamsForStorageInstance( | |||
| filesystemAttachments := make(map[names.FilesystemTag]FilesystemAttachmentParams) | |||
|
|
|||
| switch storage.Kind() { | |||
| case StorageKindBlock: | |||
| volumeAttachmentParams := VolumeAttachmentParams{ | |||
| case StorageKindFilesystem: | |||
There was a problem hiding this comment.
any reason for swapping the order of the switch values?
There was a problem hiding this comment.
that would be for the fallthrough at the end of the first case
Support attaching existing storage to the first unit when deploying an application.
axw
force-pushed
the
state-addapplication-attachstorage
branch
from
bcd764e
to
fc661da
Compare
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Description of change
Support attaching existing storage to the first unit when deploying an application. When the api/cmd changes are made, this will enable users to
juju deploy foo --attach-storage bar/0, attaching existing persistent storage to a new application unit.QA steps
Smoke test existing attach, since the new stuff is not connected yet:
Documentation changes
None yet.
Bug reference
None.