public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Shannon Sterz" <s.sterz@proxmox.com>
To: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>,
	"Proxmox Backup Server development discussion"
	<pbs-devel@lists.proxmox.com>,
	"Wolfgang Bumiller" <w.bumiller@proxmox.com>
Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>
Subject: Re: [pbs-devel] [PATCH backup] fix #3336: cleanup when deleting last snapshot
Date: Mon, 10 Mar 2025 16:22:59 +0100	[thread overview]
Message-ID: <D8COTTAPLN3P.1QCNWLZOZN0QH@proxmox.com> (raw)
In-Reply-To: <1305796503.6535.1741619969928@webmail.proxmox.com>

On Mon Mar 10, 2025 at 4:19 PM CET, Fabian Grünbichler wrote:
>
>> Wolfgang Bumiller <w.bumiller@proxmox.com> hat am 10.03.2025 11:53 CET geschrieben:
>>
>>
>> On Fri, Mar 07, 2025 at 04:53:14PM +0100, Shannon Sterz wrote:
>> > 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,
>>
>> I suppose that could work. Depending on how we want to deal with
>> old-version support there? @Thomas?
>
> I don't think that works, unless we want to require putting all datastores
> into maintenance mode prior to the upgrade and until the system has been
> rebooted?
>
> otherwise, the upgrade from 3.x to 4.x is just like any other, with all the
> same problems if the old proxy still has a backup or other lock-holding task
> running and the new one uses different locking..

i feel like it would be fine to do that though? we already optionally
recommended that when upgrading from 2 -> 3 [1]. so requiring that and
documenting it in the upgrade notes sounds fine to me.

[1]: https://pbs.proxmox.com/wiki/index.php/Upgrade_from_2_to_3#Optional:_Enable_Maintenance_Mode

>
>> > 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?
>>
>> For upgrade->downgrade->upgrade switcheroo the postinst script should be
>> able to prevent this, since it gets old and new versions.
>>
>> For anything else I'm putting "accidentally creating that file" in the
>> same category as "accidentally modifying files in their PBS storage" and
>> "accidentally putting their hard drives in the microwave".
>
> agreed on that part :)



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

  reply	other threads:[~2025-03-10 15:23 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
2025-03-10 10:53       ` Wolfgang Bumiller
2025-03-10 15:19         ` Fabian Grünbichler
2025-03-10 15:22           ` Shannon Sterz [this message]
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=D8COTTAPLN3P.1QCNWLZOZN0QH@proxmox.com \
    --to=s.sterz@proxmox.com \
    --cc=f.gruenbichler@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=t.lamprecht@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