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

Support for Calico IPAM in KDD mode #1010

Merged
merged 8 commits into from Feb 12, 2019

Conversation

Projects
None yet
3 participants
@caseydavenport
Copy link
Member

caseydavenport commented Feb 4, 2019

Description

Updated version of #816 which brings the branch up to date with master and gets more tests passing.

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Calico IPAM can now be used with the Kubernetes API datastore
@caseydavenport
Copy link
Member Author

caseydavenport left a comment

First pass of comments.

Show resolved Hide resolved lib/apis/v3/blockaffinity.go Outdated
Show resolved Hide resolved lib/apis/v3/blockaffinity.go Outdated
Show resolved Hide resolved lib/apis/v3/blockaffinity.go
Show resolved Hide resolved lib/apis/v3/ipam_block.go Outdated
Show resolved Hide resolved lib/apis/v3/ipam_block.go
Show resolved Hide resolved lib/ipam/ipam_block_reader_writer.go Outdated
Show resolved Hide resolved lib/ipam/ipam_block_reader_writer.go Outdated
Show resolved Hide resolved lib/ipam/ipam_block_reader_writer.go
Show resolved Hide resolved lib/ipam/ipam_test.go Outdated
Show resolved Hide resolved lib/ipam/ipam_test.go Outdated
Show resolved Hide resolved lib/backend/k8s/k8s.go Outdated

@briansan briansan referenced this pull request Feb 5, 2019

Merged

Kdd ipam merge #6

Show resolved Hide resolved lib/backend/model/block.go Outdated

@caseydavenport caseydavenport changed the title WIP: Support for Calico IPAM in KDD mode Support for Calico IPAM in KDD mode Feb 8, 2019

@caseydavenport caseydavenport added this to the Calico v3.6.0 milestone Feb 8, 2019

@caseydavenport caseydavenport force-pushed the caseydavenport:kdd-ipam-merge branch from 47b0e90 to 11cfc8f Feb 8, 2019

Show resolved Hide resolved lib/backend/k8s/k8s.go Outdated

@caseydavenport caseydavenport force-pushed the caseydavenport:kdd-ipam-merge branch 4 times, most recently from 8a9bd59 to 6086654 Feb 11, 2019

@caseydavenport caseydavenport force-pushed the caseydavenport:kdd-ipam-merge branch from 6086654 to 5ad33c8 Feb 12, 2019

Show resolved Hide resolved lib/backend/model/block.go Outdated
Show resolved Hide resolved lib/backend/model/block.go Outdated
@fasaxc
Copy link
Member

fasaxc left a comment

A few minor comments and one thought: should we try to hide the multi-stage deletion logic in the KDD layer rather than having to write code in the IPAM layer to do it? I.e. make it so that DeleteKVP does the CaS and then the delete with precondition.

@briansan

This comment has been minimized.

Copy link
Member

briansan commented Feb 12, 2019

I agree that hiding the logic within KDD implementation of DeleteKVP would be worth it. It would avoid having our etcd implementation go through these unnecessary checks since it is not vulnerable to this potential race. Seems like a pretty small change.. @caseydavenport WDYT

@caseydavenport

This comment has been minimized.

Copy link
Member Author

caseydavenport commented Feb 12, 2019

@briansan yep, I've made that change locally. Will push soon.

@caseydavenport

This comment has been minimized.

Copy link
Member Author

caseydavenport commented Feb 12, 2019

@fasaxc OK, I think it should all be addressed in the latest commit.

Note: I've bumped the "Burst" back up on the k8s client, since for some /32 blocksize allocations we require lots of datastore requests to find an unclaimed block, and to find an empty block. I've left the QPS at the default 5 though, as it seems to work fine.

I'd like to optimize that further later, but it needs more design and I'd rather not hold this PR up further on it.

@caseydavenport caseydavenport merged commit 9ccf86f into projectcalico:master Feb 12, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@caseydavenport caseydavenport deleted the caseydavenport:kdd-ipam-merge branch Feb 12, 2019

@caseydavenport caseydavenport referenced this pull request Feb 12, 2019

Closed

Full support in k8s datastore driver #355

15 of 15 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.