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

NVIPAMPool CRD #24

Merged
merged 5 commits into from
Sep 12, 2023
Merged

NVIPAMPool CRD #24

merged 5 commits into from
Sep 12, 2023

Conversation

rollandf
Copy link
Member

No description provided.

api/v1alpha1/groupversion_info.go Show resolved Hide resolved
api/v1alpha1/ippool_type.go Show resolved Hide resolved
api/v1alpha1/ippool_type.go Outdated Show resolved Hide resolved
api/v1alpha1/ippool_type.go Show resolved Hide resolved
pkg/ipam-controller/allocator/allocator.go Outdated Show resolved Hide resolved
pkg/ipam-controller/allocator/allocator.go Outdated Show resolved Hide resolved
cmd/ipam-controller/app/app_test.go Outdated Show resolved Hide resolved
cmd/ipam-controller/app/app_test.go Outdated Show resolved Hide resolved
cmd/ipam-controller/app/app_test.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
api/v1alpha1/ippool_type.go Show resolved Hide resolved
cmd/ipam-node/app/app.go Outdated Show resolved Hide resolved
cmd/ipam-controller/app/options/options.go Outdated Show resolved Hide resolved
cmd/ipam-node/app/options/options.go Show resolved Hide resolved
@rollandf rollandf marked this pull request as ready for review August 23, 2023 09:49
@rollandf rollandf force-pushed the pool-crd branch 3 times, most recently from 3964a63 to caa0e7b Compare August 24, 2023 08:47
@ykulazhenkov ykulazhenkov self-requested a review August 28, 2023 08:29
@coveralls
Copy link

Coverage Status

coverage: 77.323% (-0.8%) from 78.142% when pulling caa0e7b on rollandf:pool-crd into 2e7aeab on Mellanox:main.

README.md Outdated Show resolved Hide resolved
cmd/ipam-controller/app/app.go Outdated Show resolved Hide resolved
cmd/ipam-controller/app/app_test.go Show resolved Hide resolved
cmd/ipam-controller/app/options/options.go Outdated Show resolved Hide resolved
examples/ippool-1.yaml Outdated Show resolved Hide resolved
pkg/ipam-controller/controllers/pool/pool.go Outdated Show resolved Hide resolved
pkg/ipam-controller/migrator/migrator.go Outdated Show resolved Hide resolved
pkg/ipam-controller/migrator/migrator.go Outdated Show resolved Hide resolved
pkg/ipam-controller/migrator/migrator.go Show resolved Hide resolved
pkg/ipam-controller/migrator/migrator.go Outdated Show resolved Hide resolved
api/v1alpha1/ippool_type.go Outdated Show resolved Hide resolved
api/v1alpha1/ippool_type.go Outdated Show resolved Hide resolved
pkg/ipam-controller/migrator/migrator_test.go Outdated Show resolved Hide resolved
pkg/ipam-controller/migrator/migrator.go Outdated Show resolved Hide resolved
pkg/ipam-controller/migrator/migrator.go Outdated Show resolved Hide resolved
pkg/ipam-controller/controllers/pool/pool.go Outdated Show resolved Hide resolved
pkg/ipam-node/controllers/ippool/ippool.go Outdated Show resolved Hide resolved
pkg/ipam-node/controllers/ippool/ippool.go Outdated Show resolved Hide resolved
boilerplate.go.txt Outdated Show resolved Hide resolved
pkg/ipam-node/controllers/ippool/ippool.go Show resolved Hide resolved
- Add CRD definition
- Add Make targets
- Add CR examples based on ConfigMap example

Signed-off-by: Fred Rolland <frolland@nvidia.com>
@ykulazhenkov
Copy link
Collaborator

The controller part looks good for me. I added one extra comment about the node part.

Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

a few minor comments, once addressed, LGTM from my side.

m.lock.Lock()
defer m.lock.Unlock()
m.reader = nil
return m.poolByName
Copy link
Collaborator

Choose a reason for hiding this comment

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

locking/unlocking doesnt help us much here
should we create a copy of the map ? or just drop mutex ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

locking/unlocking doesnt help us much here
should we create a copy of the map

Why? We use mutex here to protect shared internal map from concurrent writes

pkg/ipam-controller/allocator/allocator.go Show resolved Hide resolved
pkg/ipam-controller/migrator/migrator.go Show resolved Hide resolved
pkg/ipam-controller/migrator/migrator.go Outdated Show resolved Hide resolved
pkg/ipam-controller/controllers/ippool/ippool.go Outdated Show resolved Hide resolved
Add IPPool controller, watching Pools.
The IP range allocations are available on the CR status.
Node controller triggers event on all Pools when
Node is added/updated/deleted.

Signed-off-by: Fred Rolland <frolland@nvidia.com>
Move from watching Nodes object and read IP range from
annotation, to watch IPPools objects and get Allocations
from their Status.

Signed-off-by: Fred Rolland <frolland@nvidia.com>
In case the IP configuration ConfigMap exists:
- Create IPPools CR according to config spec
- Read Nodes ranges annotation
- Populate the IPpools Status allocations
- Clear Nodes ranges annotation
- Delete ConfigMap

In case an issue is preventing the migration flow,
it can be skipped by setting the env var 'MIGRATOR_DISABLE_MIGRATION'.

Signed-off-by: Fred Rolland <frolland@nvidia.com>
Signed-off-by: Fred Rolland <frolland@nvidia.com>
@ykulazhenkov ykulazhenkov merged commit 1ee40f5 into Mellanox:main Sep 12, 2023
9 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