-
Notifications
You must be signed in to change notification settings - Fork 1.7k
restapi-sidecar support critical process crash to exit supervisord, and some bugfixes #25303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 18d1dbd.
to host Fix: supervisor-proc-exit-listener-rs: listen on right events
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the docker-restapi-sidecar container with several critical improvements and bugfixes. The main focus is on supporting critical process crash detection to exit supervisord, adding missing sonic_py_common dependency, and implementing per-branch file synchronization to support multiple SONiC release branches (202311, 202405, 202411, 202505, 202511).
Changes:
- Added critical process monitoring with supervisor-proc-exit-listener-rs to exit supervisord when systemd_stub crashes
- Implemented runtime branch detection from SONiC version to select appropriate per-branch files
- Fixed missing SONIC_PY_COMMON_PY3 dependency in build rules
- Added rsyslog configuration to relay logs from container to host
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
| rules/docker-restapi-sidecar.mk | Added SONIC_PY_COMMON_PY3 wheel dependency |
| dockers/docker-restapi-sidecar/systemd_stub.py | Added _get_branch_name() function for runtime branch detection and per-branch file selection |
| dockers/docker-restapi-sidecar/systemd_scripts/v1/restapi.sh_* | Five branch-specific startup scripts with per-branch differences |
| dockers/docker-restapi-sidecar/systemd_scripts/restapi.service_* | Five branch-specific systemd service files |
| dockers/docker-restapi-sidecar/systemd_scripts/container_checker_* | Five branch-specific container checker scripts |
| dockers/docker-restapi-sidecar/supervisord.conf | Changed systemd_stub autorestart to false, added supervisor-proc-exit-listener |
| dockers/docker-restapi-sidecar/critical_processes | New file marking systemd_stub as critical |
| dockers/docker-restapi-sidecar/etc/rsyslog.conf | New rsyslog configuration for log relay to host |
| dockers/docker-restapi-sidecar/etc/rsyslog.d/supervisor.conf | New rsyslog configuration for supervisor logs |
| dockers/docker-restapi-sidecar/Dockerfile.j2 | Updated to install dependencies and copy rsyslog configs |
| dockers/docker-restapi-sidecar/cli-plugin-tests/test_systemd_stub.py | Added comprehensive tests for branch detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| --net=$NET \ | ||
| -e RUNTIME_OWNER=local \ | ||
| --uts=host \ | ||
| --tmpfs /var/log/supervisor:rw \ |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line starting with --tmpfs /var/log/supervisor:rw \ uses a tab character for indentation instead of spaces. This is inconsistent with the rest of the file which uses spaces for indentation. For consistency and to avoid potential issues with shell scripts, spaces should be used instead of tabs.
| --tmpfs /var/log/supervisor:rw \ | |
| --tmpfs /var/log/supervisor:rw \ |
| --net=$NET \ | ||
| -e RUNTIME_OWNER=local \ | ||
| --uts=host \ | ||
| --tmpfs /var/log/supervisor:rw \ |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line starting with --tmpfs /var/log/supervisor:rw \ uses a tab character for indentation instead of spaces. This is inconsistent with the rest of the file which uses spaces for indentation. For consistency and to avoid potential issues with shell scripts, spaces should be used instead of tabs.
| --tmpfs /var/log/supervisor:rw \ | |
| --tmpfs /var/log/supervisor:rw \ |
| [eventlistener:supervisor-proc-exit-listener] | ||
| command=/usr/bin/supervisor-proc-exit-listener-rs --container-name restapi | ||
| events=PROCESS_STATE_EXITED,PROCESS_STATE_RUNNING | ||
| autostart=true | ||
| autorestart=unexpected | ||
| stdout_logfile=/dev/stdout | ||
| stdout_logfile_maxbytes=0 | ||
| stderr_logfile=/dev/stderr | ||
| stderr_logfile_maxbytes=0 |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The supervisor-proc-exit-listener eventlistener is missing the buffer_size=1024 configuration parameter. All other containers in the codebase that use supervisor-proc-exit-listener-rs include this parameter (e.g., dockers/docker-nat/supervisord.conf:20, dockers/docker-sonic-restapi/supervisord.conf:20). This parameter should be added for consistency with the rest of the codebase.
| FROM $BASE | ||
|
|
||
| {{ rsync_from_builder_stage() }} | ||
| RUN --mount=type=bind,from=base,target=/changes-to-image rsync -axAX --no-D --exclude=/sys --exclude=/proc --exclude=/dev --exclude=resolv.conf /changes-to-image/ / |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rsync command is missing the --omit-dir-times flag that is present in the rsync_from_builder_stage() macro (dockers/dockerfile-macros.j2:49). While this may not cause functional issues, it's inconsistent with the macro pattern used elsewhere in the codebase. Consider using the macro or adding the flag for consistency.
| def main(): | ||
| """ | ||
| @summary: This function will compare the difference between the current running containers | ||
| and the containers which were expected to run. If containers which were exepcted |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error: "exepcted" should be "expected".
| and the containers which were expected to run. If containers which were exepcted | |
| and the containers which were expected to run. If containers which were expected |
| pass | ||
| except docker.errors.DockerException as err: | ||
| print(f"Docker client error: '{err}'") | ||
| pass |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary 'pass' statement.
| pass | |
| except docker.errors.DockerException as err: | |
| print(f"Docker client error: '{err}'") | |
| pass | |
| except docker.errors.DockerException as err: | |
| print(f"Docker client error: '{err}'") |
| pass | ||
| except docker.errors.DockerException as err: | ||
| print(f"Docker client error: '{err}'") | ||
| pass |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary 'pass' statement.
| pass | |
| except docker.errors.DockerException as err: | |
| print(f"Docker client error: '{err}'") | |
| pass | |
| except docker.errors.DockerException as err: | |
| print(f"Docker client error: '{err}'") |
| running_containers.add(name) | ||
| except (docker.errors.NotFound, docker.errors.APIError) as err: | ||
| print("Failed to get container '{}'. Error: '{}'".format(name, err)) | ||
| pass |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary 'pass' statement.
| pass | ||
| except docker.errors.DockerException as err: | ||
| print(f"Docker client error: '{err}'") | ||
| pass |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary 'pass' statement.
| pass | |
| except docker.errors.DockerException as err: | |
| print(f"Docker client error: '{err}'") | |
| pass | |
| except docker.errors.DockerException as err: | |
| print(f"Docker client error: '{err}'") |
| pass | ||
| except docker.errors.DockerException as err: | ||
| print(f"Docker client error: '{err}'") | ||
| pass |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary 'pass' statement.
| pass | |
| except docker.errors.DockerException as err: | |
| print(f"Docker client error: '{err}'") | |
| pass | |
| except docker.errors.DockerException as err: | |
| print(f"Docker client error: '{err}'") |
Why I did it
(merge after #25230)
Enhance docker-restapi-sidecar:
Work item tracking
How I did it
How to verify it
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)