-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix service volume update #1742
Conversation
Why not allow to update anonymous volumes for stateful service? |
Because anonymous volume cannot be updated to a data-volume container. Bind & named volume are "outside" so they can be added later. |
Technically nothing stops to add anonymous volumes to the service container. The data-volume container is immutable. But as we discussed, it might be too confusing for the users as they will not see what data is persisted on the data-volume and what are only on the local volumes. I think this should be explained in the documentation. |
add_error(:volumes, :invalid, 'Adding a non-named volume is not supported to a stateful service') | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be also updated in the create mutation to have same "rules" in both create and update? This basically means that it should be separate method in common
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only volumes_from
is the same in both cases.. but yes, that can be in common
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the existing GridServices::Create#validate
already enforces this:
[fail] Creating stack wordpress
[error] {"services"=>"Service create failed for service 'wordpress': Cannot combine stateful & volumes_from"}
👍 on fixing the
|
if self.volumes | ||
changed_volumes = self.volumes.select { |v| !self.grid_service.volumes.include?(v) } | ||
if changed_volumes.any? { |v| !v.include?(':') } | ||
add_error(:volumes, :invalid, 'Adding a non-named volume is not supported to a stateful service') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor wording fix: "Adding a non-named volume to a stateful service is not supported"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
expect(outcome.result.volumes).to eq(['/data', '/foo:/foo']) | ||
end | ||
|
||
it 'allows to remove a volume' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing an anonymous volumes: /var/lib/test
from a stateful service doesn't actually remove volumes present when it was initially deployed, since those anonymous volumes will still be configured on the *-volumes
container, and will still be visible to the re-deployed service container.
core@core-01 ~ $ docker inspect -f '{{.Config.Volumes}}' wordpress.wordpress-1
map[/var/www/html:{}]
core@core-01 ~ $ docker inspect -f '{{range .Mounts}}{{.Destination}} {{end}}' wordpress.wordpress-1-volumes
/var/lib/test /var/www/html
core@core-01 ~ $ docker exec wordpress.wordpress-1 ls -la /var/lib/test/
total 20
drwxr-xr-x. 2 root root 4096 Feb 1 13:29 .
drwxr-xr-x. 1 root root 4096 Feb 1 13:29 ..
-rw-r--r--. 1 root root 0 Feb 1 13:29 asdf
Probably not much of an issue, but that's how it behaves :)
Allow to update service volume definitions, except when service is stateful and added volume is not a bind-mounted or a named volume.