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

[BFW-5926] Enable DNS for sntp and set default to prusa3d.pool.ntp.org #3730

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

thess
Copy link
Contributor

@thess thess commented Feb 8, 2024

This PR changes the default behavior of the SNTP client to use DNS name resolution for an NTP server target. The main advantage is load-balancing servers by using NTP server pools (eg: pool.ntp.org) which rotate addresses periodically via DNS. Another reason is the DNS query response from the pool is distributed by geography of requester as well.

I think this change and #3679 are complimentary - both are desired.

Ref: https://www.ntppool.org/

@MattBlissett
Copy link

The NTP pool service doesn't allow people to use pool.ntp.org as a default, the vendor (Prusa) must ask for a custom name.

See https://www.ntppool.org/en/vendors.html#basic-guidelines

@thess
Copy link
Contributor Author

thess commented Apr 4, 2024

Yes, they do request you not use the default pool if you are a vendor. However, you can easily request and use a 'prusa' vendor pool (prusa.pool.ntp.org) and use that instead. It is better than the current hard-wired Czech CESNET NTP server and one will get better geographic locality.

Copy link
Member

@danopernis danopernis left a comment

Choose a reason for hiding this comment

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

Good news everyone, NTP pool approved our request for vendor zone 🥳

@thess can you please use prusa3d.pool.ntp.org ?

Also, this needs to be rebased onto current master. If you have difficulties with that, I can try rebasing in your fork, if I have the correct permissions.

Edit: I see there is also open #3679 by @bkerler

Can you folks please remind me what those two PRs were about? There seems to be more discussion in #3679 so maybe we should close this as a duplicate?

@bkerler
Copy link
Contributor

bkerler commented Jun 26, 2024

@danopernis this one is just enabling dns for sntp and enables ip pooling via pool.ntp.org. #3679 however is including this but does also enhance more, such as setting the dns ip manually instead of hardcoding, dns v6 support preparation and setting ntp via dns server. #3679 was also already reviewed by @vorner but is stalling right now.

@thess thess force-pushed the enable_sntp_dns branch from cbc9035 to c33b0d1 Compare June 28, 2024 12:17
@thess thess changed the title Enable DNS for sntp and set default to pool.ntp.org Enable DNS for sntp and set default to prusa3d.pool.ntp.org Jun 28, 2024
@thess
Copy link
Contributor Author

thess commented Jun 28, 2024

@danopernis - glad to hear you got a Prusa3D NTP pool zone. I have made the requested change and have rebased the PR to the master branch with that change. PR #3679 is a super-set of this change with a lot more options for setting an NTP server. This PR is addressing the problem of having a fixed IP for NTP server time-setting functionality only.

@thess
Copy link
Contributor Author

thess commented Jun 28, 2024

Not sure what this CI failure is about -- nothing in my PR deals with this.

[387/1020] Generating qoi.data, ../gui/res/qoi_resources.gen
FAILED: src/resources/qoi.data src/gui/res/qoi_resources.gen /home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/build/mini-en-pl_release_boot/src/resources/qoi.data /home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/build/mini-en-pl_release_boot/src/gui/res/qoi_resources.gen 
cd /home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/build/mini-en-pl_release_boot/src/resources && /home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/.venv/bin/python3.10 /home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/utils/qoi_packer.py /home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/src/gui/res/png /home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/build/mini-en-pl_release_boot/src/gui/res/qoi_resources.gen /home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/build/mini-en-pl_release_boot/src/resources/qoi.data -input_filter=/home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/src/gui/res/mini_used_imgs.txt
Traceback (most recent call last):
  File "/home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/utils/qoi_packer.py", line 6, in <module>
    import qoi
  File "/home/admin/workspace/rmware-Buddy_Multibranch_PR-3730/.venv/lib/python3.10/site-packages/qoi/__init__.py", line 2, in <module>
    from qoi.qoi import QOIColorSpace, decode, encode, read, write
  File "src/qoi/qoi.pyx", line 1, in init qoi.qoi
ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject

Edit: Error indicates a cpython API compatibility problem between qoi and numpy. The CI system has defaulted to numpy 2.0 which apparently is incompatible with qoi 0.5.0. Commit 2425c11 removed the numpy requirement for 1.24.6 which works with qoi. The file utils/holly/heavy-requirements.txt is not referenced anywhere. Shouldn't it be included in the Dockerfile script?

Copy link
Contributor Author

@thess thess left a comment

Choose a reason for hiding this comment

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

Updated as requested

@danopernis danopernis changed the title Enable DNS for sntp and set default to prusa3d.pool.ntp.org [BFW-5926] Enable DNS for sntp and set default to prusa3d.pool.ntp.org Aug 16, 2024
Copy link
Member

@danopernis danopernis left a comment

Choose a reason for hiding this comment

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

@thess thanks, I am merging this. Sorry for the inconveniences due to rebasing.

@danopernis danopernis merged commit 712663a into prusa3d:master Aug 16, 2024
1 check passed
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.

4 participants