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

Return previous IP allocation for add cmd #471

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

mlguerrero12
Copy link
Collaborator

@mlguerrero12 mlguerrero12 commented May 15, 2024

  • This allows the cni to return a previous allocation for a pod with the same podRef and interface name. This is needed on networks with limited IPs.
  • A new property called Interface was added on the API in order to uniquely identify allocations for pods with multiple interfaces.
  • Testing was improved and new cases (multiple interfaces and reclaim previous allocations) are added.
  • New field does not break backwards compatibility. Users that wish to solve the issue of limited ip reservations in statefulsets will have to recreate the pods to have this new field populated.

@mlguerrero12 mlguerrero12 force-pushed the addifnameapi branch 10 times, most recently from 09439c6 to e57f5d9 Compare May 22, 2024 14:37
@mlguerrero12 mlguerrero12 changed the title WIP - Add ifname to api Return previous IP allocation for add cmd May 22, 2024
@mlguerrero12
Copy link
Collaborator Author

@maiqueb, @dougbtv, @pliurh, please have a look when you have time. Thanks!

Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
@coveralls
Copy link

coveralls commented May 23, 2024

Pull Request Test Coverage Report for Build 9402269686

Details

  • 5 of 22 (22.73%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 71.647%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/whereabouts.go 2 8 25.0%
pkg/allocate/allocate.go 3 14 21.43%
Totals Coverage Status
Change from base Build 9221798376: -0.6%
Covered Lines: 1127
Relevant Lines: 1573

💛 - Coveralls

@pliurh
Copy link

pliurh commented May 30, 2024

@mlguerrero12 I can't find the code for updating the existing IP allocation records. Did you take care of that in this PR?

This is needed in order to uniquely identify allocations
for pods with multiple interfaces. These allocations have
the same podRef.

Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
Support the new Interface property on IP and cluster wide
allocations.

Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
This allows to validate the lifecycle of allocations for
single and multiple interfaces pods.

Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
This allows the cni to return a previous allocation
for a pod with the same podRef and interface name. This
is needed on networks with limited IPs.

Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
@mlguerrero12
Copy link
Collaborator Author

mlguerrero12 commented Jun 4, 2024

@mlguerrero12 I can't find the code for updating the existing IP allocation records. Did you take care of that in this PR?

No, I decided not to add this functionality. It will be more overhead for little gain in my opinion. Everything will continue working, so it does not break backwards compatibility. Users that wish to solve the issue of limited ip reservations in statefulsets will have to recreate the pods to have this new field populated.

@mlguerrero12 mlguerrero12 reopened this Jun 6, 2024
Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you a TON!!!

@dougbtv dougbtv merged commit 638d58d into k8snetworkplumbingwg:master Jun 11, 2024
26 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants