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

fix: prevent creation of clones after reverting to previous snap revision #275

Merged

Conversation

st3v3nmw
Copy link
Collaborator

@st3v3nmw st3v3nmw commented Oct 3, 2024

Reverting to a previous snap revision leads to duplicate machine entries because the snap's data is stored in $SNAP_DATA and not $SNAP_COMMON. When the data is stored in $SNAP_DATA, the snap starts seeing the old data from the previous revision of the snap. Since the state (message id, etc) doesn't match the one tracked by the server, a re-registration is requested leading to clones.

The full bug report is available on Launchpad.

How to reproduce

  1. Install landscape-client from the stable channel & register:
$ snap install landscape-client
$ snap set landscape-client log-level="debug"
$ snap set landscape-client exchange-interval=300
$ sudo landscape-client.config ...
[...]
Registration request sent successfully.
  1. Wait until the first full message exchange takes place i.e. the computer's info like Distribution, Hardware, etc are populated on Landscape.

  2. Refresh to the distribution from the edge channel.

$ snap refresh landscape-client --edge
  1. Wait until a message exchange takes place i.e. the computer's Last ping time is after the refresh. Then we'll revert back to the previous revision from stable:
$ snap revert landscape-client
landscape-client reverted to 24.08
  1. A clone should now appear on Landscape.

Checking the snap's folders, you'll notice that they've diverged (revision 244 is stable & 299 is edge). We moved from 244 -> 299 -> 244 but 244 still has files that are behind e.g. broker.bpickle, monitor.bpickle.

$ ls -la /var/snap/landscape-client/*/var/lib/landscape/client/
/var/snap/landscape-client/244/var/lib/landscape/client/:
total 88
drwxr-xr-x 7 root root  4096 Oct  7 12:39 .
drwxr-xr-x 3 root root  4096 Oct  7 12:35 ..
drwxr-xr-x 2 root root  4096 Oct  7 12:35 annotations.d
-rw-r--r-- 1 root root  1730 Oct  7 12:38 broker.bpickle
-rw-r--r-- 1 root root  1689 Oct  7 12:38 broker.bpickle.old
drwxr-xr-x 2 root root  4096 Oct  7 12:35 custom-graph-scripts
-rw-r--r-- 1 root root 12288 Oct  7 12:35 manager.database
drwxr-xr-x 3 root root  4096 Oct  7 12:35 messages
-rw-r--r-- 1 root root 17254 Oct  7 12:39 monitor.bpickle
-rw-r--r-- 1 root root 17247 Oct  7 12:37 monitor.bpickle.old
drwxr-xr-x 5 root root  4096 Oct  7 12:35 package
drwxr-x--- 2 root root  4096 Oct  7 12:39 sockets

/var/snap/landscape-client/299/var/lib/landscape/client/:
total 88
drwxr-xr-x 7 root root  4096 Oct  7 12:44 .
drwxr-xr-x 3 root root  4096 Oct  7 12:35 ..
drwxr-xr-x 2 root root  4096 Oct  7 12:35 annotations.d
-rw-r--r-- 1 root root  1651 Oct  7 12:42 broker.bpickle
-rw-r--r-- 1 root root  1651 Oct  7 12:41 broker.bpickle.old
drwxr-xr-x 2 root root  4096 Oct  7 12:35 custom-graph-scripts
-rw-r--r-- 1 root root 12288 Oct  7 12:35 manager.database
drwxr-xr-x 3 root root  4096 Oct  7 12:35 messages
-rw-r--r-- 1 root root 17242 Oct  7 12:44 monitor.bpickle
-rw-r--r-- 1 root root 17235 Oct  7 12:42 monitor.bpickle.old
drwxr-xr-x 5 root root  4096 Oct  7 12:35 package
drwxr-x--- 2 root root  4096 Oct  7 12:44 sockets

/var/snap/landscape-client/current/var/lib/landscape/client/:
total 88
drwxr-xr-x 7 root root  4096 Oct  7 12:39 .
drwxr-xr-x 3 root root  4096 Oct  7 12:35 ..
drwxr-xr-x 2 root root  4096 Oct  7 12:35 annotations.d
-rw-r--r-- 1 root root  1730 Oct  7 12:38 broker.bpickle
-rw-r--r-- 1 root root  1689 Oct  7 12:38 broker.bpickle.old
drwxr-xr-x 2 root root  4096 Oct  7 12:35 custom-graph-scripts
-rw-r--r-- 1 root root 12288 Oct  7 12:35 manager.database
drwxr-xr-x 3 root root  4096 Oct  7 12:35 messages
-rw-r--r-- 1 root root 17254 Oct  7 12:39 monitor.bpickle
-rw-r--r-- 1 root root 17247 Oct  7 12:37 monitor.bpickle.old
drwxr-xr-x 5 root root  4096 Oct  7 12:35 package
drwxr-x--- 2 root root  4096 Oct  7 12:39 sockets

Fix

We'll move the snap's data from the versioned $SNAP_DATA to $SNAP_COMMON which doesn't change across snap revisions. We need to migrate the existing data using a post-refresh hook & remove $SNAP_DATA/var/lib/landscape/client from the current revision.

To check if the migration works:

  1. Remove landscape-client if it's still installed (we need to start on a blank slate) & repeat steps 1 & 2 from the How to reproduce section above. Please register the computer with a different computer title.

  2. Build the snap & install it:

$ snapcraft
Generated snap metadata
Created snap package landscape-client_24.08_amd64.snap
$ snap install landscape-client_24.08_amd64.snap --dangerous
landscape-client 24.08 installed
  1. Checking the new directory structure:
$ ls -la /var/snap/landscape-client/*/var/lib/landscape/client/
/var/snap/landscape-client/244/var/lib/landscape/client/:
total 104
drwxr-xr-x 7 root root  4096 Oct  8 14:11 .
drwxr-xr-x 3 root root  4096 Oct  8 13:55 ..
drwxr-xr-x 2 root root  4096 Oct  8 14:01 annotations.d
-rw-r--r-- 1 root root  1730 Oct  8 14:04 broker.bpickle
-rw-r--r-- 1 root root  1689 Oct  8 14:04 broker.bpickle.old
drwxr-xr-x 2 root root  4096 Oct  8 14:01 custom-graph-scripts
-rw-r--r-- 1 root root     2 Oct  8 14:06 keystone.bpickle
-rw-r--r-- 1 root root 12288 Oct  8 14:01 manager.database
drwxr-xr-x 3 root root  4096 Oct  8 14:01 messages
-rw-r--r-- 1 root root 30199 Oct  8 14:11 monitor.bpickle
-rw-r--r-- 1 root root 17605 Oct  8 14:06 monitor.bpickle.old
drwxr-xr-x 5 root root  4096 Oct  8 14:01 package
drwxr-x--- 2 root root  4096 Oct  8 14:11 sockets

/var/snap/landscape-client/common/var/lib/landscape/client/:
total 200
drwxr-xr-x 7 root root  4096 Oct  8 14:23 .
drwxr-xr-x 3 root root  4096 Oct  8 14:11 ..
-rw-r--r-- 1 root root     0 Oct  8 14:11 .migrated
drwxr-xr-x 2 root root  4096 Oct  8 14:01 annotations.d
-rw-r--r-- 1 root root  1652 Oct  8 14:23 broker.bpickle
-rw-r--r-- 1 root root  1652 Oct  8 14:23 broker.bpickle.old
drwxr-xr-x 2 root root  4096 Oct  8 14:01 custom-graph-scripts
-rw-r--r-- 1 root root     2 Oct  8 14:21 keystone.bpickle
-rw-r--r-- 1 root root     2 Oct  8 14:16 keystone.bpickle.old
-rw-r--r-- 1 root root 12288 Oct  8 14:01 manager.database
drwxr-xr-x 3 root root  4096 Oct  8 14:01 messages
-rw-r--r-- 1 root root 70119 Oct  8 14:22 monitor.bpickle
-rw-r--r-- 1 root root 70119 Oct  8 14:22 monitor.bpickle.old
drwxr-xr-x 5 root root  4096 Oct  8 14:01 package
drwxr-x--- 2 root root  4096 Oct  8 14:11 sockets

Moving forward, the snaps will use $SNAP_COMMON.

@st3v3nmw st3v3nmw force-pushed the fix-dupes-after-snap-revision-changes branch from 92c5b8d to 2acfd6e Compare October 3, 2024 07:48
@st3v3nmw st3v3nmw changed the title fix: prevent creation of clone machines after changing snap revision fix: prevent creation of clones after reverting to previous snap revision Oct 3, 2024
@st3v3nmw st3v3nmw force-pushed the fix-dupes-after-snap-revision-changes branch 7 times, most recently from e6ec3cb to 0b1a2bb Compare October 8, 2024 11:19
@st3v3nmw
Copy link
Collaborator Author

st3v3nmw commented Oct 8, 2024

@Perfect5th,

This is the best fix I could find for this bug. One issue here is that if someone reverts from a snap using $SNAP_COMMON to one using $SNAP_DATA, we have the same problem - the snap will see the old data & a re-registration will occur :(. Moving back & forth between snaps using $SNAP_COMMON should be fine though.

We have two (unappealing?) options:

  1. We can overwrite the $SNAP_DATA (var/snap/landscape-client/<revision>/var/lib/landscape/client/) of all other revisions and symlink it to $SNAP_COMMON/var/lib/landscape/client so that the snaps will always see $SNAP_COMMON. While this could work, it breaks the isolation of snap revisions.
  2. We can use epochs to say that the new snap is incompatible with previous versions (i.e. epoch: 1). However, without a migration, the snaps on epoch 0 would never receive updates :(.

There might be a better fix that I'm missing...

@st3v3nmw st3v3nmw marked this pull request as ready for review October 8, 2024 11:36
@st3v3nmw st3v3nmw force-pushed the fix-dupes-after-snap-revision-changes branch from 0b1a2bb to 91869d2 Compare October 11, 2024 07:17
@st3v3nmw st3v3nmw requested a review from Perfect5th October 11, 2024 07:22
@Perfect5th
Copy link
Contributor

@st3v3nmw unfortunately I think that we're just going to have to accept that older revisions of the snap are going to be broken in this regard - perhaps we could produce documentation on the problem somewhere and how it might be avoided or remedied?

@@ -36,15 +36,14 @@ config_entries = [
("flush-interval", "flush_interval", None),
("exchange-interval", "exchange_interval", None),
("apt-update-interval", "apt_update_interval", None),
("exchange-interval", "exchange_interval", None),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a related/intended change? If so, why is it related?

Copy link
Collaborator Author

@st3v3nmw st3v3nmw Oct 11, 2024

Choose a reason for hiding this comment

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

I noticed it's repeated. It's on line 37 & 39. I removed the one in line 39.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks!

@st3v3nmw
Copy link
Collaborator Author

@Perfect5th, yes, I can talk to Yanisa to document this problem.

Copy link
Contributor

@Perfect5th Perfect5th left a comment

Choose a reason for hiding this comment

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

I think this looks good (and it tests out for me) with the provision that some kind of "FAQ" or "common issues" docs make it up somewhere to inform users why they might see this issue when reverting and what steps they should take to clear clones.

@mcw-work
Copy link
Contributor

Hi @Perfect5th - can we merge please?

@Perfect5th Perfect5th merged commit 081527d into canonical:main Oct 17, 2024
5 checks passed
@Perfect5th
Copy link
Contributor

Merged. I was under the impression that @st3v3nmw had merge rights on this repo, so I left it to him to merge. My mistake.

@st3v3nmw st3v3nmw deleted the fix-dupes-after-snap-revision-changes branch October 18, 2024 06:21
@st3v3nmw
Copy link
Collaborator Author

I was under the impression that @st3v3nmw had merge rights on this repo,

Sorry I do not 😄.

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.

3 participants