[202608] [redfish] Deliver bridged-container syslog to the host over docker0 - #3137
Conversation
…#29258) What: Adds redfish's bridged-container syslog delivery over the docker0 gateway, touching docker_image_ctl.j2 and rsyslog-config.sh; renders bridged-container syslog handling from a single list so dhcp_server and redfish stay in sync. Why: redfish runs bridge-networked, so its 127.0.0.1 isn't the host's and its rsyslog couldn't reach the host over loopback — container logs (including startup FATALs) never reached host /var/log/syslog. How: Wires up the three pieces dhcp_server already has, refactoring the bridged-container syslog config into a single list to prevent drift between dhcp_server and redfish. Testing: CI green (all required checks); mergeStateStatus CLEAN, approved. Signed-off-by: shreyansh-nexthop <shreyansh@nexthop.ai> (cherry picked from commit d5b23d5)
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Nikolay Mirin (nikamirrr)
left a comment
Nikolay Mirin (nikamirrr)
left a comment
There was a problem hiding this comment.
Reviewed as a cherry-pick of sonic-net/sonic-buildimage#29258 into 202608.
Port fidelity: good
I rendered docker_image_ctl.j2 for redfish, dhcp_server and snmp; all three render and pass bash -n. The shebang survives the new {%- set %} on line 2, and the rewritten for (( attempt=0; attempt<=BRIDGE_SYSLOG_WAIT_SECS; attempt++ )) preserves upstream's 11-checks/10-sleeps semantics exactly. The dhcp_server -> bridged_containers generalisation is faithful to upstream.
One blocking issue, and it is about merge order
The redfish_syslog iptables rule that the new wait loop polls for does not exist in the pinned caclmgrd — at either the merge-base pin (409d3b2) or the current 202608 tip pin (dfa3cc3). The companion PR that adds it, Azure/sonic-host-services.msft#23, is still open. Nothing is wrong with this diff; it just cannot work until #23 lands. Details inline on line 48.
One correctness issue worth fixing in the port
dhcp_server.service.j2 also carries User={{ sonicadmin_user }}, so the new redfish -> sudo iptables / else -> iptables split leaves dhcp_server with a check that can never succeed (iptables -C as non-root exits 4). Always using sudo is both shorter and correct. Inline on line 47.
The rest are non-blocking: exit-status handling in the two wait loops, the -C check keying on a comment label rather than on whether traffic is permitted, an unvalidated docker network inspect feeding SYSLOG_TARGET_IP, the FEATURE-state gate, and two cleanups in rsyslog-config.sh.
One item I could not anchor inline (outside the diff hunks)
docker_image_ctl.j2:767 still reads {%- if docker_container_name != "dhcp_server" %} around --net=$NET. Every other == "dhcp_server" test in the file was converted to in bridged_containers, but this one was not, so the two bridged containers get their bridge networking by two different mechanisms: dhcp_server is created with no --net flag at all (relying on docker's implicit default bridge), while redfish gets an explicit --net=bridge from line 674. "Bridged container" now means two different things in the same template, and whoever adds a third entry has to know this line exists.
Nikolay Mirin (nikamirrr)
left a comment
Nikolay Mirin (nikamirrr)
left a comment
There was a problem hiding this comment.
Please check the comments if anything is worth acting on
Hey, the only blocking issue highlighted here is that Azure/sonic-host-services.msft#23 should go in before this one, and it is already planned that way. That PR is the 202608 pick of the merged master caclmgrd change (sonic-net/sonic-host-services#429), mirroring how the two halves landed on master. The code here is exactly what was merged in master, and most of the review comments are about the pre-existing dhcp_server behaviour that redfish was built on top of. They are non-blocking and cover very extreme cases, so I am resolving the conversations. |
Nikolay Mirin (nikamirrr)
left a comment
Nikolay Mirin (nikamirrr)
left a comment
There was a problem hiding this comment.
Overall looks good, if any comments are N/A please ignore them
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
4 participants
Manual cherry-pick of sonic-net/sonic-buildimage#29258 (master commit d5b23d5) into 202608. The cherry-pick automation on the original PR hit a code conflict and asked for a manual cherry-pick PR.
The conflict no longer exists on the current 202608 head: the prerequisite dhcp_server docker0 syslog change from sonic-net/sonic-buildimage#28580 arrived through the 202605 code sync (#3112), so the commit applies cleanly and unmodified.
Why I did it
redfish runs bridge-networked, so its rsyslog cannot reach the host syslog listener over 127.0.0.1 and container logs, including startup failures, never reach host /var/log/syslog.
How I did it
git cherry-pick -x d5b23d5 on top of the 202608 head. No conflicts and no content changes relative to master.
How to verify it
Rendered the container control script for the redfish, dhcp_server, snmp and database containers; all pass bash -n and the bridged syslog wait logic renders only for the two bridged containers.