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

Pools show usage counts #116

Merged
merged 1 commit into from
May 4, 2023
Merged

Conversation

tylerschultz
Copy link
Contributor

InClusterIPPool and GlobalInClusterIPPool receive updates to their status showing the Total count of IPs in the pool's range, the InUse count, and the Free count.

IPv6 IP ranges can be enormous (2^128) IPs. Range totals that exceed int are shown as the value of math.MaxInt (arch dependent).

Adds a new reconciler for pools.
Adds columns to pools for Total, Free, Used

Remove Zap logger remnants.

Fixes #112

@tylerschultz tylerschultz requested a review from schrej as a code owner April 11, 2023 21:52
@tylerschultz tylerschultz force-pushed the pool-reconciler branch 3 times, most recently from 6783e49 to f811b8e Compare April 11, 2023 22:16
@tylerschultz
Copy link
Contributor Author

We've rebased, and resolved test failures that resulted.

We see that the new controller causes Claims to be reconciled. The scenario where a claim is paused needed special handling. The 'filter' for claims that are not paused are somehow bypassed by the changes the new controller makes to the pool. We've added a new pause check on the claim, which seems correct in any case.

We'll spend some time this afternoon manually testing the pause functionality along with these new usage counts and report back later.

@tylerschultz
Copy link
Contributor Author

Ok, we think this commit is in a good place to merge. The controllers look well behaved.

@schrej
Copy link
Member

schrej commented Apr 25, 2023

Since the IPAddressClaim controller has a watch on the pools, and requeues all claims for reconciliation whenever they are updated, this change now causes all claims to be reconciled whenever a new claim is created.
new claim -> status update on pool -> all claims reconciled.
Can we change the watches on the IPAddressClaim controller so it only queues when there is a change to the spec to avoid unnecessary reconciliations?
We can do that in a separate PR though.

lgtm apart from that.

@christianang
Copy link
Contributor

christianang commented Apr 25, 2023

Yeah, that makes sense. Seems simple enough to just do in this PR. We will make an update.

Edit: Looking more closely at the watch for the pool. I think what we want is it only queues a change when the pool is unpaused (similar to what we were doing for Cluster pause). I don't think we care about any other updates to the pool itself when reconciling IPAddressClaims.

@christianang christianang force-pushed the pool-reconciler branch 2 times, most recently from 2143ede to 22d81c5 Compare April 25, 2023 22:04
@tylerschultz
Copy link
Contributor Author

That was a great catch @schrej. We've updated the PR to prevent all of the pool's claims from reconciling when the pool status is updated.

Copy link
Member

@schrej schrej left a comment

Choose a reason for hiding this comment

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

lgtm with a few nits and one suggestion

internal/controllers/ipaddressclaim.go Outdated Show resolved Hide resolved
internal/controllers/ipaddressclaim.go Outdated Show resolved Hide resolved
api/v1alpha1/inclusterippool_types.go Outdated Show resolved Hide resolved
@tylerschultz
Copy link
Contributor Author

I think we've addressed the last round of feedback, standing by.

@tylerschultz tylerschultz force-pushed the pool-reconciler branch 2 times, most recently from a55e6b9 to d920352 Compare April 27, 2023 23:11
api/v1alpha1/inclusterippool_types.go Outdated Show resolved Hide resolved
InClusterIPPool and GlobalInClusterIPPool receive updates to their
status showing the Total count of IPs in the pool's range, the InUse count,
and the Free count.

IPv6 IP ranges can be enormous (2^128) IPs. Range totals that exceed int
are shown as the value of math.MaxInt (arch dependent).

Adds a new reconciler for pools.
Adds columns to pools for Total, Free, Used

Remove Zap logger remnants.

Co-authored-by: Edwin Xie <[email protected]>
Co-authored-by: Christian Ang <[email protected]>
Co-authored-by: Aidan Obley <[email protected]>
@tylerschultz
Copy link
Contributor Author

Hi Jakob, we think the PR is once again ready for another round of review.

@schrej schrej merged commit d8e755e into kubernetes-sigs:main May 4, 2023
@schrej
Copy link
Member

schrej commented May 4, 2023

Thank you!

@tylerschultz tylerschultz deleted the pool-reconciler branch May 11, 2023 17:16
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.

Pool usage metrics
3 participants