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

Update NTP tests to use hostname #862

Closed
wants to merge 7 commits into from

Conversation

noursaidi
Copy link
Collaborator

@noursaidi noursaidi commented May 10, 2021

Additional discussion on issue #845

Updates the NTP tests to use the hostname rather than DHCP by adding a hostname ntp.daqlocal and the NTP server IP address from DHCP

@noursaidi noursaidi requested review from grafnu, pisuke and pbatta May 10, 2021 09:12
@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #862 (1f9f8c3) into master (cd59227) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #862      +/-   ##
==========================================
- Coverage   81.90%   81.77%   -0.14%     
==========================================
  Files          43       43              
  Lines        5273     5398     +125     
==========================================
+ Hits         4319     4414      +95     
- Misses        954      984      +30     
Flag Coverage Δ
ata 62.85% <16.66%> (?)
aux 68.52% <100.00%> (-0.08%) ⬇️
base 66.33% <16.66%> (-0.10%) ⬇️
dhcp 67.65% <16.66%> (-0.07%) ⬇️
many 67.78% <16.66%> (-0.46%) ⬇️
mud 72.72% <16.66%> (-0.24%) ⬇️
switch 66.69% <16.66%> (-0.05%) ⬇️
topo 66.71% <16.66%> (-0.19%) ⬇️
unit 30.84% <0.00%> (-0.28%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
daq/report.py 98.39% <100.00%> (+0.02%) ⬆️
daq/network.py 91.15% <0.00%> (-2.03%) ⬇️
daq/test_modules/ipaddr_module.py 87.61% <0.00%> (-1.86%) ⬇️
daq/base_gateway.py 92.98% <0.00%> (-1.79%) ⬇️
daq/runner.py 85.00% <0.00%> (-1.50%) ⬇️
daq/entry.py 90.27% <0.00%> (-0.27%) ⬇️
daq/host.py 90.94% <0.00%> (-0.14%) ⬇️
daq/proto/system_config_pb2.py 100.00% <0.00%> (ø)
daq/test_modules/base_module.py 100.00% <0.00%> (ø)
daq/test_modules/docker_module.py 100.00% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd59227...1f9f8c3. Read the comment docs.

@noursaidi noursaidi marked this pull request as draft May 10, 2021 15:14
# NTPv3 query to the IP of time.google.com (since resolv.conf is modified by other tests)
if [ -n "${options[ntpv4]}" ]; then
dhcp_ntp=$(fgrep NTPSERVERS= /run/ntpdate.dhcp)
ntp_server=`echo $dhcp_ntp | cut -d "'" -f 2`
ntp_server=ntp.daqlocal
Copy link
Collaborator

Choose a reason for hiding this comment

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

since ntp.daqlocal is used a couple of times, make it a named variable in this file? (LOCAL_NTP_SERVER_NAME=ntp.daqlocal or something like that)

ip addr add 10.20.$subnet.2 dev $LOCAL_IF
echo dhcp-option=6,10.20.$subnet.2 >> /etc/dnsmasq.conf
echo dhcp-option=42,10.20.$subnet.2 >> /etc/dnsmasq.conf
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we delete this? Leaving this in place would allow the DUT to either learn NTP from DHCP option, or have it statically configured, so we'll cover both options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, on the issue #845 there's been additional comments from Trevor, as well as your comment from last week on having both test cases, so this PR will be updated accordingly

@noursaidi noursaidi closed this Jun 7, 2021
@noursaidi noursaidi deleted the ntpbyhostname branch June 7, 2021 20:58
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.

2 participants