[NTP] 🐞 Fix config template to init default parameters by fastiuk · Pull Request #18736 · sonic-net/sonic-buildimage

fastiuk · 2024-04-21T00:39:30Z

fixes #17906

Why I did it

To fix NTP config generation from the minigraph and save backward compatability

Work item tracking

Microsoft ADO (number only):

How I did it

Align ntp.conf.j2 template to generate config out of empty NTP_SERVER DB configuration

How to verify it

Out of that NTP_SERVER configuration:

{
  "10.210.25.32": {},
  "10.75.202.2": {}
}

The next config in ntp.conf file should be produced:

server 10.210.25.32
restrict 10.210.25.32 kod limited nomodify notrap noquery nopeer

server 10.75.202.2
restrict 10.75.202.2 kod limited nomodify notrap noquery nopeer

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 (it is my cat Finn)

qiluo-msft · 2024-04-21T23:38:27Z

You mentioned "The NTP_SERVER configuration generated from the minigraph doesn't meet the new schema requirements" in the issue. To solve the problem, it is better to fix the schema (https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-ntp.yang) instead of fixing the config generation behavior. The behavior is old from 2017. Any new schema design should be backward-compatible. #Closed

qiluo-msft

Block as above comment.

ganglyu · 2024-04-22T00:37:11Z

Please check db_migrator, https://github.com/sonic-net/sonic-utilities/blob/master/scripts/db_migrator.py
Do we need to update db_migrator to migrate old ntp config to new schema?

fastiuk · 2024-04-22T08:42:15Z

You mentioned "The NTP_SERVER configuration generated from the minigraph doesn't meet the new schema requirements" in the issue. To solve the problem, it is better to fix the schema (https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-ntp.yang) instead of fixing the config generation behavior. The behavior is old from 2017. Any new schema design should be backward-compatible.

Minigraph doesn't use YANG to generate config, because there are default values in YANG. Anyway I already changed ntp.conf.j2 template according to your request, so it will preserve backward compatibility

fastiuk · 2024-04-22T08:42:54Z

Please check db_migrator, https://github.com/sonic-net/sonic-utilities/blob/master/scripts/db_migrator.py Do we need to update db_migrator to migrate old ntp config to new schema?

No need according to changes @qiluo-msft requested

fastiuk · 2024-04-27T21:33:31Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2024-04-27T21:33:34Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2024-04-27T21:33:46Z

Azure Pipelines successfully started running 1 pipeline(s).

fastiuk · 2024-04-28T17:23:32Z

The PR checker was failed because ntpsec does not support nopeer and notrap flags anymore: https://docs.ntpsec.org/latest/accopt.html
Fixed it by removing nopeer and notrap flags.

fastiuk · 2024-04-30T12:51:01Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2024-04-30T12:51:04Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2024-04-30T12:51:15Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>

anamehra · 2024-05-06T02:57:24Z

How do we configure some of the config params like config.iburst, config.resolve_as, etc. for NTP server used in this j2 file via sonic-mgmt? Is there any document to refer to?

liat-grozovik · 2024-05-07T10:39:57Z

@oleksandrivantsiv @dgsudharsan could you please help to review this fix?

liat-grozovik · 2024-05-07T10:40:13Z

/azpw run Azure.sonic-buildimage

dgsudharsan · 2024-05-08T04:04:28Z

-{{ config.association_type }} {{ config_as }}{{ soptions }}
+{{ association_type }} {{ resolve_as }}{{ soptions }}
 {% if global.server_role == 'disabled' %}
-restrict {{ config_as }} kod limited nomodify notrap noquery{{ aoptions }}


Can you please clarify why notrap and aoptions are removed?

nopeer and notrap is not supported by ntp-sec anymore. You can check its reference here: https://docs.ntpsec.org/latest/accopt.html
aoptions was used to add nopeer based on condition (if server is not a pool), but now it is not needed anymore.

@fastiuk Then we may need to raise a separate PR for 202311 as 202311 doesn't use bookworm. @saiarcot895 FYI

fastiuk · 2024-05-08T21:23:07Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2024-05-08T21:23:10Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2024-05-08T21:23:22Z

Azure Pipelines successfully started running 1 pipeline(s).

fastiuk · 2024-05-09T07:50:31Z

How do we configure some of the config params like config.iburst, config.resolve_as, etc. for NTP server used in this j2 file via sonic-mgmt? Is there any document to refer to?

config.iburst only trough the DB. So far only server can be added via SONiC CLI. config.resolve_as is internal option for a NTP server and shouldn't be configured directly. It used to save resolved address, because when you configure time.google.com for example it changes address every time. So now config.resolve_as is configured via SONiC CLI when new NTP server is added.

fastiuk · 2024-05-09T07:51:38Z

@oleksandrivantsiv @dgsudharsan could you please help to review this fix?

@oleksandrivantsiv @dgsudharsan : checkers are passed. All comments handled, please approve if looks good

fastiuk · 2024-05-14T20:05:49Z

@qiluo-msft the PR was approved. Could you please merge it?

fixes sonic-net#17906 To fix NTP config generation from the minigraph and save backward compatability Align `ntp.conf.j2` template to generate config out of empty `NTP_SERVER` DB configuration Out of that NTP_SERVER configuration: ```json { "10.210.25.32": {}, "10.75.202.2": {} } ``` The next config in `ntp.conf` file should be produced: ``` server 10.210.25.32 restrict 10.210.25.32 kod limited nomodify notrap noquery nopeer server 10.75.202.2 restrict 10.75.202.2 kod limited nomodify notrap noquery nopeer ``` Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>

fixes #17906 To fix NTP config generation from the minigraph and save backward compatability Align `ntp.conf.j2` template to generate config out of empty `NTP_SERVER` DB configuration Out of that NTP_SERVER configuration: ```json { "10.210.25.32": {}, "10.75.202.2": {} } ``` The next config in `ntp.conf` file should be produced: ``` server 10.210.25.32 restrict 10.210.25.32 kod limited nomodify notrap noquery nopeer server 10.75.202.2 restrict 10.75.202.2 kod limited nomodify notrap noquery nopeer ``` Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>

fastiuk changed the title ~~🐞 NTP: Fix minigraph config generation~~ [NTP] 🐞 Fix minigraph config generation Apr 21, 2024

fastiuk self-assigned this Apr 21, 2024

fastiuk added the Bug 🐛 label Apr 21, 2024

fastiuk requested review from lguohan, oleksandrivantsiv, qiluo-msft and saiarcot895 April 21, 2024 00:41

fastiuk force-pushed the dev-ntp-fix branch from 08b5e7c to 2d6f354 Compare April 21, 2024 13:34

qiluo-msft requested changes Apr 21, 2024

View reviewed changes

qiluo-msft requested review from ganglyu and wen587 April 21, 2024 23:39

fastiuk force-pushed the dev-ntp-fix branch from 2d6f354 to ad03fae Compare April 22, 2024 08:39

fastiuk changed the title ~~[NTP] 🐞 Fix minigraph config generation~~ [NTP] 🐞 Fix config template to init default parameters Apr 22, 2024

fastiuk closed this Apr 22, 2024

fastiuk reopened this Apr 22, 2024

fastiuk force-pushed the dev-ntp-fix branch from ad03fae to 407ff2d Compare April 23, 2024 21:26

fastiuk closed this Apr 27, 2024

fastiuk reopened this Apr 27, 2024

fastiuk force-pushed the dev-ntp-fix branch from 407ff2d to cf1c102 Compare April 28, 2024 17:21

fastiuk force-pushed the dev-ntp-fix branch from cf1c102 to 3f3abd7 Compare April 29, 2024 16:12

fastiuk requested a review from qiluo-msft April 29, 2024 16:13

fastiuk force-pushed the dev-ntp-fix branch 2 times, most recently from 5210cce to 214d453 Compare May 4, 2024 20:37

🐞 NTP: Fix config template to init default parameters

aba2912

Signed-off-by: Yevhen Fastiuk <yfastiuk@nvidia.com>

fastiuk force-pushed the dev-ntp-fix branch from 214d453 to aba2912 Compare May 4, 2024 20:38

saiarcot895 added the Request for 202311 Branch label May 5, 2024

anamehra mentioned this pull request May 7, 2024

Issue with NTP Server Synchronization - Clock Unsynchronized #18085

Open

dgsudharsan reviewed May 8, 2024

View reviewed changes

abdosi added the P0 Priority of the issue label May 8, 2024

oleksandrivantsiv approved these changes May 10, 2024

View reviewed changes

dgsudharsan approved these changes May 10, 2024

View reviewed changes

anamehra approved these changes May 10, 2024

View reviewed changes

saiarcot895 approved these changes May 10, 2024

View reviewed changes

qiluo-msft approved these changes May 15, 2024

View reviewed changes

qiluo-msft merged commit b952caa into sonic-net:master May 15, 2024

fastiuk mentioned this pull request May 16, 2024

[202311][cherry-pick][NTP] 🐞 Fix config template to init default parameters #18981

Merged

12 tasks

yxieca added the Included in 202311 Branch label May 28, 2024

Conversation

fastiuk commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

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 (it is my cat Finn)

Uh oh!

qiluo-msft commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

ganglyu commented Apr 22, 2024

Uh oh!

fastiuk commented Apr 22, 2024

Uh oh!

fastiuk commented Apr 22, 2024

Uh oh!

fastiuk commented Apr 27, 2024

Uh oh!

mssonicbld commented Apr 27, 2024

Uh oh!

azure-pipelines bot commented Apr 27, 2024

Uh oh!

fastiuk commented Apr 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fastiuk commented Apr 30, 2024

Uh oh!

mssonicbld commented Apr 30, 2024

Uh oh!

azure-pipelines bot commented Apr 30, 2024

Uh oh!

anamehra commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liat-grozovik commented May 7, 2024

Uh oh!

liat-grozovik commented May 7, 2024

Uh oh!

dgsudharsan May 8, 2024

Choose a reason for hiding this comment

Uh oh!

fastiuk May 8, 2024

Choose a reason for hiding this comment

Uh oh!

dgsudharsan May 10, 2024

Choose a reason for hiding this comment

Uh oh!

fastiuk commented May 8, 2024

Uh oh!

mssonicbld commented May 8, 2024

Uh oh!

azure-pipelines bot commented May 8, 2024

Uh oh!

fastiuk commented May 9, 2024

Uh oh!

fastiuk commented May 9, 2024

Uh oh!

fastiuk commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

fastiuk commented Apr 21, 2024 •

edited

Loading

qiluo-msft commented Apr 21, 2024 •

edited

Loading

fastiuk commented Apr 28, 2024 •

edited

Loading

anamehra commented May 6, 2024 •

edited

Loading