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

Support snapshot revert operation #3623

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

mulkieran
Copy link
Member

@mulkieran mulkieran commented May 23, 2024

Closes #3621
Closes #3622

@mulkieran mulkieran self-assigned this May 23, 2024
@mulkieran mulkieran force-pushed the issue_stratisd_3621 branch 6 times, most recently from bdc0316 to 61f9b17 Compare July 23, 2024 19:50
Copy link

Cockpit tests failed for commit 017f330. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

Ech, can we please stop breaking rawhide in new an "interesting" ways every censored day? I'm still catching up with the fallout from the firewalld/selinux debacle..

+ chpasswd
chpasswd: (user root) pam_chauthtok() failed, error:
Authentication token manipulation error
chpasswd: (line 1, user root) password not changed

Again, clearly not your fault. I'm still busy with investigating/reporting the other regressions from last night, so if someone else could have a look that'd be nice. However, as #3653 went green, it's worth retrying.

@martinpitt
Copy link
Contributor

The timing coincides with https://bodhi.fedoraproject.org/updates/FEDORA-2024-26a844bb1c . This also happens in podman PRs, e.g. containers/podman#23274 or containers/podman#23275 .

Trivially reproducible in a rawhide VM with updating shadow-utils. With shadow-utils-2:4.15.1-8.fc41.x86_64 (not the latest version from that bodhi update, that's -9):

# echo root:foobar | chpasswd
chpasswd: (user root) pam_chauthtok() failed, error:
Authentication token manipulation error
chpasswd: (line 1, user root) password not changed

That most recent bodhi update reverted the change again:

dnf update https://kojipkgs.fedoraproject.org//packages/shadow-utils/4.15.1/9.fc41/x86_64/shadow-utils-4.15.1-9.fc41.x86_64.rpm

and with that it works. So please retry.

Copy link

Cockpit tests failed for commit e5933ba. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

I hacked around the shadow-utils b0rkage in cockpit-project/cockpit#20802 . Can you please retry the failed tests (or ignore the rawhide failures if you don't care)?

@mulkieran mulkieran force-pushed the issue_stratisd_3621 branch 2 times, most recently from e44d40e to e9c9985 Compare August 13, 2024 17:13
@mulkieran
Copy link
Member Author

rebased. This was just a conflict w/ the filesystem metadata PR, so it was not a lot of work to resolve the conflicts.

@mulkieran mulkieran force-pushed the issue_stratisd_3621 branch 3 times, most recently from 03e19a3 to 12a2d19 Compare August 15, 2024 22:00
Copy link

Cockpit tests failed for commit 7d9664d. @martinpitt, @jelly, @mvollmer please check.

Copy link

Cockpit tests failed for commit 03e19a3. @martinpitt, @jelly, @mvollmer please check.

Copy link

Cockpit tests failed for commit 12a2d19. @martinpitt, @jelly, @mvollmer please check.

@mulkieran mulkieran force-pushed the issue_stratisd_3621 branch 5 times, most recently from f468a09 to 1d5489f Compare August 17, 2024 01:40
@mulkieran
Copy link
Member Author

mulkieran commented Sep 9, 2024

For (2), the only issue is whether, if we do it, we ought to make destroy_filesystems patch up the graph, also.

In both cases, we can have origin0, snap1, and snap1_a, where snap1_a is a snap of snap1.

For the revert case, snap1 overwrites origin1, and the proposal is that snap1_a's origin should be changed to origin0, instead of being set to None, as it is now.

For the destroy case, if snap1 is destroyed, should snap1_a's origin be changed to origin0? It is currently set to None. I don't really have an answer to this right now.

@mulkieran
Copy link
Member Author

(2) is done

@mulkieran
Copy link
Member Author

Another change is implemented. If reverting snap1_a into snap_1, the origin field is set to origin0, not to None as formerly.

@mulkieran
Copy link
Member Author

I think we should make the filesystem_destroy operation patch up the origin as well, to make it congruent with revert behavior. But that's more of a pain, because of the D-Bus.

@bmr-cymru
Copy link
Member

I think we should make the filesystem_destroy operation patch up the origin as well, to make it congruent with revert behavior. But that's more of a pain, because of the D-Bus.

I think that makes sense. I'll try to get these changes tested in my snapshot manager VMs.

@mulkieran mulkieran force-pushed the issue_stratisd_3621 branch 2 times, most recently from 21d5fd9 to e4cba73 Compare September 10, 2024 01:55
@bmr-cymru
Copy link
Member

/packit build

Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/stratis-storage-stratisd-3623
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

1 similar comment
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/stratis-storage-stratisd-3623
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

Signed-off-by: mulhern <amulhern@redhat.com>
Add MergeRequested property to filesystem D-Bus interface.
Use it to set and unset the filesystem metadata field.

Change destroy_filesystems to take into account the merge status of a
filesystem.  Move all the destroy functionality into thinpool implementation.
Do more checks to avoid deleting filesystems that shouldn't be deleted.
Check for situations where multiple snapshots are referring to the same
deleted filesystem.

Check for invalid scheduling requests when requesting or canceling a
merge request.

Add some tests.
Verify that scheduled merges are permissible.

Return with an error without setting up any filesystems if duplicate
UUIDs or names are found.

Perform the merges before setting up any filesystems. If a merge can be
rolled back, set up the two filesystems in the merge relation in the
normal way.

Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
If a snapshot was taken after the one reverted to, remove its origin
field.

If a snapshot was reverted, any snapshot that points at it should have
its origin field removed.

Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Semantics preserving, so pass None where it is invoked.

Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Make this change visible on the D-Bus.

Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
Signed-off-by: mulhern <amulhern@redhat.com>
To avoid a mutable snap and miscellaneous setting and unsetting in the
setup method.

Signed-off-by: mulhern <amulhern@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending
3 participants