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

management framework NTP support HLD #69

Merged
merged 8 commits into from
Dec 8, 2020
Merged

management framework NTP support HLD #69

merged 8 commits into from
Dec 8, 2020

Conversation

bsun-sudo
Copy link
Collaborator

First draft of HLD for management framework NTP support

doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
Copy link

@ben-gale ben-gale left a comment

Choose a reason for hiding this comment

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

Thanks.

doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Show resolved Hide resolved
@anil-kolkaleleti
Copy link

1.1.6 Scalability Requirements
1.1.7 Warm Boot Requirements
Above section details are not updated in the HLD.
Warm boot is supported ??
MAX servers supported count??
3.6.3 section -- PUT is not mentioned, is it not supported ??

doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved
doc/SONiC_OC_NTP_HLD.md Outdated Show resolved Hide resolved

```

### 3.6.2 CLI
Copy link
Collaborator

@a-barboza a-barboza Oct 21, 2020

Choose a reason for hiding this comment

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

NTPD supports behaving as an NTP server to downstream (downStratum ?) NTP clients. Is there any plan on implementing this in the UI? Requests coming in from the field.

}
```

A change in the NTP_SERVER entry triggers hostcfgd to start the NTP configuration script, which in turn writes each NTP server to the ntp.conf and then restart the ntp service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Getting requests for avoiding NTP service restart if possible. I believe the ticket is created SNC-9556 (SONIC-33081). If the configuration state is maintained outside ntpd, would it be possible (for some cases) to configure the ntpd through NTP mode 6 control message incrementally instead of forcing a restart? Investigating @brcm as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"NTP|global": {
"type": "hash",
"value": {
"src_intf": "Ethernet36"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Per 0.5 version src_intf would be a list now (with the trailing @ symbol) ? Eg: "\src_intf@": "Loopback0,eth0"


sonic(config)# ntp server pool.ntp.org

```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Getting comments: "HI Arun
how many max ntp servers we can configure ?
i cannot find this in below hld
"
I found this link on ntp.conf advising on the number of NTP servers. (http://support.ntp.org/bin/view/Support/SelectingOffsiteNTPServers) Section 5.3.4. Excessive Number of Upstream Time Servers

| Delete NTP source interface| Verify that NTP source interface is removed from the configDB, NTP packets are transmitted and received over the default interface|
| Configure NTP vrf| Verify that NTP vrf is installed correctly in the configDB and ntp service is running in the specified VRF|
| | Verify that only default and mgmt can be configured as NTP vrf|
| Delete NTP vrf| Verify that NTP vrf is removed from the configDB and ntp service is running in the default instance|
Copy link
Collaborator

@a-barboza a-barboza Nov 16, 2020

Choose a reason for hiding this comment

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

There seems to be some confusion between Section 1.1.1.5 which talks about VRF configuration when Management VRF is configured (1.1.1.5 Overall Behavior related to NTP source interface and NTP vrf
, When mgmt VRF is configured ), and this Unit Test. Please clarify that this UT is being run when no Management VRF is configured ? (Reference JIRA ID https://it-jira.broadcom.net/browse/SONIC-33434) (Dell SNC-????)


Enhancing the management framework backend code and transformer methods to add support for NTP.

## 3.2 DB Changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please put all the config DB schema change details in this section, even though they are spread across multiple sections in the doc. Please consolidate them into this section.

@bhavini-gada bhavini-gada merged commit b000d3d into master Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants