[pve-devel] [PATCH container 0/2] Improve volume deactivation

[Aaron Lauterer]

On 12/2/21 08:40, Fabian Ebner wrote:
> Am 01.12.21 um 17:27 schrieb Aaron Lauterer:
>>
>>
>> On 12/1/21 11:12, Fabian Ebner wrote:
>>> Am 26.11.21 um 11:19 schrieb Aaron Lauterer:
>>>> While working on the reassign feature we (F.Ebner & I) discovered that
>>>> it is possible, mainly with RBD volumes, to get into situations where it
>>>> is not possible to remove that volume as an old orphaned RBD mapping
>>>> still exists.
>>>>
>>>> Mainly when converting a container on RBD storage to a template and when
>>>> adding a new MP to a container that is not running and reassigning that
>>>> MP right away to another container.
>>>>
>>>
>>> I feel like cleaning up such things should be the responsibility of the storage plugin itself. It knows best when a volume gets a new name and what needs to happen if there is still something using the old name around.
>>>
>>> For example, after a full clone, volumes from both containers will be active and then reassigning or converting to template will lead to the issue again. There are likely other places where we don't cleanly deactivate. Of course we could try and hunt them all down ;), but quoting from [0]:
>>>
>>> this is fundamentally how volume activation works in PVE - we activate (and skip the expensive parts if already activated) often, but are very careful about de-activating only where necessary (shared volumes when migrating) or clearly 100% right (error handling before removing a newly allocated volume for example).
>>>
>>> [0]: https://bugzilla.proxmox.com/show_bug.cgi?id=3756#c3
>>
>> Hmm okay yeah, definitely valid regarding the second patch. But the first one would still be valid AFAIU because I don't understand why we activate the volumes when creating a template for containers only, but not for VMs if we don't need to do anything in the volume. So not activating it in the first place would help at least in that case.
>>
> 
> Sorry, I didn't mean to imply that the patches were wrong, just wanted to point out that they don't fully address the issue.

No need to excuse yourself, I came across much meaner than I intended to. :) I'll have to think about how to handle the second patch better to avoid these issues of potentially deactivating a storage when we shouldn't.

>>>
>>>> Aaron Lauterer (2):
>>>>    template_create: remove volume activation
>>>>    apply_pending_mountpoint: deactivate volumes if not running
>>>>
>>>>   src/PVE/LXC.pm        | 2 --
>>>>   src/PVE/LXC/Config.pm | 2 ++
>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>