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

Serializing bitseq alloc #1788

Merged
merged 3 commits into from Oct 3, 2017

Conversation

Projects
None yet
6 participants
@abhi
Member

abhi commented May 31, 2017

Previously the bitseq alloc was allocating the first available bit from the beginning of the bit sequence for each sequence handle. With this commit the bitseq alloc will proceed
from the current allocation. This change will affect the way ipam and vni
allocation is done currently. The ip allocation will prcoeeed sequentially from the previous allocation as opposed to the first available IP. So even if the IPs are released it will not be reused until the allocator rolls over to the beginner of the bit sequence.

The commit also contains a fix for byteoffset allocation which plays a role in calculating the allocated ordinal. This scenerio play when the starting ordinal is part of the head sequence block and the available bit is part of subsequent blocks in the bitseq.

Signed-off-by: Abhinandan Prativadi abhi@docker.com

@abhi abhi changed the title from Serializing bitseq alloc to [Not Ready for Review ] Serializing bitseq alloc May 31, 2017

@abhi abhi changed the title from [Not Ready for Review ] Serializing bitseq alloc to Serializing bitseq alloc Jun 5, 2017

Show outdated Hide outdated bitseq/sequence.go Outdated
Show outdated Hide outdated bitseq/sequence.go Outdated
if end < ret {
err = ErrNoBitAvailable
if err == nil {
h.curr = ret + 1

This comment has been minimized.

@fcrisciani

fcrisciani Jun 5, 2017

Member

when the last bit is allocated is there the risk that doing the +1 the number wraps to 0?

@fcrisciani

fcrisciani Jun 5, 2017

Member

when the last bit is allocated is there the risk that doing the +1 the number wraps to 0?

This comment has been minimized.

@abhi

abhi Jun 5, 2017

Member

that will result in failure and cause the allocation from the start=

@abhi

abhi Jun 5, 2017

Member

that will result in failure and cause the allocation from the start=

Show outdated Hide outdated bitseq/sequence.go Outdated
bytePos, bitPos, _ = getFirstAvailable(head, curr)
ret := posToOrdinal(bytePos, bitPos)
if end < ret {
goto begin

This comment has been minimized.

@fcrisciani

fcrisciani Jun 5, 2017

Member

you don't really need a goto here.
If start < ret < end then the result is valid and you can return the result. Else you continue you just continue with the other lookup

@fcrisciani

fcrisciani Jun 5, 2017

Member

you don't really need a goto here.
If start < ret < end then the result is valid and you can return the result. Else you continue you just continue with the other lookup

This comment has been minimized.

@abhi

abhi Jun 5, 2017

Member

avoding multiple condition in if. IMO this would be cleaner.

@abhi

abhi Jun 5, 2017

Member

avoding multiple condition in if. IMO this would be cleaner.

Show outdated Hide outdated bitseq/sequence_test.go Outdated
Show outdated Hide outdated idm/idm_test.go Outdated
Show outdated Hide outdated bitseq/sequence.go Outdated
@trapier

This comment has been minimized.

Show comment
Hide comment
@trapier

trapier Sep 21, 2017

Have not reviewed the code, but very much like the concept. This will provide the gossip control plane maximum time to settle during task churn.

trapier commented Sep 21, 2017

Have not reviewed the code, but very much like the concept. This will provide the gossip control plane maximum time to settle during task churn.

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Sep 21, 2017

Member

@trapier gossip control plane is a transportation mean, does not take any decision on IP matter also the key of every element is the endpoint ID that is unique.
The underlying issue that brings potential IP overlap is due to how swarmkit handles resources, allowing the IP reuse also when a task is in the process to shutdown but still active and running

Member

fcrisciani commented Sep 21, 2017

@trapier gossip control plane is a transportation mean, does not take any decision on IP matter also the key of every element is the endpoint ID that is unique.
The underlying issue that brings potential IP overlap is due to how swarmkit handles resources, allowing the IP reuse also when a task is in the process to shutdown but still active and running

@trapier

This comment has been minimized.

Show comment
Hide comment
@trapier

trapier Sep 22, 2017

Understood @fcrisciani. What I was thinking is if there are many joins and leaves occurring at the same time on a particular network (and despite the fact that it has a lamport clock), numerically ordered ip allocation will reduce the likelihood of same-time same-address events on the control plane.

trapier commented Sep 22, 2017

Understood @fcrisciani. What I was thinking is if there are many joins and leaves occurring at the same time on a particular network (and despite the fact that it has a lamport clock), numerically ordered ip allocation will reduce the likelihood of same-time same-address events on the control plane.

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Sep 22, 2017

Member

@trapier sure, but the problem would be in the application layer, because from a networkdb point of view each single event will have a different endpoint ID so a different key and no collision would be possible

Member

fcrisciani commented Sep 22, 2017

@trapier sure, but the problem would be in the application layer, because from a networkdb point of view each single event will have a different endpoint ID so a different key and no collision would be possible

@mavenugo

Minor Comment. LGTM otherwise.

Can you pls address all other open comments ?

Show outdated Hide outdated ipamapi/labels.go Outdated
@mavenugo

This comment has been minimized.

Show comment
Hide comment
@mavenugo

mavenugo Sep 29, 2017

Contributor

@abhi ping

@fcrisciani are you good with this change ?

Contributor

mavenugo commented Sep 29, 2017

@abhi ping

@fcrisciani are you good with this change ?

abhi added some commits May 31, 2017

Serializing bitseq alloc
Previously the bitseq alloc was allocating the first available bit from the
begining of the sequence. With this commit the bitseq alloc will proceed
from the current allocation. This change will affect the way ipam and vni
allocation is done currently. The ip allocation will be done sequentially
from the previous allocation as opposed to the first available IP.

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>
Minor unit test change
Since bit allocation is no longer first available from
the start some verfications are removed/modified to
the change allocation model

Signed-off-by: Abhinandan Prativadi <abhi@docker.com>
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 29, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@7447e54). Click here to learn what that means.
The diff coverage is 97.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1788   +/-   ##
=========================================
  Coverage          ?   38.02%           
=========================================
  Files             ?      137           
  Lines             ?    27313           
  Branches          ?        0           
=========================================
  Hits              ?    10386           
  Misses            ?    15656           
  Partials          ?     1271
Impacted Files Coverage Δ
drivers/overlay/ov_network.go 0.25% <0%> (ø)
bitseq/store.go 64.89% <100%> (ø)
ipam/allocator.go 69.33% <100%> (ø)
bitseq/sequence.go 81.83% <100%> (ø)
idm/idm.go 76.31% <100%> (ø)
drivers/overlay/ovmanager/ovmanager.go 52.34% <100%> (ø)

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 7447e54...a2bcac0. Read the comment docs.

codecov-io commented Sep 29, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@7447e54). Click here to learn what that means.
The diff coverage is 97.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1788   +/-   ##
=========================================
  Coverage          ?   38.02%           
=========================================
  Files             ?      137           
  Lines             ?    27313           
  Branches          ?        0           
=========================================
  Hits              ?    10386           
  Misses            ?    15656           
  Partials          ?     1271
Impacted Files Coverage Δ
drivers/overlay/ov_network.go 0.25% <0%> (ø)
bitseq/store.go 64.89% <100%> (ø)
ipam/allocator.go 69.33% <100%> (ø)
bitseq/sequence.go 81.83% <100%> (ø)
idm/idm.go 76.31% <100%> (ø)
drivers/overlay/ovmanager/ovmanager.go 52.34% <100%> (ø)

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 7447e54...a2bcac0. Read the comment docs.

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Sep 29, 2017

Member

@mavenugo will give another pass within the eod

Member

fcrisciani commented Sep 29, 2017

@mavenugo will give another pass within the eod

Show outdated Hide outdated idm/idm.go Outdated
Show outdated Hide outdated ipamapi/labels.go Outdated
@GordonTheTurtle

This comment has been minimized.

Show comment
Hide comment
@GordonTheTurtle

GordonTheTurtle Oct 3, 2017

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "ipam_alloc" git@github.com:abhi/libnetwork.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354211816
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

GordonTheTurtle commented Oct 3, 2017

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "ipam_alloc" git@github.com:abhi/libnetwork.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354211816
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Adding a unit case to verify rollover
Signed-off-by: Abhinandan Prativadi <abhi@docker.com>

@GordonTheTurtle GordonTheTurtle removed the dco/no label Oct 3, 2017

@@ -56,7 +56,7 @@ func (i *Idm) GetSpecificID(id uint64) error {
}
// GetIDInRange returns the first available id in the set within a [start,end] range

This comment has been minimized.

@fcrisciani

fcrisciani Oct 3, 2017

Member

I don't think that this is anymore accurate, correct? (being a comment it's not critical, can be handled on top)

@fcrisciani

fcrisciani Oct 3, 2017

Member

I don't think that this is anymore accurate, correct? (being a comment it's not critical, can be handled on top)

i.Release(52)
err = i.GetSpecificID(52)
if err != nil {

This comment has been minimized.

@fcrisciani

fcrisciani Oct 3, 2017

Member

can you check that is also 52 the value?

@fcrisciani

fcrisciani Oct 3, 2017

Member

can you check that is also 52 the value?

@fcrisciani

This comment has been minimized.

Show comment
Hide comment
@fcrisciani

fcrisciani Oct 3, 2017

Member

2 small comments, rest LGTM
@mavenugo PTAL

Member

fcrisciani commented Oct 3, 2017

2 small comments, rest LGTM
@mavenugo PTAL

@fcrisciani fcrisciani merged commit 2154459 into docker:master Oct 3, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
dco-signed All commits are signed

mavenugo added a commit that referenced this pull request Oct 4, 2017

Merge pull request #1967 from abhi/pick1788
Backport Serializing bitseq alloc (#1788)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment