Skip to content

Added scale method to the Service model. #1846

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
Merged
merged 3 commits into from
Jan 25, 2018
Merged

Added scale method to the Service model. #1846

merged 3 commits into from
Jan 25, 2018

Conversation

feliperuhland
Copy link
Contributor

@feliperuhland feliperuhland commented Dec 22, 2017 •
edited
Loading

This should satisfy the request #1734

@feliperuhland
Added scale method to the Service model.
edb9e3c 
Signed-off-by: Felipe Ruhland <felipe.ruhland@gmail.com>
shin-
shin- reviewed Dec 26, 2017
View reviewed changes
Copy link
Contributor

@shin- shin- left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, though I'm wondering if we should check whether the service is in global mode and raise an error if the user tries to rescale a global service. WDYT?

@feliperuhland
Ensure that global containers are not scaled
0e0a852 
Signed-off-by: Felipe Ruhland <felipe.ruhland@gmail.com>
@feliperuhland feliperuhland force-pushed the issue-1734-scale-method branch from 747fd9d to 0e0a852 Compare December 26, 2017 20:30
@feliperuhland
Copy link
Contributor Author

@shin- You're right. I just submitted a validation for it. If you think that the exception message isn't good, please tell me. Thanks.

@feliperuhland
Copy link
Contributor Author

Could not update commit status. Message: Server returned HTTP response code: 201, message: 'Created' for URL: https://api.github.com/repos/docker/docker-py/statuses/0e0a8526801d207bfdf2e9011ab1348525437a79

I think it's a network problem.

shin-
shin- reviewed Jan 2, 2018
View reviewed changes
Copy link
Contributor

@shin- shin- left a comment

Choose a reason for hiding this comment

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

Changes look good, just a small test issue.

tests/integration/models_services_test.py
spec = service.attrs['Spec']['TaskTemplate']['ContainerSpec']
assert spec.get('Command') == ['sleep', '300']

def test_scale_method_service(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is failing because the default mode for a service is global. Make sure to set the initial mode to replicated!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @shin-. I already changed.

@feliperuhland feliperuhland force-pushed the issue-1734-scale-method branch 4 times, most recently from df6274b to 9855dcd Compare January 3, 2018 01:51
feliperuhland
feliperuhland commented Jan 3, 2018
View reviewed changes
tests/integration/models_services_test.py

def test_scale_method_global_service(self):
client = docker.from_env(version=TEST_API_VERSION)
mode = ServiceMode('global')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the default value is replicated.

feliperuhland
feliperuhland commented Jan 3, 2018
View reviewed changes
docker/models/services.py Outdated
``True``if successful.
"""

if not self.attrs['Spec']['Mode'].get('Global'):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conditional is wrong.

Copy link
Contributor Author

@feliperuhland feliperuhland Jan 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In fact, the conditional was very wrong. It returned an empty dictionary and did not raise the exception.

@feliperuhland feliperuhland force-pushed the issue-1734-scale-method branch 2 times, most recently from eeee297 to 41e67c9 Compare January 3, 2018 02:13
@feliperuhland
Fix test to make sure the initial mode is replicated
663c608 
Signed-off-by: Felipe Ruhland <felipe.ruhland@gmail.com>
@feliperuhland feliperuhland force-pushed the issue-1734-scale-method branch from 41e67c9 to 663c608 Compare January 3, 2018 02:22
@shin- shin- added this to the 2.8.0 milestone Jan 3, 2018
shin-
shin- approved these changes Jan 25, 2018
View reviewed changes
Copy link
Contributor

@shin- shin- left a comment

Choose a reason for hiding this comment

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

Obrigado!

@shin- shin- merged commit 4590e2a into docker:master Jan 25, 2018
@shin- shin- modified the milestones: 2.8.0, 3.0 Jan 26, 2018
@lhenry7
Copy link

lhenry7 commented Mar 7, 2018

Hi, I find this new method does not work consistently. I got varying results, 1st time I called it with 1 task already running, service.scale(2) it worked like 'service update -force' and updated the running task and started a 2nd one. But after that, service.scale(3) just didn't work at all, didn't start any new tasks or update existing ones.

I find
mode = docker.types.ServiceMode('replicated', replicas=running_tasks + 1)
service.update(mode=mode, fetch_current_spec=True)
works like 'service scale', so I 'll continue doing that for now.

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

Reviewers

1 more reviewer

@shin- shin- shin- approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@feliperuhland @lhenry7 @shin-