Avoid usage of umount in ISOSR when legacy_mode is used

[Wescoeur]

umount should not be called when legacy_mode is enabled, otherwise a mounted dir
used during SR creation is unmounted at the end of the create call (and also
when a PBD is unplugged) in detach block.

Signed-off-by: Ronan Abhamon ronan.abhamon@vates.fr

[MarkSymsCtx]

I'd be very surprised if legacy_mode got used at all on current systems, I'd much rather just remove all the legacy_mode handling code and clean up the technical debt.

[Wescoeur]

Well, legacy_mode is used to create local SRs. :) We have several users who use this mode.

[MarkSymsCtx]

Well, it's technical debt as far as Citrix is concerned and a candidate for removal, we just haven't got around to do so.

[stormi]

I'll try to be the voice of users somehow. I can see how it's technical debt from the technical side, but local ISO SRs are a valid use case (ideally, on a separate mounted partition as in the case that revealed the issue discussed here), so I would be concerned if the code were removed without an alternate solution offered to users. Would both code cleaning and a new feature - a separate local ISO SR driver? A new driver in smapiv3? - be done at the same time or is it the official XAPI project position that ISO SRs on local disks are not a valid use case?

[stormi]

You can see it still used in this 2018 article: https://linuxconfig.org/how-to-add-iso-image-storage-repository-on-xenserver-7-linux

Or in the XS forum: https://discussions.citrix.com/topic/395569-how-to-add-local-iso-repository-to-xenserver-72/

An internet search for local ISO SRs on Citrix hypervisor also gives this old KB that I saw users still refer to https://support.citrix.com/article/CTX121671

Interaction with XCP-ng users also regularly reveals that in small setups (single hosts, two hosts...) a local ISO SR is needed to avoid depending on a SMB or NFS ISO SR to be able to create any VM.

[MarkSymsCtx]

An internet search for local ISO SRs on Citrix hypervisor also gives this old KB that I saw users still refer to https://support.citrix.com/article/CTX121671

yes, for XenServer 3.2, a release, out of support for over a decade.

This code is untested and unmaintained and therefore at risk of being broken by changes that do not anticipate the needs for it.

[stormi]

I understand that. My concern remains that, to my knowledge, a fair amount of users - granted, probably users small enough that they are not important to Citrix - do depend on it already. The use case was important enough at the time of XS 5.0 that a KB was created, users relied on it, and I never saw an announcement saying that it is not valid anymore (there may be one I haven't seen). That's why I'm wondering about what the feature will become.

This legacy_mode thing is clearly something that should probably go, but what about a replacement for it such as a dedicated LocalISOSR driver, that would accept either a location on the filesystem, or a device?

[MarkSymsCtx]

Doing this "properly" would be relatively straightforward as a small subclass of the ISOSR class that just defined empty, no-op implementations for attach and detach and deferred everything else to the base class. This could even live as a standalone implementation and not necessarily be part of this code repository.

[stormi]

Thanks for the pointers. We will choose what to do if/when the legacy code gets removed. I still suspect you would get a few surprised Citrix Hypervisor users though.

We already have a fork of sm that has commits that either were not accepted in your repository or that we think would not be accepted (we'd gladly contribute them if there's interest in them, but I understand smapi v1 is old and is not where new features or drivers are added). Our policy remains upstream first as we believe that cooperation is better for everyone, us included, and we had quite a few PRs merged in the XAPI project already (one was just a few hours ago 🎉). So were we to create a new local ISO SR driver, we'd probably try to contribute it, with its unit tests. Either to smapi v1 or to smapi v3 if there's a way to do it. And resort to having it only on our branch if rejected.

Now, about this very PR, wouldn't it be possible to accept it? It does fix a bug, should not break any existing tests, and makes the feature tested, a feature that is maybe not officially supported (you might want to have the KB updated and/or include it in the list of deprecated features in the next CH release) but is used nonetheless. And this does not prevent you from removing the whole legacy_mode stuff sometime in the future.