search

ganeti / instance-debootstrap

Debootstrap instance OS (migrated from http://git.ganeti.org/?p=instance-debootstrap.git;a=summary)
GNU General Public License v2.0
4 stars 13 forks source link

change kpartx separator to '-part' #17

Closed jcharaoui closed 2 years ago

jcharaoui commented 2 years ago

This is a fix for #13 and supersedes the #14 PR.

anarcat commented 2 years ago

On 2022-05-11 13:55:45, Ben Kochie wrote:

There are some surprising gotchas with single brackets.

https://www.delftstack.com/howto/linux/difference-between-single-and-double-square-brackets-in-bash/

Can you outline a specific problem you can see in that page? i don't see much apart from && and || not being supported in plain "test" which is fine by me (you just use the normal shell operators instead).

in general, i think we should strive to avoid bashism, and, in this specific case, we would add one where there wasn't any before. all other uses of test in that file use single brackets, why focus on this one?

SuperQ commented 2 years ago

I'm just saying, if it's a new bit of code, following bash best practices is a good idea. I also think it's fine as-is.

saschalucas commented 2 years ago

Thanks to everyone stressing the point, that there are issues regarding partition mapping. However the reason for seems not clear:

@anarcat states in #13 that he is using blockdev disk template in combination with iSCSI. It is common to use iSCSI with multipath. From multipath.conf(5) skip_kpartx defaults to no. So there's a chance that two different kpartx fight against each other?

Does @SuperQ also use something around possible multipath, or is it just plain/drbd etc. disk template?

jcharaoui commented 2 years ago

@anarcat states in #13 that he is using blockdev disk template in combination with iSCSI. It is common to use iSCSI with multipath. From multipath.conf(5) skip_kpartx defaults to no. So there's a chance that two different kpartx fight against each other?

It's possible that it could work with skip_kpartx=no, I haven't tested this. However this is somewhat besides the point, because some of our machines need those partitions to be exposed by multipath, otherwise our setup will break and these machines will fail to find their blockdev on the next reboot of the nodes.

Creating a new instance should work regardless of skip_kpartx, and it can be done rather easily.

anarcat commented 2 years ago

Thanks to everyone stressing the point, that there are issues regarding partition mapping. However the reason for seems not clear:

@anarcat states in #13 that he is using blockdev disk template in combination with iSCSI. It is common to use iSCSI with multipath. From multipath.conf(5) skip_kpartx defaults to no. So there's a chance that two different kpartx fight against each other?

Does @SuperQ also use something around possible multipath, or is it just plain/drbd etc. disk template?

i think you might have a very good point here. in fact, testing our procedures with this patch showed that it didn't fundamentally solve our problem. furthermore, looking again at #13, i see that I created multiple LUNs per VM, which is not something we do anymore. our new procedure now creates a single LUN, partitions it correctly, then passes those devices individually to the VM, with separate adopt blocks.

It's not ideal, but it works, and I suspect it would work even without this patch. (but that remains to be tested.)

saschalucas commented 2 years ago

Thanks at @jcharaoui and @anarcat. I see now your point, that you somehow rely on multipath and partitions on this LUNs. Is this a nested stack of partition inside an other partition, or just two different mappings of the same partition (one side from multipath, the other from instance-debootstrap)? If the later is the case, I could image to align instance-debootstraps partition prefix with multipath, like in commit 90388ca (ATM overwritten by fore push), if that would solve your problem?

jcharaoui commented 2 years ago

@saschalucas They are different mappings of the same partitions, yes. The commit that I pushed and removed from this PR, the one that adjusts the partition delimiter, is what we ended up using to successfully. I verified that it also works when create DRBD instances as well. If that looks like a proper fix to you, you may close this PR and I'll submit a new one with that fix.

Also FTR I did try to adjust partition_delimiter in multipath.conf but did not find that it improved or even changed anything.

saschalucas commented 2 years ago

If that looks like a proper fix to you, you may close this PR and I'll submit a new one with that fix.

Yes, I'm fine with changing the partition prefix in instance-debootstrap to be aligned with multipath.

I'm not sure about the right etiquette, but instead of closing this PR and creating a new one, we can change the title of this and force-push a different solution (namely commit 90388ca)? As PR creator @jcharaoui should be able to change title/close etc. If not I'll help out. Please decide for yourself which way you want to go.

jcharaoui commented 2 years ago

Alright @saschalucas, I've changed this PR according to what we discussed.

saschalucas commented 2 years ago

Just for reference: This aligns with https://github.com/opensvc/multipath-tools/blob/686797b7d0da4fc24fdfcb0fc6364df78a0b232d/kpartx/kpartx.rules#L42