pkg/ipam: dynamically fetch the allocatable ipv4 addresses amount from instance limits #10831

mvisonneau · 2020-04-02T15:37:46Z

This PR is a follow-up of #10786

It primarily focuses on AWS, dynamically figuring out how many addresses we could potentially be allocated to an instance. This will ensure the cilium-operator does not go into a IP deficit loop whenever all the IPv4s of have been allocated and used.

IPAM: dynamically fetch the allocatable ipv4 addresses amount from instance limits (AWS)

maintainer-s-little-helper · 2020-04-02T15:37:48Z

Commit 0c7a2a8bbb31504ba2e6b060ffce9e56597936e9 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2020-04-02T15:37:50Z

Please set the appropriate release note label.

maintainer-s-little-helper · 2020-04-02T15:37:51Z

Please set the appropriate release note label.

maintainer-s-little-helper · 2020-04-02T15:38:19Z

Commit 0c7a2a8bbb31504ba2e6b060ffce9e56597936e9 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2020-04-02T15:38:36Z

Commit 0c7a2a8bbb31504ba2e6b060ffce9e56597936e9 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

mvisonneau · 2020-04-02T16:20:22Z

I have just tested it on an EKS cluster in conjuction with #10786.

Only updated the deployment image of the operator:
s/image: docker.io/cilium/operator:v1.7.1/image: docker.io/mvisonneau/cilium-operator:1.7.1-1

The logs have been dropping instantly 👌

joestringer · 2020-04-06T20:10:22Z

Nice!

Looks like Travis is failing (make unit-tests locally), otherwise let us know when you're looking for review on this by switching it out from draft status.

joestringer · 2020-04-06T20:10:28Z

test-me-please

mvisonneau · 2020-04-07T09:48:58Z

thanks @joestringer, I reckon it is ready to be reviewed. As it is including 366b94ef4f4d82196b7dfc4cad69e1c498d3ed10 from #10786 I was waiting for it to be merged before though. Do you want to superseed it?

joestringer · 2020-04-07T16:48:03Z

I think github requires the first PR to be merged first, then to rebase this PR once the first one is merged.

I note that the various checks have a lot of failures, at the very least I think it's worth digging into what Travis has to say as it has a lower false-positive rate.

Glancing at the Cilium-Tests-With-Kernel it seems like an instance of #10821 .

Also the Maintainer's little helper has provided instructions how to sign off your commits, which is a prerequisite for merging code into Cilium.

mvisonneau · 2020-04-08T15:22:32Z

Indeed, I'll have a look into this issue right away! Regarding the commits signoff I reckon that both of them already are. unless I missed something 🤔

joestringer · 2020-04-08T17:31:21Z

Maybe there was a hiccup with the commits signoff checker, they look good to me. It may resolve itself when you rebase / re-push. If this is not the case, we can investigate further.

coveralls · 2020-04-15T19:05:11Z

Coverage increased (+0.02%) to 44.51% when pulling 0682e48 on mvisonneau:ipam/maxAllocateFromLimits into 50d4acb on cilium:master.

mvisonneau · 2020-04-15T19:50:15Z

@joestringer thanks for the merge of #10786 🙇

I rebased this one and fixed the failing unit test, it is now ready to be reviewed.

joestringer · 2020-04-15T19:50:42Z

test-me-please failed to provision https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/18818/

joestringer · 2020-04-24T20:05:30Z

Not sure which of @aanm @tgraf @ungureanuvladvictor would be best situated to look over the latest version of the core changes from this PR.

pkg/ipam/node.go

pkg/aws/eni/node.go

ungureanuvladvictor

LGTM from my side.

ungureanuvladvictor · 2020-04-28T15:45:55Z

pkg/azure/ipam/node.go

+
+// GetMaximumAllocatableIPv4 returns the maximum amount of IPv4 addresses
+// that can be allocated to the instance
+// TODO: If this is necessary for Azure, need to figure out how to get or


The limits for Azure are 256 across a node (also 256 on a NIC). So in total you can every combination but in total across all NICs on a node needs to be max 256.

https://github.com/MicrosoftDocs/azure-docs/blob/master/includes/azure-virtual-network-limits.md#networking-limits---azure-resource-manager for deets on it.

@ungureanuvladvictor @mvisonneau do you want to address this in this PR or do you think it's more appropriate to split out into a separate PR to enable this functionality on the Azure side?

I know this went through many iterations so I approved it so it does not stay in the queue much time.

