osd: support create osd with metadata partition #13314

microyahoo · 2023-12-04T08:23:17Z

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

Currently, when rook provisions OSDs(in the OSD prepare job), rook effectively run a c-v command such as the following.

ceph-volume lvm batch --prepare <deviceA> <deviceB> <deviceC> --db-devices <metadataDevice>

but c-v lvm batch only supports disk and lvm, instead of disk partitions. We can resort to ceph-volume lvm prepare to implement it.

Signed-off-by: Liang Zheng zhengliang0901@gmail.com

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
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.

satoru-takeuchi · 2023-12-05T21:00:23Z

@travisn By this change, the will be difference in the supported configuration depends on whether metadataDevice is shared by multiple OSDs or not (ref). IMO, it's acceptable and updating the supported configuration is enough. Could you tell me your opinion?

travisn

@travisn By this change, the will be difference in the supported configuration depends on whether metadataDevice is shared by multiple OSDs or not (ref). IMO, it's acceptable and updating the supported configuration is enough. Could you tell me your opinion?

This PR doesn't have a description, so I'm trying to understand from the linked issue... So if the metadataDevice is a partition, we must use lvm prepare instead of lvm batch? Then this change seems reasonable.

I would just ask the following for maintainability:

Tests are needed in the CI
More comments in the code changes and in the commit description to describe the change.

satoru-takeuchi · 2023-12-06T00:45:36Z

@travisn

This PR doesn't have a description, so I'm trying to understand from the linked issue...

Oh, I should have explained this PR in detail.

So if the metadataDevice is a partition, we must use lvm prepare instead of lvm batch?

Correct.

I would just ask the following for maintainability:

Tests are needed in the CI
More comments in the code changes and in the commit description to describe the change.

Yes, I agree with you. I'm going to comment it later.

@microyahoo Please wait for a while to review this. I don't have enough time in this week.

microyahoo · 2023-12-06T02:30:08Z

@travisn By this change, the will be difference in the supported configuration depends on whether metadataDevice is shared by multiple OSDs or not (ref). IMO, it's acceptable and updating the supported configuration is enough. Could you tell me your opinion?

This PR doesn't have a description, so I'm trying to understand from the linked issue... So if the metadataDevice is a partition, we must use lvm prepare instead of lvm batch? Then this change seems reasonable.

I will add more detailed description for PR.

I would just ask the following for maintainability:

Tests are needed in the CI

If this solution works well I will add unit tests and integration tests.

More comments in the code changes and in the commit description to describe the change.

OK.

travisn · 2023-12-15T18:32:23Z

@microyahoo Thanks for all the unit tests. I will wait for @satoru-takeuchi to review it and suggest if integration tests are needed.

microyahoo · 2023-12-16T03:56:38Z

@microyahoo Thanks for all the unit tests. I will wait for @satoru-takeuchi to review it and suggest if integration tests are needed.

yes, integration tests are also needed, I will add it ASAP. Do the integration tests need to be merged together with this PR, or can another PR be merged separately?

satoru-takeuchi · 2023-12-16T07:24:30Z

Do the integration tests need to be merged together with this PR,

Yes, please add an integration test.

microyahoo · 2023-12-16T13:56:34Z

Do the integration tests need to be merged together with this PR,

Yes, please add an integration test.

👌

microyahoo · 2023-12-18T03:28:20Z

Hi @satoru-takeuchi, @travisn I noticed github ci runner has only one 64G disks except for root disk, my test cases is similar to osd-with-metadata-device, but I have to create a partition from another disk as a metadata device first. How to add one more disk in ci runner? Thanks.

https://github.com/rook/rook/actions/runs/7220671504/job/19674086946?pr=13314

+ sudo lsblk
NAME    MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
sda       8:0    0   64G  0 disk 
└─sda1    8:1    0   64G  0 part 
sdb       8:16   0   86G  0 disk 
├─sdb1    8:17   0 85.9G  0 part /
├─sdb14   8:30   0    4M  0 part 
└─sdb15   8:31   0  106M  0 part /boot/efi

satoru-takeuchi · 2023-12-19T09:00:36Z

@microyahoo I'll answer tomorrow or the day after tomorrorw. Please wait for a while.

satoru-takeuchi · 2023-12-21T08:27:29Z

@microyahoo You can create OSD on top of loop device if you set ROOK_CEPH_ALLOW_LOOP_DEVICES: "true" in operator.yaml. Could you try this feature?

microyahoo · 2023-12-21T11:09:27Z

@microyahoo You can create OSD on top of loop device if you set ROOK_CEPH_ALLOW_LOOP_DEVICES: "true" in operator.yaml. Could you try this feature?

yes, we can use loop device to create osd, but it will lvm, not partition type, not sure if I understand correctly. I will try it first, thanks.

satoru-takeuchi · 2023-12-21T23:41:28Z

I meant using loo device for osd and use a partition in the 64 GiB disk as a metadata device. IIRC, a partition will be created if removing the --wipe-only You can do it by changing

tests/scripts/create-bluestore-partitions.sh --disk "$BLOCK" --wipe-only

to

tests/scripts/create-bluestore-partitions.sh --disk "$BLOCK".

satoru-takeuchi · 2024-01-12T06:51:51Z

The review in progress. I'll send a feedback next week.

Documentation/CRDs/Cluster/ceph-cluster-crd.md

satoru-takeuchi

I left some comments about integration tests and documents but the review is still under progress. I'll make some other comments about go code later.

.github/workflows/canary-integration-test.yml

tests/scripts/github-action-helper.sh

Documentation/CRDs/Cluster/ceph-cluster-crd.md

.github/workflows/canary-integration-test.yml

tests/scripts/github-action-helper.sh

satoru-takeuchi · 2024-02-01T07:39:34Z

I'm reviewing and testing this PR. Please wait for a while. Probably I'll finish the review tomorrow.

Documentation/CRDs/Cluster/ceph-cluster-crd.md

pkg/daemon/ceph/osd/volume.go

pkg/daemon/ceph/osd/volume_test.go

pkg/daemon/ceph/osd/volume.go

microyahoo · 2024-02-02T08:01:16Z

test cases failed due to BLOCK not found.

++ sudo lsblk --paths
++ awk '/14G/ || /64G/ {print $1}'
++ head -1
+ : ''

subhamkrai · 2024-02-02T09:22:20Z

test cases failed due to BLOCK not found.
++ sudo lsblk --paths
++ awk '/14G/ || /64G/ {print $1}'
++ head -1
+ : ''

probably #13675 will fix

mergify · 2024-02-07T01:23:28Z

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

Currently, when rook provisions OSDs(in the OSD prepare job), rook effectively run a c-v command such as the following. ```console ceph-volume lvm batch --prepare <deviceA> <deviceB> <deviceC> --db-devices <metadataDevice> ``` but c-v lvm batch only supports disk and lvm, instead of disk partitions. We can resort to `ceph-volume lvm prepare` to implement it. Signed-off-by: Liang Zheng <zhengliang0901@gmail.com>

microyahoo · 2024-02-07T07:28:48Z

Hi @satoru-takeuchi, all test cases passed, PTAL, thanks.

satoru-takeuchi · 2024-02-08T00:31:00Z

@travisn I approved this PR. Could you also review this? It's because you left change request to add tests and improve documentation. In my feeling, it doesn't break the existing cluster's behavior and is well tested. Doducments are well updated.

travisn

Looks good with all the testing, thanks.

osd: support create osd with metadata partition (backport #13314)

microyahoo requested a review from satoru-takeuchi December 4, 2023 08:27

microyahoo force-pushed the partition branch 2 times, most recently from a98e74f to 95a99d6 Compare December 4, 2023 08:38

microyahoo changed the title ~~[WIP] feature: support create osd with metadata partition~~ [WIP] osd: support create osd with metadata partition Dec 4, 2023

microyahoo requested a review from travisn December 4, 2023 14:57

satoru-takeuchi marked this pull request as draft December 5, 2023 20:54

satoru-takeuchi added the do-not-merge DO NOT MERGE :) label Dec 5, 2023

travisn requested changes Dec 6, 2023

View reviewed changes

satoru-takeuchi mentioned this pull request Dec 6, 2023

support create osd with different metadata devices #13280

Closed

microyahoo force-pushed the partition branch from 95a99d6 to 1299eee Compare December 15, 2023 08:00

microyahoo marked this pull request as ready for review December 15, 2023 10:05

microyahoo requested a review from travisn December 15, 2023 10:06

microyahoo changed the title ~~[WIP] osd: support create osd with metadata partition~~ osd: support create osd with metadata partition Dec 15, 2023

microyahoo force-pushed the partition branch from 1299eee to 012222b Compare December 15, 2023 10:07

microyahoo force-pushed the partition branch 4 times, most recently from 891466c to 459a6cb Compare December 23, 2023 13:26

satoru-takeuchi requested changes Jan 16, 2024

View reviewed changes

Documentation/CRDs/Cluster/ceph-cluster-crd.md Outdated Show resolved Hide resolved

microyahoo force-pushed the partition branch from efe5e71 to 5ff4f4b Compare January 18, 2024 06:55

microyahoo requested a review from satoru-takeuchi January 18, 2024 07:00

satoru-takeuchi reviewed Jan 23, 2024

View reviewed changes

microyahoo force-pushed the partition branch from 5ff4f4b to 6736c20 Compare January 27, 2024 06:53

microyahoo requested a review from satoru-takeuchi January 27, 2024 08:07

satoru-takeuchi requested changes Feb 2, 2024

View reviewed changes

pkg/daemon/ceph/osd/volume.go Show resolved Hide resolved

microyahoo force-pushed the partition branch 2 times, most recently from 8ecd944 to 97ac157 Compare February 2, 2024 07:37

microyahoo requested a review from satoru-takeuchi February 2, 2024 08:03

microyahoo force-pushed the partition branch 2 times, most recently from cacd0f4 to b3c0f3c Compare February 6, 2024 02:57

microyahoo force-pushed the partition branch from b3c0f3c to ee05e82 Compare February 7, 2024 03:44

satoru-takeuchi approved these changes Feb 8, 2024

View reviewed changes

travisn approved these changes Feb 8, 2024

View reviewed changes

satoru-takeuchi merged commit f1aa4f3 into rook:master Feb 8, 2024
51 checks passed

satoru-takeuchi added backport-release-1.13 ceph-osd and removed do-not-merge DO NOT MERGE :) labels Feb 8, 2024

mergify bot mentioned this pull request Feb 8, 2024

osd: support create osd with metadata partition (backport #13314) #13733

Merged

mergify bot added a commit that referenced this pull request Feb 8, 2024

Merge pull request #13733 from rook/mergify/bp/release-1.13/pr-13314

1278a78

osd: support create osd with metadata partition (backport #13314)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osd: support create osd with metadata partition #13314

osd: support create osd with metadata partition #13314

microyahoo commented Dec 4, 2023 •

edited by satoru-takeuchi

satoru-takeuchi commented Dec 5, 2023

travisn left a comment

satoru-takeuchi commented Dec 6, 2023

microyahoo commented Dec 6, 2023 •

edited

travisn commented Dec 15, 2023

microyahoo commented Dec 16, 2023

satoru-takeuchi commented Dec 16, 2023

microyahoo commented Dec 16, 2023

microyahoo commented Dec 18, 2023 •

edited

satoru-takeuchi commented Dec 19, 2023

satoru-takeuchi commented Dec 21, 2023

microyahoo commented Dec 21, 2023 •

edited

satoru-takeuchi commented Dec 21, 2023

satoru-takeuchi commented Jan 12, 2024

satoru-takeuchi left a comment

satoru-takeuchi commented Feb 1, 2024 •

edited

microyahoo commented Feb 2, 2024 •

edited

subhamkrai commented Feb 2, 2024

mergify bot commented Feb 7, 2024

microyahoo commented Feb 7, 2024

satoru-takeuchi commented Feb 8, 2024

travisn left a comment

osd: support create osd with metadata partition #13314

osd: support create osd with metadata partition #13314

Conversation

microyahoo commented Dec 4, 2023 • edited by satoru-takeuchi

satoru-takeuchi commented Dec 5, 2023

travisn left a comment

Choose a reason for hiding this comment

satoru-takeuchi commented Dec 6, 2023

microyahoo commented Dec 6, 2023 • edited

travisn commented Dec 15, 2023

microyahoo commented Dec 16, 2023

satoru-takeuchi commented Dec 16, 2023

microyahoo commented Dec 16, 2023

microyahoo commented Dec 18, 2023 • edited

satoru-takeuchi commented Dec 19, 2023

satoru-takeuchi commented Dec 21, 2023

microyahoo commented Dec 21, 2023 • edited

satoru-takeuchi commented Dec 21, 2023

satoru-takeuchi commented Jan 12, 2024

satoru-takeuchi left a comment

Choose a reason for hiding this comment

satoru-takeuchi commented Feb 1, 2024 • edited

microyahoo commented Feb 2, 2024 • edited

subhamkrai commented Feb 2, 2024

mergify bot commented Feb 7, 2024

microyahoo commented Feb 7, 2024

satoru-takeuchi commented Feb 8, 2024

travisn left a comment

Choose a reason for hiding this comment

microyahoo commented Dec 4, 2023 •

edited by satoru-takeuchi

microyahoo commented Dec 6, 2023 •

edited

microyahoo commented Dec 18, 2023 •

edited

microyahoo commented Dec 21, 2023 •

edited

satoru-takeuchi commented Feb 1, 2024 •

edited

microyahoo commented Feb 2, 2024 •

edited