Skip to content

crimson: Use osd_objectstore instead of using crimson_osd_objectstore#63817

Merged
Matan-B merged 1 commit intoceph:mainfrom
mohit84:objectstore_type
Jul 7, 2025
Merged

crimson: Use osd_objectstore instead of using crimson_osd_objectstore#63817
Matan-B merged 1 commit intoceph:mainfrom
mohit84:objectstore_type

Conversation

@mohit84
Copy link
Contributor

@mohit84 mohit84 commented Jun 9, 2025

The osd_objectstore supports all type of objectstores (bluestore/filestore/memstore/seastore/cyanstore) so there is no benefit to keep separate option for crimson.

Fixes: https://tracker.ceph.com/issues/71593

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands

@mohit84 mohit84 requested a review from Matan-B June 9, 2025 10:08
@mohit84 mohit84 requested a review from a team as a code owner June 9, 2025 10:08
@mohit84
Copy link
Contributor Author

mohit84 commented Jun 9, 2025

jenkins test make check

@mohit84 mohit84 force-pushed the objectstore_type branch from b70a490 to 5d40c79 Compare June 9, 2025 10:17
@github-actions github-actions bot added the tests label Jun 9, 2025
@mohit84
Copy link
Contributor Author

mohit84 commented Jun 9, 2025

jenkins test make check

Copy link
Contributor

@Matan-B Matan-B left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about:

crimson: Use osd_objectstore instead of using crimson_osd_objectstore

Originally the idea with introducing a crimson dedicated configurable was to allow
changing the default object store to SeaStore without affecting the Classic option.
Moreover, if we were to consider hybrid clusters, different OSD types would require
two different dedicated options.

Given that cephadm (and possibly other orchestrators) use `--osd-obejctstore` to
set the backend - we would prefer to retain (at least for now) this behavior to not
break the **existing** cluster deployment methods.

Note: A follow up PR would be pushed against cephadm to fully support Seastore as
           backend option

src/vstart.sh Outdated
EOF
else
wconf <<EOF
wconf <<EOF
Copy link
Contributor

@Matan-B Matan-B Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is shouldn't be here, we should revert same as before: 15e746b

        kstore fsck on mount = true
        osd objectstore = $objectstore
EOF

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Matan-B Matan-B added this to Crimson Jun 9, 2025
@Matan-B Matan-B moved this to Awaits review in Crimson Jun 9, 2025
@mohit84 mohit84 force-pushed the objectstore_type branch 2 times, most recently from 1d25862 to a9c580d Compare June 9, 2025 11:14
@mohit84
Copy link
Contributor Author

mohit84 commented Jun 9, 2025

Originally the idea with introducing a crimson dedicated configurable was to allow
changing the default object store to SeaStore without affecting the Classic option.
Moreover, if we were to consider hybrid clusters, different OSD types would require
two different dedicated options.

Given that cephadm (and possibly other orchestrators) use --osd-obejctstore to
set the backend - we would prefer to retain (at least for now) this behavior to not
break the existing cluster deployment methods.

Note: A follow up PR would be pushed against cephadm to fully support Seastore as
backend option

I am ok with this.

@mohit84
Copy link
Contributor Author

mohit84 commented Jun 9, 2025

jenkins test make check

Copy link
Contributor

@Matan-B Matan-B left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!
Can you please run this thorough the suite to make sure everything is correct?

@Matan-B Matan-B moved this from Awaits review to Needs QA in Crimson Jun 9, 2025
@mohit84 mohit84 force-pushed the objectstore_type branch from a9c580d to 9b539a5 Compare June 16, 2025 12:24
@github-actions
Copy link

Config Diff Tool Output

- removed: crimson_osd_objectstore (crimson.yaml.in)

The above configuration changes are found in the PR. Please update the relevant release documentation if necessary.

Originally the idea with introducing a crimson dedicated
configurable was to allow changing the default object store to
SeaStore without affecting the Classic option. Moreover, if we
were to consider hybrid clusters, different OSD types would require
two different dedicated options.

Given that cephadm (and possibly other orchestrators) use
`--osd-obejctstore` to set the backend - we would prefer to
retain (at least for now) this behavior to not break the
**existing** cluster deployment methods.

Note: A follow up PR would be pushed against cephadm to
      fully support Seastore as backend option.

Fixes: https://tracker.ceph.com/issues/71593
Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>
@Matan-B
Copy link
Contributor

Matan-B commented Jul 3, 2025

jenkins test make check

@Matan-B
Copy link
Contributor

Matan-B commented Jul 6, 2025

@Matan-B Matan-B merged commit afeb271 into ceph:main Jul 7, 2025
12 of 13 checks passed
@Matan-B Matan-B moved this from Needs QA to Merged (Main) in Crimson Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Merged (Main)

Development

Successfully merging this pull request may close these issues.

3 participants