change kpartx separator to '-part' #17

jcharaoui · 2022-04-27T13:24:18Z

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

anarcat

LGTM.

SuperQ · 2022-05-11T20:22:47Z

create

+# in some cases, mkfs will cause the partition tables to remap, for example
+# /dev/mapper/foo-1 will turn into /dev/mapper/foo-part1. Rebuild the map
+# to ensure the filesystem can be detected onward:
+if [ ! -b "$filesystem_dev" ]; then


Minor bash style nit, it's better to use [[ ]] tests in bash.

elsewhere the code doesn't use [[ ]], so it would look odd. besides: why?

There are some surprising gotchas with single brackets.

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

anarcat · 2022-05-12T15:16:44Z

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 · 2022-05-12T15:24:30Z

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 · 2022-05-12T20:56:24Z

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 · 2022-05-16T20:27:29Z

@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 · 2022-05-26T15:26:10Z

create

@@ -61,6 +61,17 @@ else
 fi

 mke2fs -Fjqt $OSP_FILESYSTEM $filesystem_dev
+


testing showed that this won't work because it's after the mk2fs call. it should at least be done before.

anarcat · 2022-05-26T16:28:00Z

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 · 2022-06-01T15:47:25Z

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 · 2022-06-01T15:52:43Z

@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 · 2022-06-02T19:14:43Z

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.

This should fix ganeti#13 by using a kpartx partition path separator of '-part', which aligns with the default seen when multipath devices are probed for partitions.

jcharaoui · 2022-06-16T02:15:44Z

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

saschalucas

Thanks @jcharaoui. LGTM.

saschalucas · 2022-06-19T08:28:18Z

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

anarcat mentioned this pull request Apr 27, 2022

remap the partitions after creating the filesystem #14

Closed

anarcat approved these changes Apr 27, 2022

View reviewed changes

SuperQ reviewed May 11, 2022

View reviewed changes

SuperQ approved these changes May 11, 2022

View reviewed changes

anarcat suggested changes May 26, 2022

View reviewed changes

jcharaoui force-pushed the master branch from 90388ca to 52dd0fd Compare May 30, 2022 21:44

Make kpartx use '-part' separator instead of '-'

920408b

This should fix ganeti#13 by using a kpartx partition path separator of '-part', which aligns with the default seen when multipath devices are probed for partitions.

jcharaoui force-pushed the master branch from 52dd0fd to 920408b Compare June 16, 2022 02:12

jcharaoui changed the title ~~remap the partitions after creating the filesystem~~ change kpartx separator to -part Jun 16, 2022

jcharaoui changed the title ~~change kpartx separator to -part~~ change kpartx separator to '-part' Jun 16, 2022

saschalucas approved these changes Jun 19, 2022

View reviewed changes

saschalucas linked an issue Jun 19, 2022 that may be closed by this pull request

create fails mysteriously after mkfs #13

Closed

saschalucas merged commit e0df6b1 into ganeti:master Jun 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change kpartx separator to '-part' #17

change kpartx separator to '-part' #17

jcharaoui commented Apr 27, 2022

anarcat left a comment

SuperQ May 11, 2022

anarcat May 11, 2022

SuperQ May 11, 2022

anarcat commented May 12, 2022 via email

SuperQ commented May 12, 2022

saschalucas commented May 12, 2022

jcharaoui commented May 16, 2022

anarcat May 26, 2022

anarcat commented May 26, 2022

saschalucas commented Jun 1, 2022

jcharaoui commented Jun 1, 2022

saschalucas commented Jun 2, 2022

jcharaoui commented Jun 16, 2022

saschalucas left a comment

saschalucas commented Jun 19, 2022

		@@ -61,6 +61,17 @@ else
		fi

		mke2fs -Fjqt $OSP_FILESYSTEM $filesystem_dev

change kpartx separator to '-part' #17

change kpartx separator to '-part' #17

Conversation

jcharaoui commented Apr 27, 2022

anarcat left a comment

Choose a reason for hiding this comment

SuperQ May 11, 2022

Choose a reason for hiding this comment

anarcat May 11, 2022

Choose a reason for hiding this comment

SuperQ May 11, 2022

Choose a reason for hiding this comment

anarcat commented May 12, 2022 via email

SuperQ commented May 12, 2022

saschalucas commented May 12, 2022

jcharaoui commented May 16, 2022

anarcat May 26, 2022

Choose a reason for hiding this comment

anarcat commented May 26, 2022

saschalucas commented Jun 1, 2022

jcharaoui commented Jun 1, 2022

saschalucas commented Jun 2, 2022

jcharaoui commented Jun 16, 2022

saschalucas left a comment

Choose a reason for hiding this comment

saschalucas commented Jun 19, 2022