Skip to content

syslog changes Multi NPU platforms #4738

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.

Sign up for GitHub

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

Merged
arlakshm merged 8 commits into from
Jun 30, 2020
Merged

syslog changes Multi NPU platforms #4738

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a4a10a7
syslog changes Multi NPU platforms
arlakshm Jun 9, 2020
795b2b0
indentation fix
arlakshm Jun 9, 2020
3616a5f
Additional changes to support swss_log messages
arlakshm Jun 11, 2020
16c6b3d
Address review comment.
arlakshm Jun 15, 2020
1deb8a0
Addressing review comments
arlakshm Jun 15, 2020
b760de0
Addressing review comments
arlakshm Jun 16, 2020
222489d
Addressing review comments
arlakshm Jun 18, 2020
84aa4b5
fix build
arlakshm Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions files/build_templates/docker_image_ctl.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,10 @@ start() {
done
fi
{%- endif %}
# single NPU systems and container running on the host network continue to use the json-file as log-driver
Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

single NPU systems and container running on the host network continue to use the json-file as log-driver [](start = 8, length = 106)

The different behavior of single NPU vs multiple NPU is confusing and complex deign.

What about moving all use cases to log-driver, or all use cases to json-file as before? #Closed

Copy link
Copy Markdown
Contributor Author

@arlakshm arlakshm Jun 15, 2020

Choose a reason for hiding this comment

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

Changed to use json-file for all cases

{%- if '--log-driver=json-file' in docker_image_run_opt or '--log-driver' not in docker_image_run_opt %}
LOG_OPTS="--log-opt max-size=2M --log-opt max-file=5"
{%- endif %}
else
Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No need to change this line #Closed

Copy link
Copy Markdown
Contributor Author

@arlakshm arlakshm Jun 17, 2020

Choose a reason for hiding this comment

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

Fixed in latest commit

# This part of code is applicable for Multi-ASIC platforms. Here we mount the namespace specific
# redis directory into the docker running in that namespace. Below eg: is for namespace "asic1"
Expand All @@ -238,7 +242,9 @@ start() {
redis_dir=`echo $redis_dir_list | cut -d " " -f $id`
REDIS_MNT=" -v $redis_dir:$redis_dir:rw "
fi

# Containers in the docker network will use the syslog-driver as log-driver. This is mainly applicable for Multi NPU system
LOG_OPTS="--log-driver=syslog --log-opt syslog-address=udp://127.0.0.1:514 --log-opt syslog-format=rfc5424 \
Copy link
Copy Markdown
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should there be a check to make sure docker_image_run_opt does not have--log-driver=json-file ?

Copy link
Copy Markdown
Contributor Author

@arlakshm arlakshm Jun 9, 2020

Choose a reason for hiding this comment

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

@SuvarnaMeenakshi syslogs will not work if the log-driver is json on containers running the docker bridge network, so I think we can ignore --log-driver=json-file in this case.

Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Could you giver proof or reference that "syslogs will not work if the log-driver is json on containers running the docker bridge network"? #Closed

Copy link
Copy Markdown
Contributor Author

@arlakshm arlakshm Jun 10, 2020

Choose a reason for hiding this comment

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

Logs

docker logs for swss running in namespace asic0.

admin@sonic:~$ docker logs swss0 | grep orchagent
2020-06-10 00:45:01,434 INFO spawned: 'orchagent' with pid 36
2020-06-10 00:45:02,439 INFO success: orchagent entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)

same logs not available in syslog file

admin@sonic:~$ sudo zgrep -i orchagent /var/log/syslog.*
admin@sonic:~$

Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I see your point. Syslogs will not work if the log-driver is json and this is not related to docker bridge network and applicable to previous single ASIC use case.

Do you want to fix both single ASIC and multiple ASIC? #Closed

Copy link
Copy Markdown
Contributor Author

@arlakshm arlakshm Jun 10, 2020

Choose a reason for hiding this comment

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

I didnt change the Single ASIC I didnt want to break any backward compatibility for some applications,

Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Then you need to check '--log-driver=json-file' in docker_image_run_opt because it will work if user specify it explicitly #Closed

Copy link
Copy Markdown
Contributor Author

@arlakshm arlakshm Jun 12, 2020

Choose a reason for hiding this comment

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

Fixed in the latest commit

--log-opt syslog-facility=daemon --log-opt tag={{docker_container_name}}$DEV"
{%- if docker_container_name == "database" %}
Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No need to change this line #Closed

Copy link
Copy Markdown
Contributor Author

@arlakshm arlakshm Jun 17, 2020

Choose a reason for hiding this comment

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

Fixed in latest commit

NET="bridge"
{%- else %}
Expand All @@ -262,9 +268,7 @@ start() {
{%- if install_debug_image == "y" %}
-v /src:/src:ro -v /debug:/debug:rw \
{%- endif %}
{%- if '--log-driver=json-file' in docker_image_run_opt or '--log-driver' not in docker_image_run_opt %}
--log-opt max-size=2M --log-opt max-file=5 \
{%- endif %}
$LOG_OPTS \
{%- if sonic_asic_platform == "mellanox" %}
{%- if docker_container_name == "syncd" %}
-v /var/log/mellanox/sniffer:/var/log/mellanox/sniffer:rw \
Expand Down