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

Version service specs #2033

Merged
merged 1 commit into from
Mar 21, 2017
Merged

Conversation

aaronlehmann
Copy link
Collaborator

@aaronlehmann aaronlehmann commented Mar 13, 2017

Adds fields to Service and Task that keep track of a version number for ServiceSpecs. This version number is different from the service object version (which is tied to the Raft index).

Then change the scheduler to use this, instead of marshalling the specs to compare them (which didn't work reliably).

Also make the orchestrator use SpecVersion as an optimization, when available.

Note this versioning is not used by the API at this point. It's a purely internal thing.

Supersedes #1392

cc @aluzzardi @dongluochen

@codecov
Copy link

codecov bot commented Mar 13, 2017

Codecov Report

Merging #2033 into master will increase coverage by 0.05%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #2033      +/-   ##
==========================================
+ Coverage   53.39%   53.45%   +0.05%     
==========================================
  Files         111      111              
  Lines       19678    19689      +11     
==========================================
+ Hits        10508    10525      +17     
- Misses       7894     7901       +7     
+ Partials     1276     1263      -13

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b5a580...f59c95f. Read the comment docs.

@aaronlehmann
Copy link
Collaborator Author

@aluzzardi: We were discussing earlier about why SpecVersion doesn't use a Version type, with the version of the service which the spec comes from. One reason is that controlapi wouldn't know what to set here. It has to include SpecVersion when it updates the service, but the updated version information for the service won't be available until after it updates the service. We could use the version number of the service from before the spec is changed, but I feel that this would be confusing. People might try to compare SpecVersion to service.Meta.Version, and they would find that it never matches as expected.

For this reason, I think it's better to use an independent versioning scheme for SpecVersion. And if we're not using Raft indices, we shouldn't use the Version type, which implies a connection to Raft.

service.Spec = *service.PreviousSpec.Copy()
service.SpecVersion = service.PreviousSpecVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put spec_version for a service inside of ServiceSpec? Then you don't need tracking like this.

@aaronlehmann
Copy link
Collaborator Author

aaronlehmann commented Mar 18, 2017 via email

@dongluochen
Copy link
Contributor

LGTM

@aaronlehmann aaronlehmann force-pushed the service-spec-version branch from ac42b98 to 4cb5328 Compare March 20, 2017 18:05
@aaronlehmann
Copy link
Collaborator Author

Rebased

Adds fields to Service and Task that keep track of a version number for
ServiceSpecs. This version number is different from the service object
version (which is tied to the Raft index).

Then change the scheduler to use this, instead of marshalling the specs
to compare them (which didn't work reliably).

Also make the orchestrator use SpecVersion as an optimization, when
available.

Signed-off-by: Aaron Lehmann <[email protected]>
@aaronlehmann aaronlehmann force-pushed the service-spec-version branch from 4cb5328 to f59c95f Compare March 21, 2017 00:48
@aaronlehmann
Copy link
Collaborator Author

Updated to use the Version type for SpecVersion, per a discussion with @aluzzardi. Note that the spec version will get the service's version number from before the new spec is put in place. Thus, the SpecVersion and service Version, won't match, but will always be one generation apart. The upside of doing this is that we can use the same type instead of inventing a new one for spec versions or using bare integers.

ping @aluzzardi @dongluochen

@aluzzardi
Copy link
Member

LGTM

Maybe we should use versions for the control API filter

@aaronlehmann aaronlehmann merged commit 299916c into moby:master Mar 21, 2017
@aaronlehmann aaronlehmann mentioned this pull request Mar 21, 2017
@aaronlehmann aaronlehmann deleted the service-spec-version branch March 21, 2017 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants