resolve the bug when cluster CIDR is not /8 #19580
Conversation
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
1 similar comment
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
|
Labelling this PR as size/XS |
k8s-github-robot
added
the
size/XS
|
|
||
| cidrSize, _ := nc.clusterCIDR.Mask.Size() | ||
|
|
||
| // nc.maxCIDRs = int(math.Mod((256-cidr[0])*2^24 + (256-cidr[1])*2^16 + (256-cidr[2])*2^8, 2^(32-cidrSize)) / (2^8)) |
There was a problem hiding this comment.
I don't really get a reasoning for this formula. Isn't it 2^(24 - cidrSize) assuming that clusterCIDR is valid, i.e. (IP & ~Mask == 0)?
There was a problem hiding this comment.
I think the old code let you allocate subnets from some point in the middle of the CIDR and only go forward, which might be neat for some, but does not really meet the established definition of which range CIDR notation represents exactly. :-) I'm assuming this was an attempt to replicate that non-standard behaviour. #19242 is the way to go, as it does things the proper way and supports non-/24 pod allocations.
There was a problem hiding this comment.
The logic itself is short, so instead of going nuclear with #19242 we should fix the logic here if needed.
There was a problem hiding this comment.
ok, i could fix it and just make it simple as 2^(24 - cidrSize)
Thanks
|
see #19242 |
|
@mqliang - too bad I wasn't aware of your work. By the quick look it seems to be similar to the first version of @WeixuZhuang PR. |
|
LGTM |
We will have the rigth formula to generate correct maxCIDRs now. Previous code assume cluster CIDR is /8 which may not be true. Now it generates maxCIDR based on the info of cluster IP.
|
That would be enough, I have something very similar working on my setup. Thanks! |
|
ok to test |
|
GCE e2e test build/test passed for commit 2af68d2. |
|
The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label? |
gmarek
added
lgtm
|
@k8s-bot test this Tests are more than 48 hours old. Re-running tests. |
|
GCE e2e build/test failed for commit 2af68d2. |
|
Hi, may I know why this PR has not get merged? Thanks |
|
It's a flake. @k8s-bot test this. |
|
Plus merge queue is currently blocked as we had some regression during the weekend. I'm looking into it now. |
|
GCE e2e test build/test passed for commit 2af68d2. |
|
@k8s-bot test this Tests are more than 48 hours old. Re-running tests. |
|
GCE e2e test build/test passed for commit 2af68d2. |
resolve the bug when cluster CIDR is not /8
UPSTREAM: 63321: kubelet: force filterContainerID to empty string when removeAll is true Origin-commit: 81f11178a4b644a95f3d8c489509648e9152e5e5
Ref #17754
We will have the rigth formula to generate correct maxCIDRs now.
Previous code assume cluster CIDR is /8 which may not be true.
Now it generates maxCIDR based on the info of cluster IP.