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

Allow the blockSize to be configured for Calico IPAM #931

Merged
merged 1 commit into from Oct 2, 2018

Conversation

Projects
None yet
2 participants
@tomdee
Contributor

tomdee commented Sep 26, 2018

Allow the blockSize to be configured for Calico IPAM.

This removes the current fixed block size of /26 (for ipv4) or /122 (for IPv6).
The previous fixed values, now just become defaults.

The block size can be set per-pool, but once set it can't be changed.

New Feature: Allow the blockSize to be configured for Calico IPAM.

@tomdee tomdee force-pushed the tomdee:block-size branch 6 times, most recently from 068a66d to ffc2dc0 Sep 26, 2018

@tomdee tomdee changed the title from WIP: Allow blockSize for IPAM to be configued on IPPool to Allow the blockSize to be configured for Calico IPAM Oct 1, 2018

@tomdee tomdee force-pushed the tomdee:block-size branch 7 times, most recently from 861635f to 0524cba Oct 1, 2018

Show resolved Hide resolved lib/clientv3/ippool.go
if new.Spec.BlockSize > 32 || new.Spec.BlockSize < 20 {
errFields = append(errFields, cerrors.ErroredField{
Name: "IPPool.Spec.BlockSize",
Reason: "IPPool BlockSize must be between 20 and 32",

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

Might want to say "IPv4 block size must be..."

@@ -34,28 +34,32 @@ import (
"github.com/projectcalico/libcalico-go/lib/watch"
)

var _ = testutils.E2eDatastoreDescribe("IPPool tests", testutils.DatastoreAll, func(config apiconfig.CalicoAPIConfig) {
var _ = testutils.E2eDatastoreDescribe("IPPool tests", testutils.DatastoreEtcdV3, func(config apiconfig.CalicoAPIConfig) {

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

Hm, I guess there is now some IPAM code that gets hit by these tests?

I think we should be aiming to keep this set to DatastoreAll if possible. Can we pull out the bits that only work for etcd into a separate set of tests?

I guess CRUD on IPPools now lists IPAM blocks, which makes this fail for KDD? If so, we should track adding this back somewhere as part of implementing KDD IPAM so we don't lose these.

@@ -773,8 +792,15 @@ func (c ipamClient) ClaimAffinity(ctx context.Context, cidr net.IPNet, host stri
// its affinity will not be released and no error will be returned.
// If an empty string is passed as the host, then the hostname is automatically detected.
func (c ipamClient) ReleaseAffinity(ctx context.Context, cidr net.IPNet, host string) error {
// Verify the requested CIDR falls within a configured pool.
pool := c.blockReaderWriter.withinConfiguredPools(net.IP{IP: cidr.IP})

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

I think we should rename this function from withinConfiguredPools to something like getPoolForIP or something like that to match its new behavior.

@@ -1165,13 +1193,15 @@ func (c ipamClient) decrementHandle(ctx context.Context, handleID string, blockC
// GetAssignmentAttributes returns the attributes stored with the given IP address
// upon assignment.
func (c ipamClient) GetAssignmentAttributes(ctx context.Context, addr net.IP) (map[string]string, error) {
blockCIDR := getBlockCIDRForAddress(addr)
obj, err := c.client.Get(ctx, model.BlockKey{blockCIDR}, "")
pool := c.blockReaderWriter.withinConfiguredPools(addr)

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

Don't we need to check if the pool is nil?

return allocationBlock{&b, numAddresses}
}

func getNumAddresses(cidr cnet.IPNet, blockSize int) int {

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

I think this guy could use a comment.

}

func newBlock(cidr cnet.IPNet) allocationBlock {
func newBlock(cidr cnet.IPNet, blockSize int) allocationBlock {

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

I'm a bit confused why we need both cidr and blockSize here, especially since it looks like all the calling code calculates blockSize using cidr.Mask.Size()

@@ -63,7 +64,8 @@ func (rw blockReaderWriter) getAffineBlocks(ctx context.Context, host string, ve
ids = append(ids, k.CIDR)
} else {
for _, pool := range pools {
if pool.Contains(k.CIDR.IPNet.IP) {
poolNet := cnet.MustParseCIDR(pool.Spec.CIDR)

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

I think we want to avoid using MustParseCIDR in production code - IIRC it was a testing tool?

If it fails to parse it will panic. It shouldn't ever happen, since we validate the CIDR, but I think still best to handle this gracefully and avoid panics in libcalico.

}

// No blocks found.
//return cnet.IPNet{}, errors.New(fmt.Sprintf("no blocks found containing IP %s", ip.String()))

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

Can drop this commented line

@tomdee tomdee force-pushed the tomdee:block-size branch 5 times, most recently from f3cb2b6 to 8f8f6c2 Oct 2, 2018

@caseydavenport caseydavenport added this to the Calico v3.3.0 milestone Oct 2, 2018

@caseydavenport

A couple more thoughts, but this LGTM.

Let's get it merged so it will start running through downstream CI.

// Wrap the backend AllocationBlock struct so that we can
// attach methods to it.
type allocationBlock struct {
*model.AllocationBlock
numAddresses int

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

Not a big deal, but couldn't we do this without storing this information in a new field?

e.g. just call numAddresses(..) on the block cidr whenever we need this information or expose a function that can calculate this on the fly? Doesn't seem too computationally complex.

func randomBlockGenerator(pool cnet.IPNet, hostName string) func() *cnet.IPNet {
func randomBlockGenerator(ipPool *v3.IPPool, hostName string) func() *cnet.IPNet {
_, pool, err := cnet.ParseCIDR(ipPool.Spec.CIDR)
if err != nil {

This comment has been minimized.

@caseydavenport

caseydavenport Oct 2, 2018

Member

ick, I guess we should log an error in this case so if somehow we do get here we'll see it in the logs.

@tomdee tomdee force-pushed the tomdee:block-size branch from 8f8f6c2 to 8c1c096 Oct 2, 2018

Feature: Add configurable block size for IPAM
Allow the blockSize to be configured for Calico IPAM.

This removes the current fixed block size of /26 (for ipv4) or /122 (for IPv6).
The previous fixed values, now just become defaults.

The block size can be set per-pool, but once set it can't be changed.

@tomdee tomdee force-pushed the tomdee:block-size branch from 8c1c096 to a7b2a35 Oct 2, 2018

@tomdee tomdee merged commit 3f53d6e into projectcalico:master Oct 2, 2018

2 checks passed

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

@tomdee tomdee deleted the tomdee:block-size branch Oct 2, 2018

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