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.

Sign up for GitHub

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

Jump to bottom

cloud-config user-data: ntp to provide ntp_client: ntpsec support #5631

Closed
wants to merge 2 commits into from
Closed

cloud-config user-data: ntp to provide ntp_client: ntpsec support #5631

wants to merge 2 commits into from

Conversation

blackboxsw
Copy link
Collaborator

@blackboxsw blackboxsw commented Aug 21, 2024 •
edited
Loading

Due to the ntp being a transitional package in may install ntpd or ntpsec on different distributions, it may be preferred
to specify ntpsec as the preferred ntp_client in general to strictly define the client that an admin wishes to install/enable. Provide a mechanism to directly prefer a specific ntp_client: ntpsec instead of the ntp transitional package.

Also, in Ubuntu, daily integration tests against development Ubuntu series will occasionally hit issues with APT dependency resolution when trying to provide user-data declaring a preference for ntp transitional package when systemd-timesyncd packages in Ubuntu cloud images are out of date with latest published packages in the -updates pocket. APT is unable to resolve a stale systemd-timesyncd package as it Provides: time-daemon which conflicts with ntpsec which also Provides: time-daemon.

Proposed Commit Message

see individual commits

  • pytestify tests/unittests/config/test_cc_ntp.py
  • Add ntp_client: ntpsec support to #cloud-config

Additional Context

Intermittent yet repeated ntp error on ubuntu devel releases
https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-oracular-lxd_container/48/

tests.integration_tests.modules.test_ntp_servers.TestNtpServers.test_ntp_installed (from pytest)
Failing for the past 3 builds (Since Unstable
[#46](https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-oracular-lxd_container/46/) )
[Took 20 sec.](https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-oracular-lxd_container/48/testReport/junit/tests.integration_tests.modules.test_ntp_servers/TestNtpServers/test_ntp_installed/history)
Error Message

AssertionError: assert False
 +  where False = ''.ok
 +    where '' = execute('ntpd --version')
 +      where execute = <tests.integration_tests.instances.IntegrationInstance object at 0x7fb95540a0b0>.execute

Test Steps

CLOUD_INIT_OS_IMAGE=noble CLOUD_INIT_CLOUD_INIT_SOURCE=IN_PLACE tox -e integration-tests -- tests/integration_tests/modules/test_ntp_servers.py::TestNtpServers
CLOUD_INIT_OS_IMAGE=oracular CLOUD_INIT_CLOUD_INIT_SOURCE=IN_PLACE tox -e integration-tests -- tests/integration_tests/modules/test_ntp_servers.py::TestNtpServers

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@github-actions github-actions bot added the documentation This Pull Request changes documentation label Aug 21, 2024
blackboxsw added a commit to blackboxsw/cloud-init that referenced this pull request Aug 21, 2024
@blackboxsw
feat: add netsec ntp_client support for ntp client configuration (can…
09bd0ce 
…onical#5631)

In many distributions, ntp is a transitional package that provides
a mechansim to install either ntpd or ntpsec.

Installing ntp transitional package may cause issues with APT package
dependency resolution when APT sources are not updated and latest
systemd-timesyncd is out of date because both systemd-timesyncd
and ntpsec declare a Provides: time-daemon and conflict with an
unresolvable version upgrade path unless the conflicting package
also happens to be fully updated.

Add #cloud-config 'ntpsec' option for 'ntp_client' in user-data to
allow opting into the specific ntpsec client over transitional ntp
package on alpine, debian, redhat, sles and ubuntu.

Add template config files for rhel, sles, ubuntu, debian and alpine
obtain by pulling latest configuration files from lxc container launches
of each distribution.

Add json schema strict enum of ntp_client type to better document and
limit ntp_client values to valid configuration.

Update integration tests to prefer ntpsec over ntp
@blackboxsw blackboxsw force-pushed the ntp-provide-ntpsec-client-option branch from fd75c51 to 09bd0ce Compare August 21, 2024 23:57
blackboxsw added a commit to blackboxsw/cloud-init that referenced this pull request Aug 22, 2024
@blackboxsw
chore: adapt cc_ntp unittests to pytest (canonical#5631)
2965954 
- Avoid subp call to ifconfig -a as side-effect of __init__ in
  cloudinit.distros.networking.BSDNetworking by declaring a cached
  @Property 'ifs' that is only called when needed to avoid mocking
- Drop CiTestCase and FilesystemMockingTestCase uses in favor
  of pytest, fixtures and tmpdir/caplog use
- pytest parametrize a couple of tests based on ntp_client input
  params to avoid nested for loops in unittests
@blackboxsw blackboxsw force-pushed the ntp-provide-ntpsec-client-option branch from 09bd0ce to 95c0b51 Compare August 22, 2024 03:19
blackboxsw added a commit to blackboxsw/cloud-init that referenced this pull request Aug 22, 2024
@blackboxsw
feat: add netsec ntp_client support for ntp client configuration (can…
95c0b51 
…onical#5631)

In many distributions, ntp is a transitional package that provides
a mechansim to install either ntpd or ntpsec.

Installing ntp transitional package may cause issues with APT package
dependency resolution when APT sources are not updated and latest
systemd-timesyncd is out of date because both systemd-timesyncd
and ntpsec declare a Provides: time-daemon and conflict with an
unresolvable version upgrade path unless the conflicting package
also happens to be fully updated.

Add #cloud-config 'ntpsec' option for 'ntp_client' in user-data to
allow opting into the specific ntpsec client over transitional ntp
package on alpine, debian, redhat, sles and ubuntu.

Add template config files for rhel, sles, ubuntu, debian and alpine
obtain by pulling latest configuration files from lxc container launches
of each distribution.

Add json schema strict enum of ntp_client type to better document and
limit ntp_client values to valid configuration.

Update integration tests to prefer ntpsec over ntp
blackboxsw added a commit to blackboxsw/cloud-init that referenced this pull request Aug 23, 2024
@blackboxsw
chore: adapt cc_ntp unittests to pytest (canonical#5631)
4369a50 
- Avoid subp call to ifconfig -a as side-effect of __init__ in
  cloudinit.distros.networking.BSDNetworking by declaring a cached
  @Property 'ifs' that is only called when needed to avoid mocking
- Drop CiTestCase and FilesystemMockingTestCase uses in favor
  of pytest, fixtures and tmpdir/caplog use
- pytest parametrize a couple of tests based on ntp_client input
  params to avoid nested for loops in unittests
blackboxsw added a commit to blackboxsw/cloud-init that referenced this pull request Aug 23, 2024
@blackboxsw
feat: add netsec ntp_client support for ntp client configuration (can…
931357f 
…onical#5631)

In many distributions, ntp is a transitional package that provides
a mechansim to install either ntpd or ntpsec.

Installing ntp transitional package may cause issues with APT package
dependency resolution when APT sources are not updated and latest
systemd-timesyncd is out of date because both systemd-timesyncd
and ntpsec declare a Provides: time-daemon and conflict with an
unresolvable version upgrade path unless the conflicting package
also happens to be fully updated.

Add #cloud-config 'ntpsec' option for 'ntp_client' in user-data to
allow opting into the specific ntpsec client over transitional ntp
package on alpine, debian, redhat, sles and ubuntu.

Add template config files for rhel, sles, ubuntu, debian and alpine
obtain by pulling latest configuration files from lxc container launches
of each distribution.

Add json schema strict enum of ntp_client type to better document and
limit ntp_client values to valid configuration.

Update integration tests to prefer ntpsec over ntp
@blackboxsw blackboxsw force-pushed the ntp-provide-ntpsec-client-option branch from 95c0b51 to 931357f Compare August 23, 2024 02:45
@blackboxsw
chore: adapt cc_ntp unittests to pytest (canonical#5631)
a6fbcbe 
- Avoid subp call to ifconfig -a as side-effect of __init__ in
  cloudinit.distros.networking.BSDNetworking by declaring a cached
  @Property 'ifs' that is only called when needed to avoid mocking
- Drop CiTestCase and FilesystemMockingTestCase uses in favor
  of pytest, fixtures and tmpdir/caplog use
- pytest parametrize a couple of tests based on ntp_client input
  params to avoid nested for loops in unittests
@blackboxsw
feat: add netsec ntp_client support for ntp client configuration (can…
9c66960 
…onical#5631)

In many distributions, ntp is a transitional package that provides
a mechansim to install either ntpd or ntpsec.

Installing ntp transitional package may cause issues with APT package
dependency resolution when APT sources are not updated and latest
systemd-timesyncd is out of date because both systemd-timesyncd
and ntpsec declare a Provides: time-daemon and conflict with an
unresolvable version upgrade path unless the conflicting package
also happens to be fully updated.

Add #cloud-config 'ntpsec' option for 'ntp_client' in user-data to
allow opting into the specific ntpsec client over transitional ntp
package on alpine, debian, redhat, sles and ubuntu.

Add template config files for rhel, sles, ubuntu, debian and alpine
obtain by pulling latest configuration files from lxc container launches
of each distribution.

Add json schema strict enum of ntp_client type to better document and
limit ntp_client values to valid configuration.

Update integration tests to prefer ntpsec over ntp
@blackboxsw blackboxsw force-pushed the ntp-provide-ntpsec-client-option branch from 931357f to 9c66960 Compare August 23, 2024 03:34
@aciba90 aciba90 self-assigned this Aug 29, 2024
dermotbradley
dermotbradley reviewed Aug 30, 2024
View reviewed changes
templates/ntpsec.conf.alpine.tmpl
Copy link
Contributor

Choose a reason for hiding this comment

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

Having this file doesn't make sense as Busybox's ntpd is limited and does not support NTPSEC.

ntpsec and openntpd are packages for Alpine but cloud-init doesn't currently support them (or ntpd-rs either, which I might consider adding support for in the future).

aciba90
aciba90 reviewed Sep 3, 2024
View reviewed changes
Copy link
Contributor

@aciba90 aciba90 left a comment

Choose a reason for hiding this comment

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

General approach looks good, thanks @blackboxsw.

I left an inline question about an integration tests. In addition to that, could you please resolve the merge conflict?

tests/integration_tests/modules/test_ntp_servers.py
pytest.skip("/etc/ntp.conf.dist does not exist")
dist_file = class_client.read_from_file("/etc/ntp.conf.dist")
if class_client.execute(
"test -e /etc/ntpsec/ntp.conf.dpkg-dist"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we operate over /etc/ntpsec/ntp.conf.dist?

Suggested change
"test -e /etc/ntpsec/ntp.conf.dpkg-dist"
"test -e /etc/ntpsec/ntp.conf.dist"

/etc/ntpsec/ntp.conf.dist is the file cloud-init renames if /etc/ntpsec/ntp.conf is present and /etc/ntpsec/ntp.conf.dpkg-dist is the one apt / dpkg creates on installation / upgrade paths.

tests/integration_tests/modules/test_ntp_servers.py
pytest.skip("/etc/ntpsec/ntp.conf.dpkg-dist does not exist")
dist_file = class_client.read_from_file(
"/etc/ntpsec/ntp.conf.dpkg-dist"
)
assert 0 == len(dist_file.strip().splitlines())
Copy link
Contributor

Choose a reason for hiding this comment

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

This assertion fails, the file has content. Is this due to my previous comment?

It feels like somewhere while the test instance is being customized or the last boot, ntpsec gets reinstalled with /etc/ntpsec/ntp.conf present and modified and then etc/ntpsec/ntp.conf.dpkg-dist gets created as a backup by dpkg.

Is this a testing error or what is the intention with this test?

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 5, 2024

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Oct 5, 2024
@github-actions github-actions bot closed this Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aciba90 aciba90 aciba90 left review comments

@dermotbradley dermotbradley dermotbradley left review comments

Assignees

@aciba90 aciba90

Labels
documentation This Pull Request changes documentation stale-pr Pull request is stale; will be auto-closed soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blackboxsw @dermotbradley @aciba90