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

Timezone sync issue between the host and containers #14000

Merged
merged 4 commits into from
Jun 25, 2023

Conversation

nmoray
Copy link
Contributor

@nmoray nmoray commented Feb 27, 2023

Why I did it

To fix the timezone sync issue between the containers and the host. If a certain timezone has been configured on the host (SONIC) then the expectation is to reflect the same across all the containers.

This will fix Issue:13046.

For instance, a PST timezone has been set on the host and if the user checks the link flap logs (inside the FRR), it shows the UTC timestamp. Ideally, it should be PST.

LOGS:
root@sonic:~# show clock
Mon Feb 7 12:01:16 PST 2022 ======== configured as PST ( using set timezone)

sonic# vtysh
sonic# show interface Ethernet0
Interface Ethernet0 is up, line protocol is up
Link ups: 4 last: 2022/02/03 20:17:38.34 ----> shows UTC timestamp
Link downs: 6 last: 2022/02/03 20:17:13.35 ---->
vrf: default
index 26 metric 0 mtu 9126 speed 0
flags: <UP,BROADCAST,RUNNING,MULTICAST>
Type: Unknown
HWaddr: 00:30:64:6a:fa:c7
inet 172.21.227.64/31
inet6 fd0a:200💯:340/127
inet6 fe80::230:64ff:fe6a:fac7/64
Interface Type Other

How I did it

Mounted host's /etc/timezone file on all the containers to find out the presently configured timezone during the docker initialisation phase. Later, based on the host's timezone, create a symbolic link at runtime inside each of the containers (/etc/localtime). This way, we can avoid mounting the symlink on the containers.

How to verify it

root@sonic:# timedatectl set-timezone America/Los_Angeles
root@sonic:
#
root@sonic:#
root@sonic:
# date
Fri 28 Apr 2023 10:22:40 AM PDT

root@sonic:# cat /etc/timezone
America/Los_Angeles
root@sonic:
# ls /etc/localtime -l
lrwxrwxrwx 1 root root 41 Apr 28 10:22 /etc/localtime -> ../usr/share/zoneinfo/America/Los_Angeles
root@sonic:~# docker exec -it bgp bash
root@sonic:/# date
Fri Apr 28 17:23:17 UTC 2023
root@sonic:/# exit
exit

root@sonic:# config reload -f
Clear current config and reload config in config_db format from the default config file(s) ? [y/N]: y
Running command: rm -rf /tmp/dropstat-*
Disabling container monitoring ...
Stopping SONiC target ...
Running command: /usr/local/bin/sonic-cfggen -j /etc/sonic/init_cfg.json -j /etc/sonic/config_db.json --write-to-db
Running command: /usr/local/bin/db_migrator.py -o migrate
Running command: /usr/local/bin/sonic-cfggen -d -y /etc/sonic/sonic_version.yml -t /usr/share/sonic/templates/sonic-environment.j2,/etc/sonic/sonic-environment
Restarting SONiC target ...
Enabling container monitoring ...
Reloading Monit configuration ...
Reinitializing monit daemon
root@sonic:
# docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
b50ea6d53d4b docker-snmp:latest "/usr/local/bin/supe…" About a minute ago Up 2 seconds snmp
15b9c0860197 docker-sonic-telemetry:latest "/usr/local/bin/supe…" About a minute ago Up 5 seconds telemetry
9a8b5c0fed5d docker-sonic-mgmt-framework:latest "/usr/local/bin/supe…" About a minute ago Up 5 seconds mgmt-framework
c302bdbeee41 docker-lldp:latest "/usr/bin/docker-lld…" 4 minutes ago Up 5 seconds lldp
c7e150c50494 docker-platform-monitor:latest "/usr/bin/docker_ini…" 4 minutes ago Up 14 seconds pmon
b0be22f94e6c docker-router-advertiser:latest "/usr/bin/docker-ini…" 5 minutes ago Up 4 seconds radv
4298354779e0 docker-syncd-invm:latest "/usr/local/bin/supe…" 5 minutes ago Up 6 seconds syncd
a03f8f8a300d docker-teamd:latest "/usr/local/bin/supe…" 5 minutes ago Up 6 seconds teamd
c24c23066a6b docker-orchagent:latest "/usr/bin/docker-ini…" 5 minutes ago Up 7 seconds swss
89a0522ffc86 docker-fpm-frr:latest "/usr/bin/docker_ini…" 5 minutes ago Up 13 seconds bgp
ecccce070e53 docker-database:latest "/usr/local/bin/dock…" 5 minutes ago Up 5 minutes database

root@sonic:~# docker exec -it bgp bash
root@sonic:/# date
Fri Apr 28 10:24:07 PDT 2023
root@sonic:/# cat /etc/timezone
America/Los_Angeles
root@sonic:/# ls /etc/localtime -l
lrwxrwxrwx 1 root root 39 Apr 28 10:23 /etc/localtime -> /usr/share/zoneinfo/America/Los_Angeles
root@sonic:/# exit
exit

root@sonic:~# docker exec -it snmp bash
root@sonic:/# date
Fri Apr 28 10:29:40 PDT 2023
root@sonic:/# cat /etc/timezone
America/Los_Angeles
root@sonic:/# ls /etc/localtime -l
lrwxrwxrwx 1 root root 39 Apr 28 10:24 /etc/localtime -> /usr/share/zoneinfo/America/Los_Angeles
root@sonic:/#

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

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

Respective builds will be consumed by the customer.

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 27, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

madhupalu
madhupalu previously approved these changes Mar 1, 2023
Copy link

@madhupalu madhupalu left a comment

Choose a reason for hiding this comment

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

Code changes looks good.
LGTM

@madhupalu
Copy link

@gechiang
Can you take a look and approve these changes?
Thanks,
Madhu

gechiang
gechiang previously approved these changes Mar 7, 2023
@madhupalu
Copy link

@lguohan @xumia and @qiluo-msft
This issue being triaged as p1, couple of approvals already received.
Can you take a look and approve it?

Thanks,
-Madhu

@lguohan
Copy link
Collaborator

lguohan commented Mar 13, 2023

the description is not clear, can you please update the description so we know clear the problem is? @nmoray

@nmoray
Copy link
Contributor Author

nmoray commented Mar 14, 2023

the description is not clear, can you please update the description so we know clear the problem is? @nmoray

Sure. Updated the description with more insights of the actual issue. @lguohan

@nmoray
Copy link
Contributor Author

nmoray commented Mar 24, 2023

@lguohan: Gentle reminder!

@lguohan
Copy link
Collaborator

lguohan commented Mar 27, 2023

the problem seems not specific to frr, why other containers do not need this? @qiluo-msft , do you have any concern for mount them into the container?

rules/docker-fpm-frr.mk Outdated Show resolved Hide resolved
@madhupalu
Copy link

@lguohan I believe all the questions answered, can you please take a final review and approve the commit.

@lguohan
Copy link
Collaborator

lguohan commented Apr 11, 2023

@qiluo-msft can you take a look?

@lguohan
Copy link
Collaborator

lguohan commented Apr 11, 2023

@madhupalu should we apply this to all containers? Why bgp only?

@nmoray
Copy link
Contributor Author

nmoray commented Apr 18, 2023

  1. timedatectl set-timezone America/Los_Angeles

@madhupalu should we apply this to all containers? Why bgp only?

Yes @lguohan, same logic can be applied to all other dockers as well to sync the time across all the containers. Just that the user needs to make sure it will do a "config reload" (and not just a specific container restart) after setting a specific timezone to update all the containers at the same time with the new timezone and local time.

@nmoray nmoray dismissed stale reviews from gechiang and madhupalu via c1764eb April 18, 2023 12:19
@nmoray
Copy link
Contributor Author

nmoray commented Apr 18, 2023

  1. timedatectl set-timezone America/Los_Angeles

@madhupalu should we apply this to all containers? Why bgp only?

Yes @lguohan, same logic can be applied to all other dockers as well to sync the time across all the containers. Just that the user needs to make sure it will do a "config reload" (and not just a specific container restart) after setting a specific timezone to update all the containers at the same time with the new timezone and local time.

As requested, I have updated *.mk files of all the dockers to get the time sync across all the dockers.

@nyanto
Copy link

nyanto commented Apr 19, 2023

+1 This improvement if fairly straightforward and is deployed in our environment.
It would be good if it can be merged in soon.

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.

@madhupalu
Copy link

@nmoray thanks for taking care of all the docker services, @lguohan @qiluo-msft if there are no comments could you please approve the PR. Also let us know if it needs to ported any other releases. Thanks

@madhupalu
Copy link

@nmoray do you have any pending code changes to address this PR review comments?

@nmoray
Copy link
Contributor Author

nmoray commented May 18, 2023

@nmoray do you have any pending code changes to address this PR review comments?

Yes, I do @madhupalu. I will soon update the PR to address @qiluo-msft comment.

…natively, creating the respective softlink at the time of docker init.
@nmoray
Copy link
Contributor Author

nmoray commented May 23, 2023

@qiluo-msft As per your comment, I am no more mounting the symlink (/etc/localtime). Alternatively, creating that at run time based on the already mounted /etc/timezone. Please review and share your comments.

@madhupalu FYI

@@ -95,4 +95,8 @@ do
fi
done

TZ=$(cat /etc/timezone)
rm -rf /etc/localtime
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 5, 2023

Choose a reason for hiding this comment

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

rm

This script is only run once when container starting. So the motivation "the auto time / zone sync between the container(s) and host" is not fully achieved. If after container staring, user change timezone on the host, it will not sync to the container. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we consider an actual deployment scenario, timezone is configured only once before going to the Production. It is not the config which keeps getting changed again and again. But still, in case network admin wants to change the timezone post deployment, it can still be synced across the containers by triggering the config-reload command post changing the timezone on the host.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not agree "timezone is configured only once before going to the Production". The feature name 'Time sync' seems not accurate. You only propagate it when container starting, not sync after.

Copy link
Contributor Author

@nmoray nmoray Jun 12, 2023

Choose a reason for hiding this comment

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

Once the initial timezone sync has been completed between the host and container (during docker-init), later there will not any mismatch until the timezone on the host has been changed (Which is sort of periodic sync only). And to tackle that user needs to do config-reload to propagate updated timezone inside the containers.

Can you please tell me the actual used case where we need to change the timezone frequently and config reload / container restart is not allowed?

In my opinion, user will at least get some sort of clean mechanism to sync the timezone between the host and containers with this approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We always use UTC timezone so we do not have such use case. You raised the original issue #13046. In order to fix your use case, change timezone inside FRR container is good enough. For a timezone fix, there is no need to disruptive dataplane traffic.

Copy link
Contributor Author

@nmoray nmoray Jun 15, 2023

Choose a reason for hiding this comment

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

Initially I was focusing only on the FRR but later I found that this feature is required for other containers too. For instance, sairedis logs from swss where the timezone is synced correctly with the swss container and the host.


root@sonic:/var/log/swss# tail -f sairedis.rec
2023-06-14.21:23:20.593741|s|SAI_OBJECT_TYPE_PORT:oid:0x1000000000133|SAI_PORT_ATTR_QOS_DSCP_TO_TC_MAP=oid:0x140000000003d3
2023-06-14.21:23:20.594524|s|SAI_OBJECT_TYPE_PORT:oid:0x1000000000133|SAI_PORT_ATTR_QOS_TC_TO_QUEUE_MAP=oid:0x140000000003d5
2023-06-14.21:23:20.595066|s|SAI_OBJECT_TYPE_PORT:oid:0x1000000000133|SAI_PORT_ATTR_QOS_TC_TO_PRIORITY_GROUP_MAP=oid:0x140000000003d4
2023-06-14.21:23:20.595703|s|SAI_OBJECT_TYPE_PORT:oid:0x1000000000133|SAI_PORT_ATTR_PRIORITY_FLOW_CONTROL=24
2023-06-14.21:24:16.185792|c|SAI_OBJECT_TYPE_HOSTIF_TRAP_GROUP:oid:0x110000000003d7|SAI_HOSTIF_TRAP_GROUP_ATTR_QUEUE=2
2023-06-14.21:24:16.187074|c|SAI_OBJECT_TYPE_POLICER:oid:0x120000000003d8|SAI_POLICER_ATTR_CBS=1000|SAI_POLICER_ATTR_CIR=1000|SAI_POLICER_ATTR_METER_TYPE=SAI_METER_TYPE_PACKETS|SAI_POLICER_ATTR_MODE=SAI_POLICER_MODE_SR_TCM|SAI_POLICER_ATTR_RED_PACKET_ACTION=SAI_PACKET_ACTION_DROP
2023-06-14.21:24:16.187953|s|SAI_OBJECT_TYPE_HOSTIF_TRAP_GROUP:oid:0x110000000003d7|SAI_HOSTIF_TRAP_GROUP_ATTR_POLICER=oid:0x120000000003d8
2023-06-14.21:24:16.191987|c|SAI_OBJECT_TYPE_HOSTIF:oid:0xd0000000003d9|SAI_HOSTIF_ATTR_GENETLINK_MCGRP_NAME=packets|SAI_HOSTIF_ATTR_TYPE=SAI_HOSTIF_TYPE_GENETLINK|SAI_HOSTIF_ATTR_NAME=psample
2023-06-14.21:24:16.193059|c|SAI_OBJECT_TYPE_HOSTIF_TRAP:oid:0x220000000003da|SAI_HOSTIF_TRAP_ATTR_TRAP_TYPE=SAI_HOSTIF_TRAP_TYPE_SAMPLEPACKET|SAI_HOSTIF_TRAP_ATTR_TRAP_GROUP=oid:0x110000000003d7|SAI_HOSTIF_TRAP_ATTR_PACKET_ACTION=SAI_PACKET_ACTION_TRAP|SAI_HOSTIF_TRAP_ATTR_TRAP_PRIORITY=1
2023-06-14.21:24:16.200215|c|SAI_OBJECT_TYPE_HOSTIF_TABLE_ENTRY:oid:0x230000000003db|SAI_HOSTIF_TABLE_ENTRY_ATTR_TYPE=SAI_HOSTIF_TABLE_ENTRY_TYPE_TRAP_ID|SAI_HOSTIF_TABLE_ENTRY_ATTR_TRAP_ID=oid:0x220000000003da|SAI_HOSTIF_TABLE_ENTRY_ATTR_CHANNEL_TYPE=SAI_HOSTIF_TABLE_ENTRY_CHANNEL_TYPE_GENETLINK|SAI_HOSTIF_TABLE_ENTRY_ATTR_HOST_IF=oid:0xd0000000003d9

root@sonic:/var/log/swss# date
Wed Jun 14 21:25:40 PDT 2023

Thus, this feature will fix #13046 and other similar issues too.
BTW, this feature will be optional only. It is not going to harm or disrupt any exiting operation. If someone wants to sync the timezone across the containers, he / she can use it else others can live with the UTC timezone only. There is no force (for the ones who doesn't want to change the timezone) to disrupt the dataplane traffic and the ones who wants to change it, can plan it accordingly as per their deployment strategies. There are several customers, who prefers to have the timezone set as per their geographical location. Thus, in my opinion, it is a good to have feature.

To answer to your previous comment "The feature name 'Time sync' seems not accurate. You only propagate it when container starting, not sync after", we can certainly achieve the periodic sync by simply adding a script per container which will periodically check for the change of TZ on the host but it will be an overhead just for syncing the timezone. Thus, I decided to sync it only at the time of docker init as usually that config is not being updated frequently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you explore the solution of

docker run -e TZ=HOSTTIMEZONE image command
docker create image -e TZ=HOSTTIMEZONE

The benefit is centralized code change. And it behaves the same way only running once when container starting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it works if we already knew the TIMEZONE. For instance, we can set ENV in Dockerfile to pre-set the TZ. But how are we going to make the TZ as variable field in SONIC?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I realize above 2 commands only work during creating the container, not during container restart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah :-)

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Jun 5, 2023

Please change the PR title, it is not a FRR specific PR.
Please add issue link in the PR description. So people can click the link.


In reply to: 1576190297

@nmoray
Copy link
Contributor Author

nmoray commented Jun 5, 2023

Please change the PR title, it is not a FRR specific PR. Please add issue link in the PR description. So people can click the link.

@nmoray nmoray closed this Jun 5, 2023
@nmoray nmoray reopened this Jun 5, 2023
@nmoray
Copy link
Contributor Author

nmoray commented Jun 5, 2023

Please change the PR title, it is not a FRR specific PR. Please add issue link in the PR description. So people can click the link.

I have updated the PR title as well as the respective description.

@nmoray nmoray changed the title Fixed#13046: Frr - SONiC time sync issue Time sync issue between the host and containers Jun 5, 2023
@nmoray nmoray requested a review from qiluo-msft June 6, 2023 04:43
@madhupalu
Copy link

@qiluo-msft @lguohan
@nmoray had addressed review comments, can you help to get approve this PR?
Thanks

@nmoray nmoray changed the title Time sync issue between the host and containers Timezone sync issue between the host and containers Jun 12, 2023
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 20, 2023

Choose a reason for hiding this comment

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


This file is empty.

Do you want to add

TZ=$(cat /etc/timezone)
rm -rf /etc/localtime
ln -sf /usr/share/zoneinfo/$TZ /etc/localtime
``` #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. I missed it. Thanks for pointing it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the file.

[program:start]
command=/usr/bin/start.sh
priority=2
autostart=false
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 20, 2023

Choose a reason for hiding this comment

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

false

true? #Closed

Copy link
Contributor Author

@nmoray nmoray Jun 21, 2023

Choose a reason for hiding this comment

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

Correct me if I am wrong but I guess it is not required as the dependent_startup flag is true. So it will eventually gets started post dependent-startup. Also, I have referred other supervisord.conf files and found that it is most of the times false only.

@nmoray nmoray requested a review from qiluo-msft June 21, 2023 04:19
@qiluo-msft qiluo-msft merged commit f978b2b into sonic-net:master Jun 25, 2023
sonic-otn pushed a commit to sonic-otn/sonic-buildimage that referenced this pull request Sep 20, 2023
#### Why I did it
To fix the timezone sync issue between the containers and the host. If a certain timezone has been configured on the host (SONIC) then the expectation is to reflect the same across all the containers.

This will fix [Issue:13046](sonic-net#13046).

For instance, a PST timezone has been set on the host and if the user checks the link flap logs (inside the FRR), it shows the UTC timestamp. Ideally, it should be PST.
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.

Time sync issue between SONiC and FRR stack
6 participants