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.

Sign up for GitHub

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

Jump to bottom

[CIDRPool 3/x] update ipam-node to support CIDRPool #44

Merged
merged 6 commits into from
Jun 13, 2024
Merged

[CIDRPool 3/x] update ipam-node to support CIDRPool #44

merged 6 commits into from
Jun 13, 2024

Conversation

ykulazhenkov
Copy link
Collaborator

@ykulazhenkov ykulazhenkov commented Jun 11, 2024 •
edited
Loading

This PR contains the following changes:

  • update CNI API to support poolType argument
  • update GRPC API to support poolType argument
  • rename internal IPPool structure to Pool and make it generic
  • make poolmanage/store code generic (mostly by renaming) to be compatible with both pool types
  • update ipam-node allocator package to support CIDRPool use-case: ip address exclusion, /31 and /127 prefixes

@ykulazhenkov ykulazhenkov requested review from vasrem and rollandf June 11, 2024 08:25
@coveralls
Copy link

Coverage Status

coverage: 70.796% (-1.3%) from 72.083%
when pulling 918cbe0 on ykulazhenkov:pr-cidr-pool-node
into 18fc6a0 on Mellanox:main.

@coveralls
Copy link

Coverage Status

coverage: 70.796% (+0.3%) from 70.538%
when pulling 0bba118 on ykulazhenkov:pr-cidr-pool-node
into 3ddd30f on Mellanox:main.

@ykulazhenkov ykulazhenkov changed the title [CIDRPool 4/x] update ipam-node to support CIDRPool [CIDRPool 3/x] update ipam-node to support CIDRPool Jun 11, 2024
@ykulazhenkov
Update allocator pkg to support for /31 and /127 nets
fdf0890 
Signed-off-by: Yury Kulazhenkov <[email protected]>
@ykulazhenkov
Add support for IP exlusion to allocator
db910d7
@ykulazhenkov
Update GRPC API to support PoolType argument
c54f13f 
This argument is required to indicate
type of the pool (IPPool, CIDRPool) which
is referred  by the pool field.

Signed-off-by: Yury Kulazhenkov <[email protected]>
@ykulazhenkov
Add support for poolType CNI argument
653ab95 
Signed-off-by: Yury Kulazhenkov <[email protected]>
@ykulazhenkov
Generalize code to support both pool types in ipam-node
cf4997a 
Signed-off-by: Yury Kulazhenkov <[email protected]>
@coveralls
Copy link

Coverage Status

coverage: 70.081% (+0.08%) from 70.0%
when pulling 9b0b6f6 on ykulazhenkov:pr-cidr-pool-node
into 9531cf5 on Mellanox:main.

vasrem
vasrem reviewed Jun 12, 2024
View reviewed changes
api/grpc/proto/nvidia/ipam/node/v1/node.proto Show resolved Hide resolved
pkg/common/pool_key.go
Comment on lines +19 to +20
// to avoid migration of the store, and to support downgrade
return poolName
Copy link
Member

Choose a reason for hiding this comment

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

👍

pkg/ip/cidr.go Show resolved Hide resolved
pkg/ipam-node/controllers/cidrpool/cidrpool.go Outdated Show resolved Hide resolved
pkg/ipam-node/controllers/cidrpool/cidrpool.go Show resolved Hide resolved
@ykulazhenkov
Add node controller for CIDRPool CR
cdcd33f 
Signed-off-by: Yury Kulazhenkov <[email protected]>
@ykulazhenkov
Copy link
Collaborator Author

Thanks for the review @vasrem. I addressed your comments.

@coveralls
Copy link

Coverage Status

coverage: 68.731% (-1.3%) from 70.0%
when pulling cdcd33f on ykulazhenkov:pr-cidr-pool-node
into 9531cf5 on Mellanox:main.

@ykulazhenkov ykulazhenkov requested a review from vasrem June 12, 2024 10:27
This was referenced Jun 12, 2024
Merged
Merged
Merged
rollandf
rollandf approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@rollandf rollandf left a comment

Choose a reason for hiding this comment

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

LGTM

@ykulazhenkov ykulazhenkov merged commit 8642f37 into Mellanox:main Jun 13, 2024
9 checks passed
@ykulazhenkov ykulazhenkov deleted the pr-cidr-pool-node branch June 13, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rollandf rollandf rollandf approved these changes

@vasrem vasrem vasrem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ykulazhenkov @coveralls @vasrem @rollandf