OCPBUGS-77344: enable dcm on route changes#778
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-77344, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPer-endpoint Weight and VerifyHostname fields are added and persisted in a route EndpointTable. Router uses the table for dynamic add/update/replace/remove. Config-manager reconciles endpoints by ID (add→update→delete) with updated health-check enable/disable logic. Backend APIs now accept endpoint pointers, support replace flows, and expose explicit health-setting commands; tests updated accordingly. ChangesEndpoint-Driven Server Management Refactor
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-77344, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (iamin@redhat.com), skipping review request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-77344, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (iamin@redhat.com), skipping review request. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/assign @gcs278 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/router/template/router.go (2)
886-896: 💤 Low valueConsider using
oldConfig.EndpointTable[serviceKey]for removed service endpoints.When removing a service from a route, using
service.EndpointTablerelies on the invariant that the ServiceUnit's endpoint table matches what was previously synced to this backend. UsingoldConfig.EndpointTable[serviceKey]would be more explicit about removing exactly what was added to this specific backend.While the state should normally be consistent due to locking, using the old config's endpoint table would be more defensive.
♻️ Suggested change
for serviceKey := range oldConfig.ServiceUnits { - if service, ok := r.findMatchingServiceUnit(serviceKey); ok { - if _, newFound := newConfig.ServiceUnits[serviceKey]; !newFound { - // Service was removed, remove the endpoints. - if err := r.dynamicConfigManager.RemoveRouteEndpoints(backendKey, service.EndpointTable); err != nil { + if _, newFound := newConfig.ServiceUnits[serviceKey]; !newFound { + if service, ok := r.findMatchingServiceUnit(serviceKey); ok { + // Service was removed from route, remove the endpoints that were in the old config. + if err := r.dynamicConfigManager.RemoveRouteEndpoints(backendKey, oldConfig.EndpointTable[serviceKey]); err != nil { log.Info("router will reload due to error removing endpoints", "backendKey", backendKey, "serviceKey", serviceKey, "error", err) failed = true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/router/template/router.go` around lines 886 - 896, The code currently passes service.EndpointTable when removing endpoints for services present in oldConfig.ServiceUnits but no longer in newConfig; instead, use the exact endpoint table that was part of the previous route config to be defensive. Update the removal call in the loop that iterates oldConfig.ServiceUnits (around findMatchingServiceUnit and the RemoveRouteEndpoints call) to use oldConfig.EndpointTable[serviceKey] (or the appropriate map/key in oldConfig) rather than service.EndpointTable, preserving backendKey and serviceKey when calling r.dynamicConfigManager.RemoveRouteEndpoints.
957-972: 💤 Low valueMisleading weight comparison in log message.
The log at line 966 compares
oldWeight(fromcfg.ServiceUnitNames, the calculated per-endpoint weight) withnewWeight(fromcfg.ServiceUnits, the route-level service weight). These values have different meanings and scales, making the log confusing for debugging.The actual replacement logic is correct since it compares the full endpoint structs including their Weight fields.
♻️ Suggested change for clearer logging
- log.V(4).Info("updating weight for sibling service due to endpoint change", "service", otherServiceKey, "backendKey", backendKey, "oldWeight", oldWeight, "newWeight", newWeight) + log.V(4).Info("updating sibling service endpoints due to weight redistribution", "service", otherServiceKey, "backendKey", backendKey)Alternatively, compare the actual endpoint weights:
oldEpWeight := int32(0) newEpWeight := int32(0) if len(oldOtherEndpoints) > 0 { oldEpWeight = oldOtherEndpoints[0].Weight } if len(newOtherEndpoints) > 0 { newEpWeight = newOtherEndpoints[0].Weight } log.V(4).Info("updating sibling service endpoints", "service", otherServiceKey, "backendKey", backendKey, "oldEpWeight", oldEpWeight, "newEpWeight", newEpWeight)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/router/template/router.go` around lines 957 - 972, The log message in the loop that updates sibling services is misleading because it compares oldWeight (from oldWeights/cfg.ServiceUnitNames per-endpoint weight) to newWeight (cfg.ServiceUnits route-level weight); change the log to report endpoint-level weights instead: read the first endpoint Weight from oldOtherEndpoints and newOtherEndpoints (guarding for empty slices) and log those as oldEpWeight and newEpWeight in the log.V(4).Info call inside the loop that references otherServiceKey, backendKey, oldOtherEndpoints, newOtherEndpoints and the ReplaceRouteEndpoints error path so the message reflects actual endpoint weight changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/router/template/router.go`:
- Around line 886-896: The code currently passes service.EndpointTable when
removing endpoints for services present in oldConfig.ServiceUnits but no longer
in newConfig; instead, use the exact endpoint table that was part of the
previous route config to be defensive. Update the removal call in the loop that
iterates oldConfig.ServiceUnits (around findMatchingServiceUnit and the
RemoveRouteEndpoints call) to use oldConfig.EndpointTable[serviceKey] (or the
appropriate map/key in oldConfig) rather than service.EndpointTable, preserving
backendKey and serviceKey when calling
r.dynamicConfigManager.RemoveRouteEndpoints.
- Around line 957-972: The log message in the loop that updates sibling services
is misleading because it compares oldWeight (from
oldWeights/cfg.ServiceUnitNames per-endpoint weight) to newWeight
(cfg.ServiceUnits route-level weight); change the log to report endpoint-level
weights instead: read the first endpoint Weight from oldOtherEndpoints and
newOtherEndpoints (guarding for empty slices) and log those as oldEpWeight and
newEpWeight in the log.V(4).Info call inside the loop that references
otherServiceKey, backendKey, oldOtherEndpoints, newOtherEndpoints and the
ReplaceRouteEndpoints error path so the message reflects actual endpoint weight
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1a3caf78-e491-44da-b467-71d20f596926
📒 Files selected for processing (6)
pkg/router/template/configmanager/haproxy/backend.gopkg/router/template/configmanager/haproxy/backend_test.gopkg/router/template/configmanager/haproxy/blueprint_plugin_test.gopkg/router/template/configmanager/haproxy/manager.gopkg/router/template/router.gopkg/router/template/types.go
jcmoraisjr
force-pushed
the
OCPBUGS-77344-dcm-update-route
branch
from
a5ac103 to
5dbcbdf
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/router/template/router.go (1)
982-996:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove endpoints per route, not from the full ServiceUnit table.
This helper now sends
service.EndpointTableto every associated backend. If two routes select different target ports on the same service, Line 995 will attempt to delete endpoints that were never configured for some of those backends, which turns a normal endpoint deletion into a forced reload.Proposed fix
-func (r *templateRouter) dynamicallyRemoveEndpoints(service ServiceUnit) bool { +func (r *templateRouter) dynamicallyRemoveEndpoints(id ServiceUnitKey, service ServiceUnit) bool { if r.dynamicConfigManager == nil || !r.synced { return false } @@ for backendKey := range service.ServiceAliasAssociations { - if _, ok := r.state[backendKey]; !ok { + cfg, ok := r.state[backendKey] + if !ok { continue } @@ - if err := r.dynamicConfigManager.RemoveRouteEndpoints(backendKey, service.EndpointTable); err != nil { + if err := r.dynamicConfigManager.RemoveRouteEndpoints(backendKey, cfg.EndpointTable[id]); err != nil { // Error dynamically modifying the config, so return false to cause a reload to happen. log.Info("router will reload as the ConfigManager could not dynamically remove endpoints for backend", "backendKey", backendKey, "error", err) return false } }You'll also need the matching call-site update at Line 1014.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/router/template/router.go` around lines 982 - 996, The dynamicallyRemoveEndpoints helper is passing the entire ServiceUnit.EndpointTable to every associated backend, causing RemoveRouteEndpoints(backendKey, service.EndpointTable) to delete endpoints that don't belong to specific backends; change dynamicallyRemoveEndpoints (and its call-site) to pass only the route-specific endpoints for each backend (i.e., the endpoints that correspond to that backendKey) instead of service.EndpointTable, updating the use of ServiceAliasAssociations and the call-site that invokes dynamicallyRemoveEndpoints so each backend removal receives the correct per-route endpoint slice/map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/router/template/configmanager/haproxy/backend.go`:
- Around line 370-375: When innerDeleteServer(oldEP) fails after you already
called innerSetServerState(oldEP, false), restore the original server state to
avoid leaving the fallback endpoint drained; change the code around the
innerDeleteServer call to capture the delete error, call
b.innerSetServerState(oldEP, true) to revert the change, and return a combined
or wrapped error if the restore also fails (reference innerSetServerState and
innerDeleteServer to locate the logic).
- Around line 361-363: The code only updates HAProxy weight when oldEP.Weight !=
newEP.Weight via b.innerUpdateServerWeight, but fails to toggle the server's
state between "ready" and "drain" when the weight crosses the zero boundary (<=0
vs >0), causing drained endpoints to still serve or recovered ones to remain
drained (and passthrough always sends 100%). Modify the branch handling in the
function that compares oldEP and newEP so that when weight changes you detect a
crossing of the drain threshold (oldEP.Weight > 0 && newEP.Weight <= 0 =>
transition to "drain"; oldEP.Weight <= 0 && newEP.Weight > 0 => transition to
"ready") and invoke the appropriate HAProxy state-change routine in addition to
calling b.innerUpdateServerWeight(newEP, isPassthrough); ensure isPassthrough
still results in correct weight payload but do the state flip via the same
server-state API used elsewhere.
In `@pkg/router/template/configmanager/haproxy/manager.go`:
- Around line 524-545: The code must only attempt to re-enable the singleton
health check if that exact old endpoint still exists; before calling
backend.EnableHealthCheck on oldEndpoints[0] (and before calling
backend.ReplaceServer/adding to newEPs), verify that the endpoint is still
present in the current service/backend endpoints (e.g. check svc.Endpoints or
the current backend server list) and skip this whole branch if the endpoint no
longer exists so a prior ReplaceServer/delete isn't turned into a forced reload.
In `@pkg/router/template/router.go`:
- Around line 886-890: When removing a service from a route, the code currently
passes the raw service list (service.EndpointTable) to RemoveRouteEndpoints;
instead it should pass the route-scoped endpoint slice that was actually
attached to this backend (the EndpointTable stored on the service unit from the
old route config). Update the call in the loop over oldConfig.ServiceUnits so
that, after locating the removed service via r.findMatchingServiceUnit, you
retrieve the service unit from oldConfig.ServiceUnits[serviceKey] (or the local
variable representing that unit) and pass that unit's EndpointTable (the
PreferPort-filtered slice) to
r.dynamicConfigManager.RemoveRouteEndpoints(backendKey, ... ) instead of
service.EndpointTable, keeping the same backendKey and error handling.
---
Outside diff comments:
In `@pkg/router/template/router.go`:
- Around line 982-996: The dynamicallyRemoveEndpoints helper is passing the
entire ServiceUnit.EndpointTable to every associated backend, causing
RemoveRouteEndpoints(backendKey, service.EndpointTable) to delete endpoints that
don't belong to specific backends; change dynamicallyRemoveEndpoints (and its
call-site) to pass only the route-specific endpoints for each backend (i.e., the
endpoints that correspond to that backendKey) instead of service.EndpointTable,
updating the use of ServiceAliasAssociations and the call-site that invokes
dynamicallyRemoveEndpoints so each backend removal receives the correct
per-route endpoint slice/map.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aa126d80-4b4b-44bb-841c-1121e049641b
📒 Files selected for processing (6)
pkg/router/template/configmanager/haproxy/backend.gopkg/router/template/configmanager/haproxy/backend_test.gopkg/router/template/configmanager/haproxy/blueprint_plugin_test.gopkg/router/template/configmanager/haproxy/manager.gopkg/router/template/router.gopkg/router/template/types.go
|
/assign |
This reverts commit 0e47c83.
jcmoraisjr
force-pushed
the
OCPBUGS-77344-dcm-update-route
branch
from
5dbcbdf to
b6c9a42
Compare
Added a separate commit reverting 0e47c83, the new implementation is on top of it. |
jcmoraisjr
force-pushed
the
OCPBUGS-77344-dcm-update-route
branch
from
b6c9a42 to
09d388f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/router/template/router.go (1)
982-996:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the route-scoped endpoint slice when deleting service endpoints.
service.EndpointTableis still the raw service list here. For aPreferPortroute, Line 995 can try to delete servers that were never attached to that backend and force an avoidable reload. The stored per-route snapshot inr.state[backendKey].EndpointTable[...]is the right source, just like the earlier route-removal fix.Suggested fix
-func (r *templateRouter) dynamicallyRemoveEndpoints(service ServiceUnit) bool { +func (r *templateRouter) dynamicallyRemoveEndpoints(id ServiceUnitKey, service ServiceUnit) bool { if r.dynamicConfigManager == nil || !r.synced { return false } @@ for backendKey := range service.ServiceAliasAssociations { - if _, ok := r.state[backendKey]; !ok { + cfg, ok := r.state[backendKey] + if !ok { continue } log.V(4).Info("dynamically removing endpoints for associated backend", "backendKey", backendKey) - if err := r.dynamicConfigManager.RemoveRouteEndpoints(backendKey, service.EndpointTable); err != nil { + if err := r.dynamicConfigManager.RemoveRouteEndpoints(backendKey, cfg.EndpointTable[id]); err != nil { // Error dynamically modifying the config, so return false to cause a reload to happen. log.Info("router will reload as the ConfigManager could not dynamically remove endpoints for backend", "backendKey", backendKey, "error", err) return false @@ - configChanged := r.dynamicallyRemoveEndpoints(service) + configChanged := r.dynamicallyRemoveEndpoints(id, service)Also applies to: 1014-1014
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/router/template/router.go` around lines 982 - 996, The code is calling RemoveRouteEndpoints with the raw service.EndpointTable which can differ from the per-route snapshot for a given backend (causing wrong deletions for PreferPort routes); in dynamicallyRemoveEndpoints replace service.EndpointTable with the stored route-scoped endpoints from r.state[backendKey].EndpointTable[...] (use the entry keyed by the service/route identifier used in r.state, e.g. r.state[backendKey].EndpointTable[service.Name]) when invoking r.dynamicConfigManager.RemoveRouteEndpoints; make the same change at the other occurrence around the later block (near line 1014) so deletions use the route-specific snapshot rather than the raw service list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/router/template/router.go`:
- Around line 855-899: The dynamicallyUpdateRoute function must bail out when
dynamic config management is unavailable or before the initial sync to avoid
panics; add the same guard used by other dynamically* helpers at the start of
dynamicallyUpdateRoute by checking r.dynamicConfigManager for nil and the
manager's "synced/initialized" state (e.g., r.dynamicConfigManager == nil ||
!r.dynamicConfigManager.IsSynced()/IsInitialized() or whatever method your
DynamicConfigManager exposes) and return false if not ready so
ReplaceRouteEndpoints and RemoveRouteEndpoints are never called when the DCM is
disabled or not yet synced.
---
Duplicate comments:
In `@pkg/router/template/router.go`:
- Around line 982-996: The code is calling RemoveRouteEndpoints with the raw
service.EndpointTable which can differ from the per-route snapshot for a given
backend (causing wrong deletions for PreferPort routes); in
dynamicallyRemoveEndpoints replace service.EndpointTable with the stored
route-scoped endpoints from r.state[backendKey].EndpointTable[...] (use the
entry keyed by the service/route identifier used in r.state, e.g.
r.state[backendKey].EndpointTable[service.Name]) when invoking
r.dynamicConfigManager.RemoveRouteEndpoints; make the same change at the other
occurrence around the later block (near line 1014) so deletions use the
route-specific snapshot rather than the raw service list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ef3f03f-4a2b-450e-b3bb-4c12ef9191ab
📒 Files selected for processing (6)
pkg/router/template/configmanager/haproxy/backend.gopkg/router/template/configmanager/haproxy/backend_test.gopkg/router/template/configmanager/haproxy/blueprint_plugin_test.gopkg/router/template/configmanager/haproxy/manager.gopkg/router/template/router.gopkg/router/template/types.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/router/template/configmanager/haproxy/blueprint_plugin_test.go
- pkg/router/template/types.go
- pkg/router/template/configmanager/haproxy/backend_test.go
- pkg/router/template/configmanager/haproxy/manager.go
- pkg/router/template/configmanager/haproxy/backend.go
jcmoraisjr
force-pushed
the
OCPBUGS-77344-dcm-update-route
branch
from
09d388f to
5052289
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/router/template/configmanager/haproxy/manager.go`:
- Around line 460-461: The code currently skips updates when only the
health-check policy flips because reflect.DeepEqual detects changes but the
UpdateServer flow (UpdateServer and the later health pass) doesn’t apply
health-only changes; modify the update flow so health-only differences in
modifiedEndpoints (epPair entries in modifiedEndpoints map) are handled: when
iterating modifiedEndpoints (where modifiedEndpoints[newEP.ID] = epPair{...}),
detect if the only diff is health-related and call the same logic that updates
health checks (the UpdateServer/replace health-check path or the health pass
that walks added/replaced endpoints) for that endpoint ID so HAProxy gets
reconfigured; ensure you reference the epPair fields (oldEP/newEP), the
modifiedEndpoints map, and the UpdateServer health-check update code path so
health-only changes trigger the appropriate reconfigure instead of being treated
as a no-op.
- Around line 766-770: The unconditional "return nil" disables dynamic route
adds; change it so we only skip using blueprints when the HAProxy 2.8 bug
actually applies and a matching blueprint exists. In practice, restore the
normal flow in AddRoute by removing the unconditional return and replace it with
a conditional that checks the HAProxy version and the result of
findMatchingBlueprint(...) (e.g., only return nil when haproxy version is 2.8
and findMatchingBlueprint(...) returns a non-nil blueprint); otherwise proceed
so the rest of the matcher runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d0103cac-5801-43d7-be47-9c6bd41df0ae
📒 Files selected for processing (6)
pkg/router/template/configmanager/haproxy/backend.gopkg/router/template/configmanager/haproxy/backend_test.gopkg/router/template/configmanager/haproxy/blueprint_plugin_test.gopkg/router/template/configmanager/haproxy/manager.gopkg/router/template/router.gopkg/router/template/types.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/router/template/types.go
- pkg/router/template/configmanager/haproxy/blueprint_plugin_test.go
- pkg/router/template/configmanager/haproxy/backend_test.go
- pkg/router/template/configmanager/haproxy/backend.go
- pkg/router/template/router.go
DCM was disabled on route changes due to the way the resource changes are being handled: without DCM, the template plugin applies a route remove, followed by a route add, which works well in the fork-and-reload approach since the removal is not applied until adding the route again succeed. On DCM, each of these operations are done separately as well, which means that the route is completely disabled during some time. This update changes this behavior by saving previous and updated states of a route, identifying the differences, and dynamically applying them if possible. A fork-and-reload is made in case the dynamic update is not possible, as before. Part of the changes also include extending the Endpoint struct so that it stores the weight and the VerifyHostname properties of each endpoint in order to make comparisons easier, as well as decrease the number of parameters sent to inner methods. These fields cannot be updated during the Endpoint creation, since they depend on the backend the endpoint is assigned. The router's updateEndpointTable() method centralizes all changes to these context based fields. https://redhat.atlassian.net/browse/OCPBUGS-77344
jcmoraisjr
force-pushed
the
OCPBUGS-77344-dcm-update-route
branch
from
5052289 to
b69f565
Compare
|
@jcmoraisjr: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
DCM was disabled on route changes due to the way the resource changes are being handled: without DCM, the template plugin applies a route remove, followed by a route add, which works well in the fork-and-reload approach since the removal is not applied until adding the route again succeed. On DCM, each of these operations are done separately as well, which means that the route is completely disabled during some time.
This update changes this behavior by saving previous and updated states of a route, identifying the differences, and dynamically applying them if possible. A fork-and-reload is made in case the dynamic update is not possible, as before.
Part of the changes also include extending the Endpoint struct so that it stores the weight and the VerifyHostname properties of each endpoint in order to make comparisons easier, as well as decrease the number of parameters sent to inner methods. These fields cannot be updated during the Endpoint creation, since they depend on the backend the endpoint is assigned. The router's updateEndpointTable() method centralizes all changes to these context based fields.
Also, reverting #739 as a separate commit since it fixes the root cause of the problem.
https://redhat.atlassian.net/browse/OCPBUGS-77344
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests