subnet routes (fixed, completed) #204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenjaminLudwigSAP
force-pushed
the
subnet-routes
branch
from
e2a3940
to
c53ce1d
Compare
This PR introduces l2 generation and update for interface routes for subnets of a network that have no self-ip. This allows having pool members from foreign subnets without the need to route via default gw and allow setups like no-snat.
BenjaminLudwigSAP
force-pushed
the
subnet-routes
branch
from
c53ce1d
to
69fdc51
Compare
BenjaminLudwigSAP
changed the title
BenjaminLudwigSAP
force-pushed
the
subnet-routes
branch
from
69fdc51
to
be0b327
Compare
- Provision subnet routes to Common tenant, because
- Tenant deletion happens before L2 cleanup and fails when routes
are still present in the tenant
- A small caveat is that we can't filter by regex or wildcard when
fetching routes from the BigIP via iControl REST, so we need to
filter after fetching.
- Rename methods to something with 'subnet routes' instead of 'static
routes'. The routes are static insofar as they are statically
configured, but so are other routes provisioned by the F5 provider
driver, so 'static' is not needed.
- A separate CleanupSubnetRoutes task is needed since SyncSubnetRoutes
does not know whether the network is to be deleted (i. e. whether it
was called from remove_l2) and happily creates routes for all
subnets of a network at network tenant deletion.
- Fix: SyncSubnetRoutes creates only one single subnet route due to
unconditional return statement in the for loop.
- Fix: Missing RaisesIControlRestError decorator on
CleanupDefaultRoute execute method
- Simplify subnet route sync logic
TODO:
- Remove orphaned subnet routes in L2SyncManager.full_sync
- SelfIPs and subnet routes are mutually exclusive. Their creation and
deletion has to be coordinated, respectively
BenjaminLudwigSAP
force-pushed
the
subnet-routes
branch
from
be0b327
to
15a5177
Compare
notandy
requested changes
Mar 3, 2023
Ignore existing and needed route immediately without iterating over the rest of the needed subnet routes. This does not change behavior as long as each subnet ID appears only once in the list of subnets, which we know is the case.
existing_route does not have a 'path' key. This error was introduced in #196
Same functionalty as before, but without changing the array that is being iterated and a bit more compact.
|
I've reproduced that the feature is suffering from the bug, that happens when adding a load-balancer to a subnet that already got a static subnet route. But so far it looks good. |
notandy
approved these changes
Mar 10, 2023
- L2SyncManager:
- Rename sync_l2_selfips_flow to
sync_l2_selfips_and_subnet_routes_flow.
I may in a future PR rename this again to something that makes the
dichotomy more clear between provisioning all l2 objects and only
SelfIPs and subnet routes. Maybe something along the lines of
sync_l2_network_scope vs. sync_l2_subnet_scope, we'll see.
- If there are no SelfIPs to provision (i.e. selfips is empty) we
don't return immediately anymore, but instead provision subnet
routes for all subnets. This gives us the least surprising behavior
in the event that no SelfIP ports were created.
- _do_sync_l2_selfips_and_subnet_routes_flow:
Don't calculate SelfIPs to add/remove here. That calculation has
to happen in the cleanup_selfips_and_subnet_routes and
ensure_selfips_and_subnet_routes flows, respectively. This is so
that selection of which subnet routes to delete can happen in the
CleanupSubnetRoutes task. This avoids code duplication because
that task is also called from remove_l2, but to delete ALL subnet
routes of a network.
- F5Flows: Split up sync_l2_selfips into
cleanup_selfips_and_subnet_routes for deletion and
ensure_selfips_and_subnet_routes for creation
- F5Tasks:
- Rename SyncSubnetRoutes to EnsureSubnetRoutes
- Separate subnet_in_selfips sub-function out of EnsureSubnetRoutes
to reuse it in both EnsureSubnetRoutes and CleanupSubnetRoutes
- CleanupSubnetRoutes: Clean up either all subnet routes or only the
unneeded ones. The former case is used by F5Flows.remove_l2
- Calculation of which subnet routes are needed was wrong: All subnets of all networks were considered. This means that orphaned subnet routes of still existing subnet routes were not deleted, even if the subnet doesn't have LBs anymore. - continue statement was in nested for-loop. This means that no subnet routes were really skipped, the for-loop did nothing at all. This in turn means that all subnet routes were deleted at this point during full_sync and then recreated later when full_sync executes ensure_l2_flow.
notandy
approved these changes
Mar 27, 2023
There was a problem hiding this comment.
I've added minor comments, but in general it seems to work fine.
Afaik, _do_sync_l2_selfips_and_subnet_routes_flow can be completely embedded (and have therefor less boilerplate) in ensure_selfips_and_subnet_routes and is not reused elsewhere, but it's fine to do it another time.
|
I've opened #211 for future L2 refactoring. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes and completes #196 and #202.
This will be followed up by - but is independent of - refactoring PR #205.
Closes #207