Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix service volume update #1742

Merged
merged 2 commits into from Feb 2, 2017
Merged

Fix service volume update #1742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a056775
fix service volume update
jakolehm Jan 30, 2017
2aee1fc
fix volumes_from
jakolehm Jan 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion server/app/mutations/grid_services/update.rb
View file
Open in desktop
Expand Up @@ -18,7 +18,6 @@ def validate
if self.links
validate_links(self.grid_service.grid, self.grid_service.stack, self.links)
end

if self.strategy && !self.strategies[self.strategy]
add_error(:strategy, :invalid_strategy, 'Strategy not supported')
end
Expand All @@ -28,6 +27,17 @@ def validate
if self.secrets
validate_secrets_exist(self.grid_service.grid, self.secrets)
end
if self.grid_service.stateful?
if self.volumes_from && self.volumes_from.size > 0
add_error(:volumes_from, :invalid, 'Cannot combine stateful & volumes_from')
end
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')
Copy link
Contributor

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"

end
end
end
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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"}

end

def execute
Expand All @@ -53,6 +63,8 @@ def execute
attributes[:devices] = self.devices if self.devices
attributes[:deploy_opts] = self.deploy_opts if self.deploy_opts
attributes[:health_check] = self.health_check if self.health_check
attributes[:volumes] = self.volumes if self.volumes
attributes[:volumes_from] = self.volumes_from if self.volumes_from

if self.links
attributes[:grid_service_links] = build_grid_service_links(
Expand Down
105 changes: 105 additions & 0 deletions server/spec/mutations/grid_services/update_spec.rb
View file
Open in desktop
Expand Up @@ -93,6 +93,111 @@
).run
expect(outcome.success?).to be(true)
end

context 'volumes' do
context 'stateless service' do
it 'allows to add non-named volume' do
outcome = described_class.new(
grid_service: redis_service,
volumes: ['/foo']
).run
expect(outcome.success?).to be_truthy
expect(outcome.result.volumes).to eq(['/foo'])
end

it 'allows to add named volume' do
outcome = described_class.new(
grid_service: redis_service,
volumes: ['foo:/foo']
).run
expect(outcome.success?).to be_truthy
expect(outcome.result.volumes).to eq(['foo:/foo'])
end

it 'allows to add bind mounted volume' do
outcome = described_class.new(
grid_service: redis_service,
volumes: ['/foo:/foo']
).run
expect(outcome.success?).to be_truthy
expect(outcome.result.volumes).to eq(['/foo:/foo'])
end
end

context 'stateful service' do
let(:stateful_service) do
GridService.create(
grid: grid, name: 'redis', image_name: 'redis:2.8', stateful: true,
volumes: ['/data']
)
end

it 'does not allow to add non-named volume' do
outcome = described_class.new(
grid_service: stateful_service,
volumes: ['/data', '/foo']
).run
expect(outcome.success?).to be_falsey
end

it 'allows to add named volume' do
outcome = described_class.new(
grid_service: stateful_service,
volumes: ['/data', 'foo:/foo']
).run
expect(outcome.success?).to be_truthy
expect(outcome.result.volumes).to eq(['/data', 'foo:/foo'])
end

it 'allows to add bind mounted volume' do
outcome = described_class.new(
grid_service: stateful_service,
volumes: ['/data', '/foo:/foo']
).run
expect(outcome.success?).to be_truthy
expect(outcome.result.volumes).to eq(['/data', '/foo:/foo'])
end

it 'allows to remove a volume' do
Copy link
Contributor

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 :)

outcome = described_class.new(
grid_service: stateful_service,
volumes: []
).run
expect(outcome.success?).to be_truthy
expect(outcome.result.volumes).to eq([])
end
end
end

context 'volumes_from' do
context 'stateless service' do
it 'allows to update volumes_from' do
outcome = described_class.new(
grid_service: redis_service,
volumes_from: ['data-1']
).run
expect(outcome.success?).to be_truthy
expect(outcome.result.volumes_from).to eq(['data-1'])
end
end

context 'stateful service' do
let(:stateful_service) do
GridService.create(
grid: grid, name: 'redis', image_name: 'redis:2.8', stateful: true,
volumes: ['/data']
)
end

it 'does not allow to update volumes_from' do
outcome = described_class.new(
grid_service: stateful_service,
volumes_from: ['data-1']
).run
expect(outcome.success?).to be_falsey
end
end
end
end

describe '#build_grid_service_hooks' do
Expand Down