Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-09 Thread DERUMIER, Alexandre via pve-devel
--- Begin Message --- > +sub get_snap_name { >>should this be public? I'll make it private > +sub get_snap_volname { >>should this be public? > + > +sub parse_snapname { >>should this be public? This two methods are used in volume_snapshot_info(), defined in plugin, and use by lvmplugin too

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread Fabian Grünbichler
> DERUMIER, Alexandre hat am 08.07.2025 > 15:42 CEST geschrieben: > > > >>okay, that means we instead need to become more strict with 'snapext' > >>storages and restrict the volnames there.. maybe to (vm-|base-)-XXX- > >>*.fmt? > > $plugin->parse_volname($volname) don't have $scfg param cu

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread DERUMIER, Alexandre via pve-devel
--- Begin Message --- >>okay, that means we instead need to become more strict with 'snapext' >>storages and restrict the volnames there.. maybe to (vm-|base-)-XXX- >>*.fmt? $plugin->parse_volname($volname) don't have $scfg param currently, Do you want to extend it ? (and do change in every plug

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread DERUMIER, Alexandre via pve-devel
--- Begin Message --- >>Can we please use an actually telling name though? As "ext" is quite >>often used as term for "extension", and we really win nothing with >>doing this. >> >>Strongly preferring words to be spelled out in full and separated >>with >>hyphens, instead of something else or being

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread Thomas Lamprecht
Am 08.07.25 um 13:35 schrieb DERUMIER, Alexandre: >>> okay, that means we instead need to become more strict with 'snapext' >>> storages and restrict the volnames there.. maybe to (vm-|base-)-XXX- >>> *.fmt? > >>> that means only allowing such names when allocating volumes, and >>> filtering >>> w

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread DERUMIER, Alexandre via pve-devel
--- Begin Message --- > > I think it'll break parsing of already configured storage without > snapext option ? >>I don't think it does? ah , I have tried with { fixed => 1 } only but it's ok with { optional => 1, fixed => 1 } pvesm set teststorage --snapext 1 update storage failed: can't cha

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread DERUMIER, Alexandre via pve-devel
--- Begin Message --- >>okay, that means we instead need to become more strict with 'snapext' >>storages and restrict the volnames there.. maybe to (vm-|base-)-XXX- >>*.fmt? >>that means only allowing such names when allocating volumes, and >>filtering >>when listing images.. >> >>since we want to

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread Fabian Grünbichler
> DERUMIER, Alexandre hat am 08.07.2025 > 12:04 CEST geschrieben: > > > > +my sub alloc_backed_image { > > +    my ($class, $storeid, $scfg, $volname, $backing_snap) = @_; > > + > > +    my $path = $class->path($scfg, $volname, $storeid); > > +    my $backing_path = $class->path($scfg, $volna

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread DERUMIER, Alexandre via pve-devel
--- Begin Message --- > +my sub alloc_backed_image { > +    my ($class, $storeid, $scfg, $volname, $backing_snap) = @_; > + > +    my $path = $class->path($scfg, $volname, $storeid); > +    my $backing_path = $class->path($scfg, $volname, $storeid, > $backing_snap); >>should we use a relative path

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread Fabian Grünbichler
> DERUMIER, Alexandre hat am 08.07.2025 > 10:44 CEST geschrieben: > > > > preallocation => { optional => 1 }, > > +    snapext => { optional => 1 }, > > >>needs to be "fixed", as the code doesn't handle mixing internal > >>and external snapshots on a single storage.. > > I thin

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-08 Thread DERUMIER, Alexandre via pve-devel
--- Begin Message --- > preallocation => { optional => 1 }, > +    snapext => { optional => 1 }, >>needs to be "fixed", as the code doesn't handle mixing internal >>and external snapshots on a single storage.. I think it'll break parsing of already configured storage without snapext

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-07 Thread Fabian Grünbichler
> DERUMIER, Alexandre hat am 07.07.2025 > 12:18 CEST geschrieben: > >>for the DirPlugin we need to add another level for the snapshots > >>on-disk, else we cannot differentiate between weirdly-named image > >>files and snapshot files.. > >> > >>so we could add a directory "snapshots" and put a

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-07 Thread DERUMIER, Alexandre via pve-devel
--- Begin Message --- > > or maybe use something else than volume_snapshot_info here, simply > glob > all the vm disk && snap files and delete them in random order, as we > want to delete it anyway. yes, this is exactly what I meant with tricky ;) >>if we start deleting snapshots from the "first

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-07 Thread Fabian Grünbichler
> DERUMIER, Alexandre hat am 04.07.2025 > 15:22 CEST geschrieben: > > +    #delete external snapshots > > +    if ($scfg->{snapext}) { > > +    my $snapshots = $class->volume_snapshot_info($scfg, > > $storeid, $volname); > > +    for my $snapid ( > > +    sor

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-04 Thread DERUMIER, Alexandre via pve-devel
--- Begin Message --- > +    snapext => { optional => 1 }, >>needs to be "fixed", as the code doesn't handle mixing internal >>and external snapshots on a single storage.. indeed, I'll fix it > >   > +my sub alloc_backed_image { > +    my ($class, $storeid, $scfg, $volname, $backing_snap) =

Re: [pve-devel] [PATCH pve-storage 08/10] qcow2: add external snapshot support

2025-07-04 Thread Fabian Grünbichler
> Alexandre Derumier via pve-devel hat am > 04.07.2025 08:45 CEST geschrieben: > add a snapext option to enable the feature > > When a snapshot is taken, the current volume is renamed to snap volname > and a current image is created with the snap volume as backing file > > Signed-off-by: Alex