If @mvisonneau wants to fix this I'm happy to take another look.

oh thanks for the info, I will update it 👍

ungureanuvladvictor · 2020-04-28T15:48:32Z

test-me-please

ungureanuvladvictor · 2020-04-28T20:55:34Z

test-gke

joestringer · 2020-04-29T00:55:15Z

Looks like GKE hit known flake #11105 , also GKE is not required to merge the PR. Safe to ignore.

joestringer

Overall flow and general logging etc looks good.

tgraf · 2020-05-01T08:40:50Z

pkg/azure/ipam/node.go

+// TODO: If this is necessary for Azure, need to figure out how to get or
+// compute the limits
+func (n *Node) GetMaximumAllocatableIPv4() int {
+	return 0


This can just return InterfaceAddressLimit from pkg/azure/types

very cool, thanks for the info! I will configure it and try to work on a few additionnal tests 👍

done, I also added some more tests on both aws and azure functions (TestGetMaximumAllocatableIPv4)

I haven't done any addtional tests on the azure side on how this integrates with the node_manager though.

ungureanuvladvictor · 2020-05-01T13:36:28Z

test-me-please

qmonnet · 2020-05-01T14:41:07Z

test-me-please

pkg/aws/eni/node_test.go

… types limits Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>

qmonnet · 2020-05-01T20:01:52Z

test-me-please

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 2, 2020

maintainer-s-little-helper bot added the dont-merge/needs-release-note label Apr 2, 2020

maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 2, 2020

mvisonneau force-pushed the ipam/maxAllocateFromLimits branch from 0c7a2a8 to 53c96a5 Compare April 2, 2020 15:38

mvisonneau force-pushed the ipam/maxAllocateFromLimits branch from 53c96a5 to 79001e3 Compare April 2, 2020 15:38

joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Apr 6, 2020

maintainer-s-little-helper bot removed the dont-merge/needs-release-note label Apr 6, 2020

mvisonneau force-pushed the ipam/maxAllocateFromLimits branch from 79001e3 to 7071b33 Compare April 10, 2020 09:30

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 10, 2020

mvisonneau force-pushed the ipam/maxAllocateFromLimits branch 4 times, most recently from 275793c to 88d08c1 Compare April 15, 2020 17:47

mvisonneau marked this pull request as ready for review April 15, 2020 19:47

mvisonneau requested a review from a team as a code owner April 15, 2020 19:47

mvisonneau force-pushed the ipam/maxAllocateFromLimits branch 2 times, most recently from 880026e to c3465b7 Compare April 24, 2020 10:05

joestringer requested a review from ungureanuvladvictor April 24, 2020 20:04

ungureanuvladvictor reviewed Apr 25, 2020

View reviewed changes

pkg/ipam/node.go Outdated Show resolved Hide resolved

pkg/aws/eni/node.go Outdated Show resolved Hide resolved

pkg/aws/eni/node.go Outdated Show resolved Hide resolved

mvisonneau force-pushed the ipam/maxAllocateFromLimits branch 3 times, most recently from 0e419bc to 7a79f03 Compare April 27, 2020 13:17

ungureanuvladvictor approved these changes Apr 28, 2020

View reviewed changes

joestringer approved these changes Apr 29, 2020

View reviewed changes

tgraf requested changes May 1, 2020

View reviewed changes

mvisonneau force-pushed the ipam/maxAllocateFromLimits branch from 7a79f03 to 8abf889 Compare May 1, 2020 12:47

qmonnet self-assigned this May 1, 2020

qmonnet requested changes May 1, 2020

View reviewed changes

pkg/aws/eni/node_test.go Show resolved Hide resolved

pkg/aws/eni/node_test.go Outdated Show resolved Hide resolved

Fetch the maximum allocatable IPv4 addresses amount from the instance…

0682e48

… types limits Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>

mvisonneau force-pushed the ipam/maxAllocateFromLimits branch from 8abf889 to 0682e48 Compare May 1, 2020 17:14

qmonnet approved these changes May 2, 2020

View reviewed changes

maintainer-s-little-helper bot requested a review from tgraf May 2, 2020 11:06

ungureanuvladvictor added the integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. label May 2, 2020

tgraf approved these changes May 4, 2020

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 4, 2020

qmonnet merged commit 5cc56f7 into cilium:master May 4, 2020

1.8.0 automation moved this from In progress to Merged May 4, 2020

mvisonneau deleted the ipam/maxAllocateFromLimits branch May 4, 2020 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/ipam: dynamically fetch the allocatable ipv4 addresses amount from instance limits #10831

pkg/ipam: dynamically fetch the allocatable ipv4 addresses amount from instance limits #10831

mvisonneau commented Apr 2, 2020 •

edited

maintainer-s-little-helper bot commented Apr 2, 2020

maintainer-s-little-helper bot commented Apr 2, 2020

maintainer-s-little-helper bot commented Apr 2, 2020

maintainer-s-little-helper bot commented Apr 2, 2020

maintainer-s-little-helper bot commented Apr 2, 2020

mvisonneau commented Apr 2, 2020

joestringer commented Apr 6, 2020

joestringer commented Apr 6, 2020

mvisonneau commented Apr 7, 2020

joestringer commented Apr 7, 2020 •

edited

mvisonneau commented Apr 8, 2020

joestringer commented Apr 8, 2020 •

edited

coveralls commented Apr 15, 2020 •

edited

mvisonneau commented Apr 15, 2020

joestringer commented Apr 15, 2020 •

edited by aanm

joestringer commented Apr 24, 2020

ungureanuvladvictor left a comment

ungureanuvladvictor Apr 28, 2020 •

edited

joestringer Apr 29, 2020 •

edited

ungureanuvladvictor Apr 29, 2020

mvisonneau May 1, 2020

mvisonneau May 1, 2020

ungureanuvladvictor commented Apr 28, 2020

ungureanuvladvictor commented Apr 28, 2020

joestringer commented Apr 29, 2020

joestringer left a comment

tgraf May 1, 2020

mvisonneau May 1, 2020 •

edited

mvisonneau May 1, 2020 •

edited

ungureanuvladvictor commented May 1, 2020

qmonnet commented May 1, 2020

qmonnet commented May 1, 2020

pkg/ipam: dynamically fetch the allocatable ipv4 addresses amount from instance limits #10831

pkg/ipam: dynamically fetch the allocatable ipv4 addresses amount from instance limits #10831

Conversation

mvisonneau commented Apr 2, 2020 • edited

maintainer-s-little-helper bot commented Apr 2, 2020

maintainer-s-little-helper bot commented Apr 2, 2020

maintainer-s-little-helper bot commented Apr 2, 2020

maintainer-s-little-helper bot commented Apr 2, 2020

maintainer-s-little-helper bot commented Apr 2, 2020

mvisonneau commented Apr 2, 2020

joestringer commented Apr 6, 2020

joestringer commented Apr 6, 2020

mvisonneau commented Apr 7, 2020

joestringer commented Apr 7, 2020 • edited

mvisonneau commented Apr 8, 2020

joestringer commented Apr 8, 2020 • edited

coveralls commented Apr 15, 2020 • edited

mvisonneau commented Apr 15, 2020

joestringer commented Apr 15, 2020 • edited by aanm

joestringer commented Apr 24, 2020

ungureanuvladvictor left a comment

Choose a reason for hiding this comment

ungureanuvladvictor Apr 28, 2020 • edited

Choose a reason for hiding this comment

joestringer Apr 29, 2020 • edited

Choose a reason for hiding this comment

ungureanuvladvictor Apr 29, 2020

Choose a reason for hiding this comment

mvisonneau May 1, 2020

Choose a reason for hiding this comment

mvisonneau May 1, 2020

Choose a reason for hiding this comment

ungureanuvladvictor commented Apr 28, 2020

ungureanuvladvictor commented Apr 28, 2020

joestringer commented Apr 29, 2020

joestringer left a comment

Choose a reason for hiding this comment

tgraf May 1, 2020

Choose a reason for hiding this comment

mvisonneau May 1, 2020 • edited

Choose a reason for hiding this comment

mvisonneau May 1, 2020 • edited

Choose a reason for hiding this comment

ungureanuvladvictor commented May 1, 2020

qmonnet commented May 1, 2020

qmonnet commented May 1, 2020

mvisonneau commented Apr 2, 2020 •

edited

joestringer commented Apr 7, 2020 •

edited

joestringer commented Apr 8, 2020 •

edited

coveralls commented Apr 15, 2020 •

edited

joestringer commented Apr 15, 2020 •

edited by aanm

ungureanuvladvictor Apr 28, 2020 •

edited

joestringer Apr 29, 2020 •

edited

mvisonneau May 1, 2020 •

edited

mvisonneau May 1, 2020 •

edited