OCPBUGS-61487: Add timer for Azure VIP routes reconciliation #5455
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ricky-rav: This pull request references Jira Issue OCPBUGS-61487, 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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ricky-rav 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@ricky-rav: This pull request references Jira Issue OCPBUGS-61487, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
ricky-rav
force-pushed
the
OCPBUGS-61487
branch
from
00710a0 to
841e6c8
Compare
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
/test e2e-azure-ovn-upgrade |
|
/hold |
During systemd daemon-reload (e.g., during MCO updates), the openshift-azure-routes.path unit may miss file change events from apiserver-watcher. This leaves the azure-vips nftables table empty even when the API server is back up. Fix by adding a periodic timer that triggers reconciliation every 30 seconds as a fallback. The path unit still provides immediate response to file changes. Also wrap the service ExecStart with flock to prevent concurrent executions when both triggers fire simultaneously. Reduce logging so that we do not log when there are not routes to add. This prevents the journal from getting clogged, since the service is now triggered every 30 seconds. Signed-off-by: Riccardo Ravaioli <[email protected]>
ricky-rav
force-pushed
the
OCPBUGS-61487
branch
from
841e6c8 to
878f96a
Compare
|
/test e2e-azure-ovn-upgrade |
I reduced the logs to a bare minimum, but systemd still logs to the journal every time the service is triggered: I'm still wondering what a better solution might be... |
Without this setting, journal entries appear under "bash" instead of the service name, making it harder to filter logs with journalctl -u. Before: Oct 31 22:33:06 master-0 bash[983]: Processing route for NIC 0/... for 10.0.0.2 After: Oct 31 22:33:06 master-0 openshift-gcp-routes[983]: Processing route for NIC 0/... for 10.0.0.2 Signed-off-by: Riccardo Ravaioli <[email protected]>
|
in the end we log a lot also on GCP, where |
|
/assign @tssurya |
There was a problem hiding this comment.
During systemd daemon-reload the openshift-azure-routes.path unit may miss file change events from apiserver-watcher
hmm why does it miss it? cause process is not yet correctly up ? so a timing issue during reload? In that case wouldn't a better fix be if a reload or restart has happened then we trigger a sync 30seconds after reload and/or on startup? I wonder if there is an option for ordering the firing that way instead of doing it every 30seconds..I worry we are going to do unwanted work too frequently?
| if [ -z "${ovnkContainerID}" ]; then | ||
| return | ||
| fi | ||
| echo "Found ovnkube-controller pod... ${ovnkContainerID}" |
There was a problem hiding this comment.
reason I had added sooo many echo statements was because script was soo raw and we were directly touching the DBs that I was super worried what command we are transaction for history purposes....plus I wasn't a bash expert really :) so wanted to be extra careful with each step I was doing LOL
But I think its been some years now, which hopefully means this is stable, so I'm ok to remove these statements and have less logs. - but also OK with what you did, which is consolidate them into a single line.
systemd logging it every 30 seconds is going to be annoying a bit
| Description=Periodic reconciliation of Azure VIP routes | ||
| [Timer] | ||
| OnBootSec=30 |
There was a problem hiding this comment.
hmm does it mean it will run 30sec after bootup as a one time thing? or this is every 30seconds like you said in PR description.. cause isn't it too much if we act on the filechange and run this every 30seconds?
and what does onunitactivesec mean? 30seconds after the unit gets active?
There was a problem hiding this comment.
So it gets triggered the first time 30 seconds after boot (OnBootSec=30) and then 30 seconds after the service last finished (OnUnitActiveSec=30), repeatedly.
| ExecStart=/bin/bash /opt/libexec/openshift-gcp-routes.sh start | ||
| ExecStopPost=/bin/bash /opt/libexec/openshift-gcp-routes.sh cleanup | ||
| User=root | ||
| SyslogIdentifier=openshift-gcp-routes |
There was a problem hiding this comment.
nice, didn't know about this setting.
Yes, if the .up/.down file gets created while a systemd daemon-reload happens, the file change is missed and we never converge again until another file change happens. More precisely,
That was my initial idea too, but I couldn't find a trigger in systemd (
I'm not very happy about it either, I'm just comforted by the fact that we're doing the same thing with gcp and we do it every 5 seconds... |
|
@ricky-rav: The following tests failed, say
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. |
3 participants
During systemd daemon-reload (e.g., during MCO updates), the openshift-azure-routes.path unit may miss file change events from apiserver-watcher. This leaves the azure-vips nftables table empty even when the API server is back up.
Fix by adding a periodic timer that triggers reconciliation every 30 seconds as a fallback. The path unit still provides immediate response to file changes. Also wrap the service ExecStart with flock to prevent concurrent executions when both triggers fire simultaneously.