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

eni: Fix releases of excess IPs #9858

Merged
merged 1 commit into from Jan 13, 2020
Merged

eni: Fix releases of excess IPs #9858

merged 1 commit into from Jan 13, 2020

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Jan 13, 2020

The release of excess IPs has been incorrect due to not taking into account the
max-above-watermark limit in combination with min-allocate. This bug was hidden
in the unit test as min-allocate was set to a value equal to the max IP limit
of the interface which rendered the value of max-above-watermark (4) to never
be taken into account as min-allocate had already maxed out the interface limit.

Fix the calculation of excess IPs to never fall below min-allocate +
max-above-watermark and change the unit tests to cover this scenario.

This fixes a bug where IPs would always be immediately released again if
min-allocate was greater than pre-allocate and the number of used IPs did not
make up for that gap.


This change is Reviewable

The release of excess IPs has been incorrect due to not taking into account the
max-above-watermark limit in combination with min-allocate. This bug was hidden
in the unit test as min-allocate was set to a value equal to the max IP limit
of the interface which rendered the value of max-above-watermark (4) to never
be taken into account as min-allocate had already maxed out the interface limit.

Fix the calculation of excess IPs to never fall below min-allocate +
max-above-watermark and change the unit tests to cover this scenario.

This fixes a bug where IPs would always be immediately released again if
min-allocate was greater than pre-allocate and the number of used IPs did not
make up for that gap.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf added kind/bug This is a bug in the Cilium logic. pending-review area/eni Impacts ENI based IPAM. labels Jan 13, 2020
@tgraf tgraf requested a review from a team as a code owner January 13, 2020 13:25
@tgraf tgraf added this to In progress in 1.7.0 via automation Jan 13, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

2 similar comments
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@tgraf tgraf added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 13, 2020
@tgraf
Copy link
Member Author

tgraf commented Jan 13, 2020

test-me-please

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 45.932% when pulling a10d5c9 on pr/tgraf/fix-eni-release into 4f6184b on master.

@tgraf tgraf removed the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 13, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@tgraf tgraf added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 13, 2020
@aanm aanm removed the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 13, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@aanm aanm added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Jan 13, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.6.6 Jan 13, 2020
@aanm aanm merged commit dd38573 into master Jan 13, 2020
1.7.0 automation moved this from In progress to Merged Jan 13, 2020
@aanm aanm deleted the pr/tgraf/fix-eni-release branch January 13, 2020 18:16
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.6 in 1.6.6 Jan 24, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.6 to Backport done to v1.6 in 1.6.6 Jan 24, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.6 to Backport done to v1.6 in 1.6.6 Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/eni Impacts ENI based IPAM. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.6.6
Backport done to v1.6
1.7.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants