Skip to content

Conversation

@andrewphelpsj
Copy link
Member

@andrewphelpsj andrewphelpsj commented Dec 17, 2025

This change enables the usage of timedatectl ntp-servers and timedatectl revert when a snap is connected to the timeserver-control interface.

Review usage of bus_network_mgr to see how these are used in timedatectl: https://github.com/systemd/systemd/blob/main/src/timedate/timedatectl.c

This is coming from a request from field.

SNAPDENG-36147

@andrewphelpsj andrewphelpsj added the Needs security review Can only be merged once security gave a :+1: label Dec 17, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the timeserver-control interface to support per-link NTP configuration through systemd-networkd. The changes enable snaps to use timedatectl ntp-servers and timedatectl revert commands by granting necessary D-Bus permissions to interact with systemd-networkd's Manager interface.

  • Adds D-Bus permissions for SetLinkNTP, RevertLinkNTP, and GetLinkByName methods on the network1 Manager interface
  • Implements test coverage for the new ntp-servers and revert subcommands
  • Updates interface tests to verify the new AppArmor rules are properly generated

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
interfaces/builtin/timeserver_control.go Adds D-Bus permission rules for systemd-networkd per-link NTP management
interfaces/builtin/timeserver_control_test.go Adds test assertions to verify new D-Bus paths and methods in AppArmor snippets
tests/main/interfaces-timeserver-control/task.yaml Adds integration tests for ntp-servers and revert commands when systemd-networkd is enabled

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

github-actions bot commented Dec 17, 2025

Thu Dec 18 17:35:07 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/20343655611

Failures:

Preparing:

  • openstack:ubuntu-core-24-64:tests/core/snapd-refresh

Executing:

  • openstack:arch-linux-64:tests/main/interfaces-timeserver-control
  • openstack:ubuntu-core-18-64:null
  • openstack:ubuntu-core-18-64:null
  • openstack:ubuntu-core-24-64:tests/main/dbus-activation-session
  • openstack:ubuntu-20.04-64:tests/main/degraded
  • openstack:ubuntu-20.04-64:tests/main/interfaces-timeserver-control
  • openstack:ubuntu-24.04-64:tests/main/interfaces-timeserver-control

@codecov
Copy link

codecov bot commented Dec 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.48%. Comparing base (1fed470) to head (ce86bd1).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #16378      +/-   ##
==========================================
- Coverage   77.82%   77.48%   -0.34%     
==========================================
  Files        1327     1339      +12     
  Lines      184888   183042    -1846     
  Branches     2438     2438              
==========================================
- Hits       143884   141830    -2054     
- Misses      32418    32615     +197     
- Partials     8586     8597      +11     
Flag Coverage Δ
unittests 77.48% <ø> (-0.34%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI review requested due to automatic review settings December 18, 2025 16:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

exit 1
fi
if systemctl list-unit-files | grep systemd-networkd | awk '{print $2}' | MATCH "enabled"; then
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The systemd-networkd detection logic is fragile. If systemd-networkd appears multiple times in list-unit-files output (e.g., systemd-networkd.service and systemd-networkd.socket), this will print multiple state values, potentially causing MATCH to fail unexpectedly. Consider using systemctl is-enabled systemd-networkd.service instead for a more reliable single-value check.

Suggested change
if systemctl list-unit-files | grep systemd-networkd | awk '{print $2}' | MATCH "enabled"; then
if systemctl is-enabled systemd-networkd.service 2>/dev/null | MATCH "enabled"; then

Copilot uses AI. Check for mistakes.
test-snapd-timedate-control-consumer.timedatectl-timeserver ntp-servers "${iface}" 127.0.0.1
test-snapd-timedate-control-consumer.timedatectl-timeserver revert "${iface}"
elif os.query is-core-ge 20; then
echo "Error: insufficient test coverage of timedatectl per-link NTP commands"
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

This error message could be more actionable. It indicates a test configuration issue but doesn't explain why the test can't run or what the user should do. Consider clarifying that systemd-networkd is required but not enabled, e.g., 'systemd-networkd is not enabled but is required for per-link NTP command testing on Core 20+'.

Suggested change
echo "Error: insufficient test coverage of timedatectl per-link NTP commands"
echo "Error: systemd-networkd is not enabled but is required for per-link NTP command testing on Core 20+"

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs security review Can only be merged once security gave a :+1:

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant