OSD prepare job cannot create vols with /dev/mapper alias on PVCs #11301

ausias-armesto · 2022-11-11T17:13:03Z

Description of your changes:
The kubernetes job rook-ceph-osd-prepare-<server-name> is not able to complete correctly when trying to create a OSD whose device is created by a LVM and has an alias starting by /dev/mapper. The job fails trying to create a ceph-volume

Which issue is resolved by this Pull Request:
Resolves #11297

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

Remove the condition that select the devices name prefixed with /dev/mapper as it is preventing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: Ausias Armesto <ausiasarmesto@gmail.com>

Remove the condition that select the devices name prefixed with /dev/mapper as it is preventing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: Ausias Armesto <ausiasarmesto@gmail.com> Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

parth-gr · 2022-11-15T14:42:24Z

You also need to remove this check from the unit test https://github.com/rook/rook/blob/master/pkg/daemon/ceph/osd/volume_test.go
So the unit-test CI should pass

BlaineEXE · 2022-11-15T17:11:33Z

If I understand correctly, this only affects PVC-based OSDs. Is that correct and intended? If so, I would request the title and commit subject be sure to clarify that it is only the PVC OSDs.

BlaineEXE · 2022-11-15T17:12:36Z

I also think @satoru-takeuchi 's input will be invaluable here. As long as this is limited to PVC-based OSDs, I think this is safe, but I'm not sure if there are side-effects I'm not foreseeing.

…ing job prepare execution.\nRemove the condition that select the devices name prefixed with /dev/mapper as it is preventing the ceph-volume command to fail.\nCloses: rook#11301\Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

…ing job prepare execution. Remove the condition that select the devices name prefixed with /dev/mapper as it is preventing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

Remove the condition that select the devices name prefixed with /dev/mapper as it is preventing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

satoru-takeuchi · 2022-11-16T18:22:23Z

@ausias-armesto Does this PR ready to be reviewed?

In addition, please reflect this comment if necessary.

BlaineEXE · 2022-11-16T18:26:37Z

I see this commit body:

Remove the condition that select the devices name prefixed with /dev/mapper as it is preventing the ceph-volume command to fail.

The way I read this, I think preventing is intended to be causing> is that right?

ausias-armesto · 2022-11-16T20:52:26Z

If I understand correctly, this only affects PVC-based OSDs. Is that correct and intended? If so, I would request the title and commit subject be sure to clarify that it is only the PVC OSDs.

Yes, that is the case that I am facing, for PVC based OSD. I've updated the commit message and fixed the unit test so you can review again.
I've also changed 'preventing' by 'causing'

Remove the condition that select the devices name prefixed with /dev/mapper as it is causing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

ausias-armesto · 2022-11-18T09:08:59Z

Hi @satoru-takeuchi @BlaineEXE ,
Is there anything else that I can do to get approved this PR

satoru-takeuchi · 2022-11-18T12:17:49Z

@ausias-armesto I've not reviewed this yet. Please wait for a while. I'll look into it in the next week.

satoru-takeuchi · 2022-11-22T08:42:15Z

@ausias-armesto Please wait for several days to complete the review. The OSD creation code is complicated very much and I'm afraid regressions.

satoru-takeuchi · 2022-11-24T00:45:53Z

I assume the root cause is not the treatment of "/dev/mapper" but the improper handling of osdsPerDevice field. Although OSD on LV doesn't support osdsPerDevice field (it's not documented yet and I'll fix it later), users can specify this field and Rook tries to osd by mistake.

I guess you can create an OSD without applying this PR with the following configuration which you mentioned. Could you confirm this?

          storage:
            useAllNodes: false
            useAllDevices: false
            # deviceFilter:
            nodes:
              - name: "server-stage-fsn1-002"
                devices:
                  - name: "/dev/vg0/data"
                    config:
                      osdsPerDevice: "1"
                      deviceClas: "hdd"

And the following configuration results in the problem that you saw. The difference is the number of osdsPerDevice.

          storage:
            useAllNodes: false
            useAllDevices: false
            # deviceFilter:
            nodes:
              - name: "server-stage-fsn1-002"
                devices:
                  - name: "/dev/vg0/data"
                    config:
                      osdsPerDevice: "3"
                      deviceClas: "hdd"

This can be verified by reading your log of the prepare pod.

2022-11-10 22:04:17.199022 D | cephosd: won't use raw mode since osd per device is 3

I guess that your problem will disappear if osdsPerDevice is not specified or is set to 1. Please try this.

travisn · 2022-12-05T23:36:47Z

@satoru-takeuchi I'm investigating a case where an OSD cannot be created on a PVC due to the same issue where the device has mappings under /dev/mappings. But when using a PVC I don't believe we need to use those mappings since the PVC gives us the exact path. Shouldn't we remove the dev mappings handling code for OSDs on PVCs? Or why would we need it?

satoru-takeuchi · 2022-12-06T02:19:35Z

@travisn

Shouldn't we remove the dev mappings handling code for OSDs on PVCs? Or why would we need it?

You're right. This logic is not necessary. So this patch makes sense not as a bug fix but as a cleanup. How about merging this PR anyway? In addition, I'll submit a PR to handle osdsPerDevice properly.

travisn

Marking changes requested until discussion is finalized.

travisn · 2022-12-06T18:56:59Z

pkg/daemon/ceph/osd/volume.go

@@ -586,12 +579,6 @@ func (a *OsdAgent) initializeDevicesLVMMode(context *clusterd.Context, devices *

 			logger.Infof("configuring new LVM device %s", name)
 			deviceArg := path.Join("/dev", name)
-			// ceph-volume prefers to use /dev/mapper/<name> if the device has this kind of alias
-			for _, devlink := range device.PersistentDevicePaths {
-				if strings.HasPrefix(devlink, "/dev/mapper") {


@travisn

Shouldn't we remove the dev mappings handling code for OSDs on PVCs? Or why would we need it?

You're right. This logic is not necessary. So this patch makes sense not as a bug fix but as a cleanup. How about merging this PR anyway? In addition, I'll submit a PR to handle osdsPerDevice properly.

I wonder if there is still a scenario where we need to keep the /dev/mapper detection. If the user has specified useAllDevices or similar settings, the /dev/mapper path is still preferred. But in the case where the user has specified explicit paths such as with local PVs or named devices in the cluster CR, there is no need for this search since they can be specified already as such with the /dev/mapper path. If we add this check back in for the non-pvc case here, we should add it for raw mode in the initializeDevicesRawMode() method where it is missing now. Or perhaps we keep it simple, consistent, and predictable by merging this PR without further changes.
@satoru-takeuchi @ausias-armesto thoughts?

@travisn I only focused on LV-backed PVC by mistake and now I have a concern.

This logic is not necessary for LV-backed OSD for both OSD on device
nd OSD on PVC because this configuration always uses raw mode and ceph-volume raw prepare accepts all device names. However, if a new OSD creation uses lvm mode, /dev/mapper prefix must be used because ceph-volume lvm prepare only accept the device names prefixed by /dev/mapper. This limitation exists for both OSD on device and OSD on PCV.

Please wait for a while. I'll verify my concern later.

@travisn , I can only test on LV-backed PVC, and cannot provide further information about how would behave in other contexts. On the other side, changing osdsPerDevice to 1 has not changed anything, is still not working.

Thanks for following this issue

@ausias-armesto Could you show me your cluster.yaml and the log of osd prepare pod when you specified osdsPerDevice: 1?

osd-prepare-1.log
osd-prepare-2.log
osd-prepare-3.log
helm-cluster.yaml.txt

The disk partitions for servers running on osd2 and osd3 looks like this:

root@server-stage-fsn1-002 /home/debian # lsblk -f NAME FSTYPE FSVER LABEL UUID FSAVAIL FSUSE% MOUNTPOINT sda ├─sda1 linux_raid_member 1.2 rescue:0 492095b8-f9ee-e7c0-9e1e-e19193d3c00f │ └─md0 ext3 1.0 ce3f530e-9eb8-4d67-ac74-44fe208f9328 877.3M 6% /boot └─sda2 linux_raid_member 1.2 rescue:1 d9d405cd-528f-6f96-99b3-9b7fb7469523 └─md1 LVM2_member LVM2 001 ECx2sJ-9et7-Sg4F-oC59-nge2-LLDQ-3WACPR ├─vg0-root ext4 1.0 0545917f-d52f-4902-81f6-88c5f02e84ba 37.9G 17% / ├─vg0-swap swap 1 236c4901-9f82-4399-a8d6-b61147818169 ├─vg0-tmp reiserfs 3.6 9b4771e7-73a5-41f9-ab4f-56972f9be3e1 4.6G 8% /tmp └─vg0-data sdb ├─sdb1 linux_raid_member 1.2 rescue:0 492095b8-f9ee-e7c0-9e1e-e19193d3c00f │ └─md0 ext3 1.0 ce3f530e-9eb8-4d67-ac74-44fe208f9328 877.3M 6% /boot └─sdb2 linux_raid_member 1.2 rescue:1 d9d405cd-528f-6f96-99b3-9b7fb7469523 └─md1 LVM2_member LVM2 001 ECx2sJ-9et7-Sg4F-oC59-nge2-LLDQ-3WACPR ├─vg0-root ext4 1.0 0545917f-d52f-4902-81f6-88c5f02e84ba 37.9G 17% / ├─vg0-swap swap 1 236c4901-9f82-4399-a8d6-b61147818169 ├─vg0-tmp reiserfs 3.6 9b4771e7-73a5-41f9-ab4f-56972f9be3e1 4.6G 8% /tmp └─vg0-data

The server running on osd1, has 2 extra disks sdc and sdd which are correctly detected.

@travisn @ausias-armesto

This logic is not necessary for LV-backed OSD for both OSD on device and OSD on PVC because this configuration always uses raw mode and ceph-volume raw prepare accepts all device names. However, if a new OSD creation uses lvm mode, /dev/mapper prefix must be used because ceph-volume lvm prepare only accept the device names prefixed by /dev/mapper. This limitation exists for both OSD on device and OSD on PCV.

Please wait for a while. I'll verify my concern later.

My worry proved groundless because of the following reasons.

ceph-volume lvm batch accepts only "disk", "part", "lvm", and "loop". In other words, lvm mode is used only for these disk types.

Rook supports only raw mode for OSD on LV. So "/dev/mapper" prefix (will be removed by this PR) is not necessary.

loop devices are not provided by device mapper. So "/dev/mapper" prefix is not necessary too.

So we can merge this PR if the current conflict is fixed.

@ausias-armesto Could you specify LV devices explicitly by devices.name field? Rook doesn't pick up LV devices specified by filters like deviceFilter by design. As a side note, deviceClas should be deviceClass.

nodes: ... - name: "server-stage-fsn1-002" devices: - name: "/dev/vg0/data" config: osdsPerDevice: "1" deviceClass: "hdd" - name: "server-stage-hel1-001" devices: - name: "/dev/vg0/data" config: osdsPerDevice: "1" deviceClas: "hdd"

travisn · 2022-12-12T17:16:12Z

@ausias-armesto Can you rebase to remove the merge commit from the PR? A single commit is preferred. See these instructions.

Remove the condition that select the devices name prefixed with /dev/mapper as it is causing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

mergify · 2022-12-14T08:08:47Z

This pull request has merge conflicts that must be resolved before it can be merged. @ausias-armesto please rebase it. https://rook.io/docs/rook/latest/Contributing/development-flow/#updating-your-fork

Remove the condition that select the devices name prefixed with /dev/mapper as it is causing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

satoru-takeuchi · 2022-12-14T12:27:17Z

@ausias-armesto Commitlint claims "body must have leading blank line [body-leading-blank]", please check it.

https://github.com/rook/rook/actions/runs/3693055432/jobs/6252601418#step:4:20

Other test failures seem to be intermittent.

travisn · 2022-12-14T18:24:55Z

The CI was having issues earlier today. If you rebase when you update the commit message, the CI should pass now as well.

Remove the condition that select the devices name prefixed with /dev/mapper as it is causing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

It removes the condition that select the devices name prefixed with /dev/mapper as it is causing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

It removes the condition that select the devices name prefixed with /dev/mapper as it is causing the ceph-volume command to fail. Closes: #11301 Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com> (cherry picked from commit f55c457)

satoru-takeuchi · 2022-12-15T23:51:56Z

Although OSD on LV doesn't support osdsPerDevice field (it's not documented yet and I'll fix it later), users can specify this field and Rook tries to osd by mistake.

Thi problem will be handled by #11438

OSD prepare job cannot create vols with /dev/mapper alias on PVCs (backport #11301)

travisn requested a review from satoru-takeuchi November 11, 2022 19:58

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from dda64c7 to 989671e Compare November 12, 2022 09:22

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 989671e to 574240b Compare November 12, 2022 09:28

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 574240b to 3beb9d3 Compare November 12, 2022 09:30

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 3beb9d3 to 9b39405 Compare November 12, 2022 09:34

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 89eb49e to 33cd542 Compare November 16, 2022 14:41

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 33cd542 to 50407bc Compare November 16, 2022 15:12

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 50407bc to b12c16f Compare November 16, 2022 15:26

BlaineEXE changed the title ~~OSD prepare job cannot create volumes with alias /dev/mapper~~ OSD prepare job cannot create /dev/mapper volumes on PVCs Nov 16, 2022

BlaineEXE changed the title ~~OSD prepare job cannot create /dev/mapper volumes on PVCs~~ OSD prepare job cannot create vols with /dev/mapper alias on PVCs Nov 16, 2022

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from b12c16f to c2947ea Compare November 16, 2022 20:55

satoru-takeuchi approved these changes Dec 6, 2022

View reviewed changes

travisn requested changes Dec 6, 2022

View reviewed changes

satoru-takeuchi self-requested a review December 7, 2022 12:39

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 9e5f032 to 15049c4 Compare December 14, 2022 08:08

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 15049c4 to bc41656 Compare December 14, 2022 08:29

satoru-takeuchi mentioned this pull request Dec 14, 2022

shouldn't fall back to creating lvm mode if only raw mode is supported. #11438

Closed

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from bc41656 to 34c9513 Compare December 15, 2022 14:59

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 34c9513 to 42731d6 Compare December 15, 2022 15:03

osd: cannot create volumes on pvc based osd with alias dev mapper

f55c457

It removes the condition that select the devices name prefixed with /dev/mapper as it is causing the ceph-volume command to fail. Closes: rook#11301 Signed-off-by: ausias-armesto <ausiasarmesto@gmail.com>

ausias-armesto force-pushed the fix/11297-create-volumes-with-mapper-alias branch from 42731d6 to f55c457 Compare December 15, 2022 15:05

travisn approved these changes Dec 15, 2022

View reviewed changes

travisn added the backport-release-1.10 label Dec 15, 2022

travisn merged commit 03bfd5e into rook:master Dec 15, 2022

mergify bot mentioned this pull request Dec 15, 2022

OSD prepare job cannot create vols with /dev/mapper alias on PVCs (backport #11301) #11445

Merged

mergify bot added a commit that referenced this pull request Dec 16, 2022

Merge pull request #11445 from rook/mergify/bp/release-1.10/pr-11301

553379f

OSD prepare job cannot create vols with /dev/mapper alias on PVCs (backport #11301)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSD prepare job cannot create vols with /dev/mapper alias on PVCs #11301

OSD prepare job cannot create vols with /dev/mapper alias on PVCs #11301

ausias-armesto commented Nov 11, 2022 •

edited

parth-gr commented Nov 15, 2022

BlaineEXE commented Nov 15, 2022

BlaineEXE commented Nov 15, 2022

satoru-takeuchi commented Nov 16, 2022

BlaineEXE commented Nov 16, 2022

ausias-armesto commented Nov 16, 2022 •

edited

ausias-armesto commented Nov 18, 2022

satoru-takeuchi commented Nov 18, 2022

satoru-takeuchi commented Nov 22, 2022

satoru-takeuchi commented Nov 24, 2022

travisn commented Dec 5, 2022

satoru-takeuchi commented Dec 6, 2022

travisn left a comment

travisn Dec 6, 2022

satoru-takeuchi Dec 6, 2022

ausias-armesto Dec 7, 2022

satoru-takeuchi Dec 7, 2022

ausias-armesto Dec 9, 2022

satoru-takeuchi Dec 14, 2022

satoru-takeuchi Dec 14, 2022

travisn commented Dec 12, 2022

mergify bot commented Dec 14, 2022

satoru-takeuchi commented Dec 14, 2022

travisn commented Dec 14, 2022

satoru-takeuchi commented Dec 15, 2022

OSD prepare job cannot create vols with /dev/mapper alias on PVCs #11301

OSD prepare job cannot create vols with /dev/mapper alias on PVCs #11301

Conversation

ausias-armesto commented Nov 11, 2022 • edited

parth-gr commented Nov 15, 2022

BlaineEXE commented Nov 15, 2022

BlaineEXE commented Nov 15, 2022

satoru-takeuchi commented Nov 16, 2022

BlaineEXE commented Nov 16, 2022

ausias-armesto commented Nov 16, 2022 • edited

ausias-armesto commented Nov 18, 2022

satoru-takeuchi commented Nov 18, 2022

satoru-takeuchi commented Nov 22, 2022

satoru-takeuchi commented Nov 24, 2022

travisn commented Dec 5, 2022

satoru-takeuchi commented Dec 6, 2022

travisn left a comment

Choose a reason for hiding this comment

travisn Dec 6, 2022

Choose a reason for hiding this comment

satoru-takeuchi Dec 6, 2022

Choose a reason for hiding this comment

ausias-armesto Dec 7, 2022

Choose a reason for hiding this comment

satoru-takeuchi Dec 7, 2022

Choose a reason for hiding this comment

ausias-armesto Dec 9, 2022

Choose a reason for hiding this comment

satoru-takeuchi Dec 14, 2022

Choose a reason for hiding this comment

satoru-takeuchi Dec 14, 2022

Choose a reason for hiding this comment

travisn commented Dec 12, 2022

mergify bot commented Dec 14, 2022

satoru-takeuchi commented Dec 14, 2022

travisn commented Dec 14, 2022

satoru-takeuchi commented Dec 15, 2022

ausias-armesto commented Nov 11, 2022 •

edited

ausias-armesto commented Nov 16, 2022 •

edited