feat: enable watch based route reconciliation by default #1112
Conversation
lukasmetzner
temporarily deployed
to
e2e-robot
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1112 +/- ##
==========================================
+ Coverage 68.53% 68.61% +0.07%
==========================================
Files 23 23
Lines 2619 2619
==========================================
+ Hits 1795 1797 +2
Misses 651 651
+ Partials 173 171 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lukasmetzner
temporarily deployed
to
e2e-robot
GitHub Actions
Inactive
lukasmetzner
had a problem deploying
to
e2e-robot
GitHub Actions
Error
lukasmetzner
force-pushed
the
deps-k8s-v1.35
branch
from
f0e4ebf to
2f45a54
Compare
lukasmetzner
temporarily deployed
to
e2e-robot
GitHub Actions
Inactive
lukasmetzner
temporarily deployed
to
e2e-robot
GitHub Actions
Inactive
lukasmetzner
changed the title
lukasmetzner
changed the title
| } | ||
| } | ||
|
|
||
| func TestRouteDeleteCorrectRoutes(t *testing.T) { |
There was a problem hiding this comment.
The test can't work this way, as we no longer reconcile in a fixed interval. Normally these cleanups would either be directly happening in case of a node delete event, or if a stale route exists, it would be removed within 12 - 24 hours.
We might need to build a different testing setup to add or delete nodes from the cluster to test this behavior. Adding nodes is already covered by the cluster creation and the TestRouteNetworksPodIPsAreAccessible.
There was a problem hiding this comment.
Ok, if the rest of the tests are covering this in some way, I am ok with removing this, but it feels weird to have to remove tests while enabling that feature by default in the same time.
Are we trying to rush things here?
There was a problem hiding this comment.
Good point, maybe we can wait with this and not rush it now.
There was a problem hiding this comment.
I will take a look again later and decide.
There was a problem hiding this comment.
One idea for this test:
- Manually create Routes for a made up server
- Create
Nodeobject for made up node, with provider id and pod cidr set - HCCM will reconcile the node and delete it
- All the events to nodes will trigger a reconcile of the routes.
jooola
left a comment
jooola
left a comment
There was a problem hiding this comment.
I don't know enough to fully approve those changes, but the code change looks good.
### Watch-Based Route Reconciliation
Previously, route reconciliation is performed at a fixed interval of
30s. This leads to unnecessary API requests, as a `GET
/v1/networks/{id}` call is triggered every 30s, even when no changes
have occurred.
Upstream, we have contributed an event-driven approach, similar to the
mechanisms used by other controllers such as the Load Balancer
controller. With this new approach, route reconciliation is triggered by
node additions, node deletions, or changes to a node’s `PodCIDRs` or
`Addresses`. Additionally, to ensure consistency, reconciliation still
occurs periodically at a randomized interval between 12 and 24 hours.
#### Enabled by default
This feature is now **enabled by default**.
If you encounter any problems you can disable the feature by setting the
following Helm value:
`args.feature-gates=CloudControllerManagerWatchBasedRoutesReconciliation=false`
### Global Load Balancer Defaults
Configure cluster-wide defaults for Load Balancers via the extended
`HCLOUD_LOAD_BALANCERS_*` env vars. These values automatically apply
during Load Balancer creation and reconciliation whenever annotations
are omitted. Learn more about it in the [reference
documentation](docs/reference/load_balancer_envs.md)
### Features
- extend environment variables for default load balancer configuration
(#1052)
- enable watch based route reconciliation by default (#1112)
This will enable the watch based route reconciliation by default. We already mentioned this feature gate in v1.27.0