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

Addressing downscaling pools with addresses in range #117

Closed
christianang opened this issue Apr 12, 2023 · 6 comments · Fixed by #125
Closed

Addressing downscaling pools with addresses in range #117

christianang opened this issue Apr 12, 2023 · 6 comments · Fixed by #125

Comments

@christianang
Copy link
Contributor

Context

We see that pools can be updated without regard for what IPs are in use. This can lead to situations where IPs can become out of range of the pool's configuration.

Potential Solution

We are thinking of adding validation in the webhook that would check if there is an IP address that is already allocated before allowing it to be removed from the pool. This continues to allow configuration of the pool, but prevents an IP Address that is in use to be removed from the pool.

We are also thinking of adding an outOfRange status count on the pool status (similar to #112) to expose potential issues if it already happens to have out of range IPs. This is mostly considering the case of updates to the pool that may have occurred without the webhook validation.

In addition to the outOfRange status count on the pool we could also add an outOfRange status condition on the IPAddress.

@schrej
Copy link
Member

schrej commented Apr 25, 2023

I'm not sure if conditions (or other status) on IPAddress objects is a good idea. It spreads information around quite a bit.

I'd prefer to just add validation, and maybe a status field in case it does happen anyway.

@tylerschultz
Copy link
Contributor

I agree with your position @schrej.

Side note:
At one point, we had imagined a world where it would be possible to make the pool range validation for in use IPs optional, allowing the pool to change range and making some IPs out of range. If the IPAddress were to indicate it's out of range, the capi provider could watch and then recreate the resource associated with the out of range IPAddress, converging to the new pool's ranges. Users familiar with BOSH might expect or like this kind of thing. This idea has received negative reactions from most in our world.

@schrej
Copy link
Member

schrej commented Apr 26, 2023

Is there any interest in this feature? Because if there isn't, then it might make sense to postpone it until someone requires this to be implemented.
If we want to allow downscaling pools, I feel like we need some way of freeing ranges in the pool. Either by just allowing to downscale and marking remaining Addresses as out-of-pool, or by adding a field to the spec that allows to block certain ranges that are currently part of the pool, and some status information on how many blocked addresses are still in use.

@tylerschultz
Copy link
Contributor

@schrej I'm not sure I understand what you're suggesting gets postponed?

These are options that I see:

  1. prevent a pool update that would cause in use IPs from becoming out of range
  2. continue to allow users to update a pool causing in use IPs from becoming out of range. Add some way of communicating there is an issue with out of range IPs, either by pool status somehow, or as suggested earlier add a status to the IPAddress indicating it's out of range of it's pool.
  3. do nothing ? feels like we're leaving users in a lurch.

Are there other options? are you leaning towards any option?

@schrej
Copy link
Member

schrej commented Apr 26, 2023

Sorry, I was thinking about your side note too much I think. I mean that we should postpone a solution for downsizing pools with allocated IPs outside of the pool afterwards.
We should definitely add validation that prevents removing ranges from the pool that still contain addresses.

@tylerschultz
Copy link
Contributor

tylerschultz commented Apr 26, 2023

Got it, we're on the same page now. Thanks.

We think #124 and #125 should cover the validations as discussed. Those PRs are based on #116, so we'll need to rebase those PRs if/when it's merged. Thanks for your patience and thanks for the feedback and reviews. Much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants