Skip to content
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

Move eventd enabled check from build time to runtime #20248

Merged
merged 13 commits into from
Sep 23, 2024

Conversation

zbud-msft
Copy link
Contributor

@zbud-msft zbud-msft commented Sep 12, 2024

Why I did it

Work item tracking
  • Microsoft ADO (number only):28728116

How I did it

Remove eventd enabled/slim image check from Dockerfile (build time). As part of Dockerfile expose eventd_enabled and slim image flags to ENV which will be used docker_init/start.sh to check if rsyslog plugin should be moved to rsyslog.d

How to verify it

Manual test/pipeline

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

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)

@zbud-msft zbud-msft changed the title [Not4Review] eventd_enabled flag at runtime Create runtime flags to check if eventd enabled Sep 13, 2024
@zbud-msft zbud-msft changed the title Create runtime flags to check if eventd enabled Move eventd enabled check from build time to runtime Sep 13, 2024
@zbud-msft zbud-msft marked this pull request as ready for review September 13, 2024 18:00
slave.mk Outdated Show resolved Hide resolved
qiluo-msft
qiluo-msft previously approved these changes Sep 16, 2024
@@ -21,4 +21,6 @@ chmod +x /usr/bin/wait_for_intf.sh
# The docker container should start this script as PID 1, so now that supervisord is
# properly configured, we exec /usr/local/bin/supervisord so that it runs as PID 1 for the
# duration of the container's lifetime
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/events_info.json > /etc/rsyslog.d/dhcp_relay_events.conf
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ENVIRONMENT

offline discussed, need to check ConfigDB flag, not env var. #Closed

Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As comments

@qiluo-msft qiluo-msft merged commit 561479e into sonic-net:master Sep 23, 2024
23 checks passed
@StormLiangMS
Copy link
Contributor

hi @yxieca could you cherry pick?

@StormLiangMS
Copy link
Contributor

hi @zbud-msft do we need this one for 202405?

@bingwang-ms
Copy link
Contributor

@zbud-msft, @qiluo-msft We are only cherry-pick bug fix for 202405 branch. This seems to be an enhancement to me. Please clarify if this is a must-have for 202405 branch. Thanks!

mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Sep 25, 2024
## How I did it

Remove eventd enabled/slim image check from Dockerfile (build time). As part of Dockerfile expose eventd_enabled and slim image flags to ENV which will be used docker_init/start.sh to check if rsyslog plugin should be moved to rsyslog.d

#### How to verify it

Manual test/pipeline
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202405: #20351

mssonicbld pushed a commit that referenced this pull request Sep 26, 2024
## How I did it

Remove eventd enabled/slim image check from Dockerfile (build time). As part of Dockerfile expose eventd_enabled and slim image flags to ENV which will be used docker_init/start.sh to check if rsyslog plugin should be moved to rsyslog.d

#### How to verify it

Manual test/pipeline
saksarav-nokia pushed a commit to saksarav-nokia/sonic-buildimage that referenced this pull request Oct 9, 2024
## How I did it

Remove eventd enabled/slim image check from Dockerfile (build time). As part of Dockerfile expose eventd_enabled and slim image flags to ENV which will be used docker_init/start.sh to check if rsyslog plugin should be moved to rsyslog.d

#### How to verify it

Manual test/pipeline
Copy link
Collaborator

@stepanblyschak stepanblyschak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We notice few issues in 202405 regression, @zbud-msft @qiluo-msft Could you please have a look?

{%- elif docker_container_name == "eventd" %}
export EVENTD_STATE=$(sonic-db-cli -s CONFIG_DB HGET 'FEATURE|eventd' 'state')
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/events_info.json > /etc/rsyslog.d/host_events.conf
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/syncd_events_info.json > /etc/rsyslog.d/syncd_events.conf
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zbud-msft @qiluo-msft These commands are CPU heavy as well as those added in docker-init.j2 for multiple containers.
Any performance measurements done w.r.t fast-reboot, warm-reboot to understand the impact?
Any idea how to achieve same functionality without introducing additional CPU overhead during init?

@@ -21,4 +21,7 @@ chmod +x /usr/bin/wait_for_intf.sh
# The docker container should start this script as PID 1, so now that supervisord is
# properly configured, we exec /usr/local/bin/supervisord so that it runs as PID 1 for the
# duration of the container's lifetime
export EVENTD_STATE=$(sonic-db-cli -s CONFIG_DB HGET 'FEATURE|eventd' 'state')
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/events_info.json > /etc/rsyslog.d/dhcp_relay_events.conf
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zbud-msft What if eventd state changes in runtime ? Is this flow unsupported ?

export EVENTD_STATE=$(sonic-db-cli -s CONFIG_DB HGET 'FEATURE|eventd' 'state')
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/events_info.json > /etc/rsyslog.d/host_events.conf
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/syncd_events_info.json > /etc/rsyslog.d/syncd_events.conf
systemctl restart rsyslog
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zbud-msft @qiluo-msft In our testing this introduces a race between eventd's postStartAction and rsyslog-config service. rsyslog-config.service will overwrite /etc/rsyslog.conf and when at the same time we restart rsyslogd we'll get rsyslogd exited and then eventd service start failure.

export EVENTD_STATE=$(sonic-db-cli -s CONFIG_DB HGET 'FEATURE|eventd' 'state')
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/events_info.json > /etc/rsyslog.d/host_events.conf
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/syncd_events_info.json > /etc/rsyslog.d/syncd_events.conf
systemctl restart rsyslog
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case this fails eventd docker will be still running

export EVENTD_STATE=$(sonic-db-cli -s CONFIG_DB HGET 'FEATURE|eventd' 'state')
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/events_info.json > /etc/rsyslog.d/host_events.conf
j2 -f json --import-env=ENVIRONMENT /usr/share/sonic/templates/rsyslog_plugin/rsyslog_plugin.conf.j2 /usr/share/sonic/templates/rsyslog_plugin/syncd_events_info.json > /etc/rsyslog.d/syncd_events.conf
systemctl restart rsyslog
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zbud-msft @qiluo-msft We will loose syslog messages during boot up

@stepanblyschak
Copy link
Collaborator

@zbud-msft @qiluo-msft Could you please have a look - #20544 ?

aidan-gallagher pushed a commit to aidan-gallagher/sonic-buildimage that referenced this pull request Nov 16, 2024
## How I did it

Remove eventd enabled/slim image check from Dockerfile (build time). As part of Dockerfile expose eventd_enabled and slim image flags to ENV which will be used docker_init/start.sh to check if rsyslog plugin should be moved to rsyslog.d

#### How to verify it

Manual test/pipeline
zbud-msft added a commit to zbud-msft/sonic-buildimage that referenced this pull request Nov 27, 2024
zbud-msft added a commit to zbud-msft/sonic-buildimage that referenced this pull request Nov 27, 2024
bingwang-ms pushed a commit that referenced this pull request Dec 1, 2024
lguohan pushed a commit that referenced this pull request Dec 4, 2024
VladimirKuk pushed a commit to Marvell-switching/sonic-buildimage that referenced this pull request Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants