From: "Shannon Sterz" <s.sterz@proxmox.com>
To: "Wolfgang Bumiller" <w.bumiller@proxmox.com>
Cc: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH backup] fix #3336: cleanup when deleting last snapshot
Date: Fri, 07 Mar 2025 16:53:14 +0100 [thread overview]
Message-ID: <D8A5LC2K31FM.3F8NB7FLU5B74@proxmox.com> (raw)
In-Reply-To: <6rq4ttz3i7rercdebpty3wpxfdtrcllahcsofr56fu7luydgpt@e6xasbedzhvp>
On Fri Mar 7, 2025 at 4:33 PM CET, Wolfgang Bumiller wrote:
> On Fri, Mar 07, 2025 at 11:37:32AM +0100, Shannon Sterz wrote:
>> On Thu Mar 6, 2025 at 1:08 PM CET, Maximiliano Sandoval wrote:
>> > When the last snapshot from a group is deleted we clear the entire
>> > group, this in turn cleans the owner for the group.
>> >
>> > Without this change, the user is unable to change the owner of the group
>> > after the last snapshot has been deleted. This would prevent a new
>> > backups to the same group from a different owner.
>> >
>> > Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> > ---
>> > src/api2/admin/datastore.rs | 12 +++++++++++-
>> > 1 file changed, 11 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> > index dbb7ae47..305673f1 100644
>> > --- a/src/api2/admin/datastore.rs
>> > +++ b/src/api2/admin/datastore.rs
>> > @@ -423,10 +423,20 @@ pub async fn delete_snapshot(
>> > &backup_dir.group,
>> > )?;
>> >
>> > - let snapshot = datastore.backup_dir(ns, backup_dir)?;
>> > + let snapshot = datastore.backup_dir(ns.clone(), backup_dir)?;
>> >
>> > snapshot.destroy(false)?;
>> >
>> > + let group = BackupGroup::from(snapshot);
>> > + if group.list_backups().is_ok_and(|backups| backups.is_empty()) {
>> > + if let Err(err) = datastore.remove_backup_group(&ns, group.as_ref()) {
>> > + log::error!(
>> > + "error while cleaning group {path:?} - {err}",
>> > + path = group.full_group_path()
>> > + );
>> > + }
>> > + }
>> > +
>>
>> this bug... looks so harmless, but comes back to haunt me every time. as
>> explained by wobu here [1] it is not really possible to cleanly fix this
>> bug without reworking our locking mechanism. i did send some patches for
>> that (checks notes) almost 3 years ago ^^', but they are still not
>> applied and definitively need a rework at this point [2]. i can pick
>> this up again, sorry got focused on other things in the meantime.
>>
>> [1]: https://lore.proxmox.com/pbs-devel/20220314093617.n2mc2jv4k6ntzroo@wobu-vie.proxmox.com/
>> [2]: https://lore.proxmox.com/pbs-devel/20220824124829.392189-1-s.sterz@proxmox.com/
>
> IIRC we need to figure out a good upgrade strategy so running processes
> don't use different locking.
>
> One idea was to have the postinst script create a file in run (eg
> `/run/proxmox-backup/old-locking`) which, when present, instructs the
> daemons to keep using the old strategy.
>
> The new one would then automatically be used after either a reboot, or
> manually removing the file between stop & start of the daemons.
yeah i remember that being a blocker, but since pbs 4 is coming up
soon-ish, couldn't we just apply it then? upgrading between 3 -> 4
requiring a reboot seems reasonable to me, though maybe i'm missing
something (e.g. could it be problematic to have the services running,
even shortly, before the reboot?).
if that would be an option, it'd be much simpler than carrying around
that switching logic forever (or at least one major version?). also,
what would happen if a user accidentally creates that file after the new
locking is already in-place? do we consider this "bad luck" or do we
want some kind of protection in-place?
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
next prev parent reply other threads:[~2025-03-07 15:53 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-06 12:08 Maximiliano Sandoval
2025-03-07 10:37 ` Shannon Sterz
2025-03-07 15:33 ` Wolfgang Bumiller
2025-03-07 15:53 ` Shannon Sterz [this message]
2025-03-10 10:53 ` Wolfgang Bumiller
2025-03-10 15:19 ` Fabian Grünbichler
2025-03-10 15:22 ` Shannon Sterz
2025-03-11 9:21 ` Thomas Lamprecht
2025-03-11 9:52 ` Shannon Sterz
2025-03-11 10:40 ` Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D8A5LC2K31FM.3F8NB7FLU5B74@proxmox.com \
--to=s.sterz@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
--cc=w.bumiller@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal