snapd icon indicating copy to clipboard operation
snapd copied to clipboard
verified publisher icon snapcore

cmd/snap-update-ns: switch to a three-pass process

Open zyga opened this issue 1 year ago • 2 comments

Historically Change.Perform was a two-step process, first it would ensure that both the source and destination of the operation are ready and only then it would call the lowLevelPerform function to actually mount or unmount.

This works for simple cases but has the problem that the desired changes are interleaved with construction of writable mimics, especially in ensure{Source,Target}. This is a problem because it may end up performing a sequence of operations where a mount we perform is duplicated by the mimic, like in the example given below:

High-level view:

  • unmount /target
  • mount --bind /source1 /target/dir
  • mount --bind /source2 /target/dir/subdir

Low-level view:

  • umount --lazy /target
  • make writable mimic /target/dir
  • mount --bind /source1/ /target/dir
  • make writable mimic /target/dir/subdir
  • mount --bind /source2 /target/dir/subdir

The bind mount /target/dir now has two instances, one performed directly and then another that was created during the construction of a mimic at /target/dir/ in order to create the sub-directory subdir.

The new three-pass approach breaks that to:

  • apply all keep and unmount actions
  • prepare to apply all mount actions by ensuring source/target is ready and constructing any writable mimics necessary.
  • actually apply all mount actions

The new order of operations, for the example given above, is now:

New low-level view:

  • umount --lazy /target
  • make writable mimic /target/dir
  • make writable mimic /target/dir/subdir
  • mount --bind /source1/ /target/dir
  • mount --bind /source2 /target/dir/subdir

The functions related to preparation are now called PrepareToPerform and the functions related to actually doing the change are called DoPerform. A helper that calls one and then the other remains so that fewer overall changes are necessary. The lowLevelPerform method is now gone and is now the actual implementation of DoPerform.

Test mocking functions can now mock both sides individually. Mocking was also redesigned from the classical function variable due to the cycle caused by PrepareToPerform calling DoPerform internally.

Jira: https://warthogs.atlassian.net/browse/SNAPDENG-23293

Signed-off-by: Zygmunt Krynicki [email protected]

zyga avatar Oct 21 '24 15:10 zyga

Codecov Report

:x: Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 77.44%. Comparing base (1460b41) to head (e001b93).

Files with missing lines Patch % Lines
cmd/snap-update-ns/update.go 89.28% 2 Missing and 1 partial :warning:
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #14658      +/-   ##
==========================================
- Coverage   77.46%   77.44%   -0.02%     
==========================================
  Files        1333     1335       +2     
  Lines      181681   181741      +60     
  Branches     2398     2398              
==========================================
+ Hits       140741   140756      +15     
- Misses      32414    32450      +36     
- Partials     8526     8535       +9
Flag Coverage Δ
unittests 77.44% <93.18%> (-0.02%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Oct 21 '24 16:10 codecov[bot]

This has a number of regressions but I don't worry about it yet. I think it only makes sense to land after https://github.com/canonical/snapd/pull/14661 (EDIT: it's spiritual follow-up)

zyga avatar Oct 22 '24 07:10 zyga

Wed Nov 5 11:38:34 UTC 2025 The following results are from: https://github.com/canonical/snapd/actions/runs/19098882177

Failures:

Preparing:

  • openstack-ps7:ubuntu-core-24-64:tests/core/

Executing:

  • openstack-ps7:arch-linux-64:tests/regression/lp-1844496
  • openstack-ps7:arch-linux-64:tests/main/interface-allow-auto-connection:verified
  • openstack-ps7:arch-linux-64:tests/main/parallel-install-interfaces-content:data
  • openstack-ps7:arch-linux-64:tests/main/parallel-install-layout
  • openstack-ps7:arch-linux-64:tests/main/interface-allow-auto-connection:false
  • openstack-ps7:arch-linux-64:tests/main/parallel-install-interfaces-content:common
  • openstack-ps7:arch-linux-64:tests/main/parallel-install-interfaces-content:snap
  • openstack-ps7:arch-linux-64:tests/main/mounts-persist-refresh-content-snap
  • openstack-ps7:arch-linux-64:tests/main/interface-allow-auto-connection:unset
  • openstack-ps7:arch-linux-64:tests/main/interface-allow-auto-connection:true
  • openstack-ps7:debian-12-64:tests/regression/lp-1844496
  • openstack-ps7:debian-12-64:tests/main/parallel-install-layout
  • openstack-ps7:debian-12-64:tests/main/parallel-install-interfaces-content:common
  • openstack-ps7:debian-12-64:tests/main/interface-allow-auto-connection:unset
  • openstack-ps7:debian-12-64:tests/main/interface-allow-auto-connection:verified
  • openstack-ps7:debian-12-64:tests/main/interface-allow-auto-connection:false
  • openstack-ps7:debian-12-64:tests/main/mounts-persist-refresh-content-snap
  • openstack-ps7:debian-12-64:tests/main/parallel-install-interfaces-content:data
  • openstack-ps7:debian-12-64:tests/main/parallel-install-interfaces-content:snap
  • openstack-ps7:debian-12-64:tests/main/interface-allow-auto-connection:true
  • openstack-ps7:ubuntu-core-18-64:tests/main/parallel-install-layout
  • openstack-ps7:ubuntu-core-18-64:tests/main/parallel-install-interfaces-content:common
  • openstack-ps7:ubuntu-core-18-64:tests/main/parallel-install-interfaces-content:snap
  • openstack-ps7:ubuntu-core-18-64:tests/regression/lp-1844496
  • openstack-ps7:ubuntu-core-18-64:tests/core/gadget-update-pc
  • openstack-ps7:ubuntu-core-18-64:tests/main/parallel-install-interfaces-content:data
  • openstack-ps7:ubuntu-core-20-64:tests/core/snapd-maintenance-msg
  • openstack-ps7:ubuntu-core-20-64:tests/main/parallel-install-interfaces-content:snap
  • openstack-ps7:ubuntu-core-20-64:tests/core/mem-cgroup-disabled
  • openstack-ps7:ubuntu-core-20-64:tests/regression/lp-1844496
  • openstack-ps7:ubuntu-core-20-64:tests/main/parallel-install-layout
  • openstack-ps7:ubuntu-core-20-64:tests/main/parallel-install-interfaces-content:data
  • openstack-ps7:ubuntu-core-20-64:tests/main/parallel-install-interfaces-content:common
  • openstack-ps7:ubuntu-core-24-64:tests/core/writablepaths
  • openstack-ps7:ubuntu-core-24-64:tests/main/parallel-install-interfaces-content:common
  • openstack-ps7:ubuntu-core-24-64:tests/main/parallel-install-interfaces-content:snap
  • openstack-ps7:ubuntu-core-24-64:tests/main/parallel-install-layout
  • openstack-ps7:ubuntu-core-24-64:tests/main/parallel-install-interfaces-content:data
  • openstack-ps7:ubuntu-core-24-64:tests/regression/lp-1844496
  • openstack-ps7:ubuntu-core-24-64:tests/main/dbus-activation-session-legacy
  • openstack-ps7:ubuntu-20.04-64:tests/main/interface-allow-auto-connection:true
  • openstack-ps7:ubuntu-20.04-64:tests/regression/lp-1906821
  • openstack-ps7:ubuntu-20.04-64:tests/regression/lp-1844496
  • openstack-ps7:ubuntu-20.04-64:tests/main/parallel-install-interfaces-content:common
  • openstack-ps7:ubuntu-20.04-64:tests/main/parallel-install-layout
  • openstack-ps7:ubuntu-20.04-64:tests/main/interface-allow-auto-connection:false
  • openstack-ps7:ubuntu-20.04-64:tests/main/parallel-install-interfaces-content:snap
  • openstack-ps7:ubuntu-20.04-64:tests/main/parallel-install-interfaces-content:data
  • openstack-ps7:ubuntu-20.04-64:tests/main/mounts-persist-refresh-content-snap
  • openstack-ps7:ubuntu-20.04-64:tests/main/interface-allow-auto-connection:verified
  • openstack-ps7:ubuntu-20.04-64:tests/main/interface-allow-auto-connection:unset
  • openstack-ps7:ubuntu-24.04-64:tests/main/interface-allow-auto-connection:false
  • openstack-ps7:ubuntu-24.04-64:tests/regression/lp-1844496
  • openstack-ps7:ubuntu-24.04-64:tests/main/interface-allow-auto-connection:unset
  • openstack-ps7:ubuntu-24.04-64:tests/main/parallel-install-interfaces-content:snap
  • openstack-ps7:ubuntu-24.04-64:tests/main/parallel-install-layout
  • openstack-ps7:ubuntu-24.04-64:tests/main/mounts-persist-refresh-content-snap
  • openstack-ps7:ubuntu-24.04-64:tests/main/parallel-install-interfaces-content:data
  • openstack-ps7:ubuntu-24.04-64:tests/main/interface-allow-auto-connection:verified
  • openstack-ps7:ubuntu-24.04-64:tests/main/interface-allow-auto-connection:true
  • openstack-ps7:ubuntu-24.04-64:tests/main/parallel-install-interfaces-content:common

Restoring:

  • openstack-ps7:ubuntu-core-18-64:tests/core/gadget-update-pc
  • openstack-ps7:ubuntu-core-18-64:tests/core/
  • openstack-ps7:ubuntu-core-18-64
  • openstack-ps7:ubuntu-core-20-64:tests/core/snapd-maintenance-msg
  • openstack-ps7:ubuntu-core-20-64:tests/core/
  • openstack-ps7:ubuntu-core-20-64
  • openstack-ps7:ubuntu-core-20-64:tests/core/mem-cgroup-disabled
  • openstack-ps7:ubuntu-core-20-64:tests/core/
  • openstack-ps7:ubuntu-core-20-64

github-actions[bot] avatar May 22 '25 10:05 github-actions[bot]