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.

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

[WIP] Fix service updates #3108

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

[WIP] Fix service updates #3108

wants to merge 7 commits into from

Conversation

jakolehm
Copy link
Contributor

@jakolehm jakolehm commented Dec 7, 2017

Currently it's really hard for users to actually remove properties from a stack.. most likely user needs to introduce first "empty" value, deploy and then remove whole property. Things should not be this hard.. this PR aims to fix these issues with better cli/server (mutation) logic.

@@ -13,7 +13,7 @@ class GridService
field :stateful, type: Boolean, default: false
field :user, type: String
field :container_count, type: Integer, default: 1
field :cmd, type: Array
field :cmd, type: Array, default: []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might bite... not sure what is the best way to handle?

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 it changes the :cmd => [] that the agent sees via RPC... but it seems like both the agent weavewait wrapper logic and the Docker API ignores that empty array, and still uses the image's CMD.

@matti
Copy link
Contributor

matti commented Dec 7, 2017

will this also fix #960 ?

data['stop_grace_period'] = options['stop_grace_period'] ? options['stop_grace_period'] : nil
data['read_only'] = options['read_only'] || false
data['entrypoint'] = options['entrypoint'] ? options['entrypoint'] : nil

deploy_opts = options['deploy'] || {}
data['strategy'] = deploy_opts['strategy'] if deploy_opts['strategy']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strategy is not reverting to the default it you remove it from the YAML... and changing this to send {"strategy": null} ends badly:

172.17.0.1 - - [07/Dec/2017:13:17:10 +0000] "PUT /v1/stacks/development/deploy-strategy HTTP/1.1" 200 1338 0.1151
I, [2017-12-07T13:17:11.025318 #17]  INFO -- GridServices::Update: updating service development/deploy-strategy/test with changes: strategy
172.17.0.1 - - [07/Dec/2017:13:17:11 +0000] "POST /v1/stacks/development/deploy-strategy/deploy HTTP/1.1" 200 153 0.1120
I, [2017-12-07T13:17:11.052678 #17]  INFO -- StackDeployWorker: removing following services: 
I, [2017-12-07T13:17:11.075384 #17]  INFO -- StackDeployWorker: deploying service development/deploy-strategy/test...
I, [2017-12-07T13:17:11.244300 #19]  INFO -- GridServiceSchedulerWorker: starting development/deploy-strategy/test deploy
E, [2017-12-07T13:17:11.253861 #19] ERROR -- : Actor crashed!
NoMethodError: undefined method `new' for nil:NilClass
	/app/app/jobs/grid_service_scheduler_worker.rb:89:in `deployer'
	/app/app/jobs/grid_service_scheduler_worker.rb:68:in `deploy'
	/app/app/jobs/grid_service_scheduler_worker.rb:13:in `block in watch'
	/app/app/jobs/grid_service_scheduler_worker.rb:11:in `loop'
	/app/app/jobs/grid_service_scheduler_worker.rb:11:in `watch'
	/usr/lib/ruby/gems/2.4.0/gems/celluloid-0.17.3/lib/celluloid/calls.rb:28:in `public_send'
	/usr/lib/ruby/gems/2.4.0/gems/celluloid-0.17.3/lib/celluloid/calls.rb:28:in `dispatch'
	/usr/lib/ruby/gems/2.4.0/gems/celluloid-0.17.3/lib/celluloid/call/async.rb:7:in `dispatch'
	/usr/lib/ruby/gems/2.4.0/gems/celluloid-0.17.3/lib/celluloid/cell.rb:50:in `block in dispatch'
	/usr/lib/ruby/gems/2.4.0/gems/celluloid-0.17.3/lib/celluloid/cell.rb:76:in `block in task'
	/usr/lib/ruby/gems/2.4.0/gems/celluloid-0.17.3/lib/celluloid/actor.rb:339:in `block in task'
	/usr/lib/ruby/gems/2.4.0/gems/celluloid-0.17.3/lib/celluloid/task.rb:44:in `block in initialize'
	/usr/lib/ruby/gems/2.4.0/gems/celluloid-0.17.3/lib/celluloid/task/fibered.rb:14:in `block in create'
172.17.0.1 - - [07/Dec/2017:13:17:12 +0000] "GET /v1/stacks/development/deploy-strategy/deploys/5a293f577c3294001115b6db HTTP/1.1" 200 482 0.0988
172.17.0.1 - - [07/Dec/2017:13:17:12 +0000] "GET /v1/stacks/development/deploy-strategy/deploys/5a293f577c3294001115b6db HTTP/1.1" 200 482 0.0988
E, [2017-12-07T13:17:15.785039 #19] ERROR -- GridSchedulerJob: error occurred in service development/deploy-strategy/test
E, [2017-12-07T13:17:15.785152 #19] ERROR -- GridSchedulerJob: undefined method `new' for nil:NilClass

@SpComb
Copy link
Contributor

SpComb commented Dec 7, 2017

👍 in principle, the current implementation doesn't make sense: some things like data['hooks'] = options['hooks'] || {} will get cleared from the service if you remove the hooks: block from the YAML, but then others like data['certificates'] = options['certificates'] if options['certificates'] will remain configured on the service if you remove it from the YAML.

Not sure about how the server GridSerivces::Update mutation now sets the mongoid document attrs to nil, though.

@jakolehm
Copy link
Contributor Author

Rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants