Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mds: split up mdstypes #46295

Merged
merged 2 commits into from Jun 20, 2022
Merged

mds: split up mdstypes #46295

merged 2 commits into from Jun 20, 2022

Conversation

dparmar18
Copy link
Contributor

@dparmar18 dparmar18 commented May 17, 2022

mdstypes.h contains both MDS-exclusive and client-shared structs.
This PR splits it up into "mdstypes.h" and "sharedtypes.h".

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

Signed-off-by: dparmar18 dparmar@redhat.com

Contribution Guidelines

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
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@github-actions github-actions bot added the cephfs Ceph File System label May 17, 2022
@dparmar18 dparmar18 marked this pull request as ready for review May 17, 2022 06:24
@dparmar18 dparmar18 requested a review from a team May 17, 2022 06:25
src/include/cephfs/sharedtypes.h Outdated Show resolved Hide resolved
src/include/cephfs/sharedtypes.h Outdated Show resolved Hide resolved
src/include/cephfs/sharedtypes.h Outdated Show resolved Hide resolved
src/mds/mdstypes.h Outdated Show resolved Hide resolved
@rishabh-d-dave
Copy link
Contributor

@dparmar18 Signed-off-by line is missing in latest commits.

@dparmar18
Copy link
Contributor Author

@dparmar18 Signed-off-by line is missing in latest commits.

Thanks, it's fixed.

src/mds/mdstypes.h Outdated Show resolved Hide resolved
@dparmar18 dparmar18 force-pushed the tracker3998 branch 2 times, most recently from 06b4934 to 781f836 Compare May 24, 2022 13:43
@github-actions github-actions bot added the rgw label May 24, 2022
@dparmar18 dparmar18 force-pushed the tracker3998 branch 2 times, most recently from 8e6d570 to e463968 Compare May 24, 2022 13:45
@rishabh-d-dave
Copy link
Contributor

@dparmar18 I gave it a quick look, looks okay.

@dparmar18 dparmar18 removed the rgw label May 24, 2022
@rishabh-d-dave
Copy link
Contributor

rishabh-d-dave commented May 24, 2022

@dparmar18 BTW, rgw label is set on this PR. Is it by mistake?

EDIT: You just removed it. XD

@dparmar18
Copy link
Contributor Author

@batrick, Done with all the requested changes. apologies for the mixup related to whitespace modifications. IDE used to remove all the trailing white spaces and I just discovered that setting a few minutes back.

@dparmar18 dparmar18 changed the base branch from master to main May 25, 2022 05:38
Copy link
Contributor

@neesingh-rh neesingh-rh left a comment

Choose a reason for hiding this comment

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

minor nit: newline missing at the end of the /cephfs/types.h.Otherwise, LGTM

@dparmar18
Copy link
Contributor Author

minor nit: newline missing at the end of the /cephfs/types.h.Otherwise, LGTM

Yeah added now

Copy link
Contributor

@nmshelke nmshelke left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

@dparmar18 - https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63017//consoleFull

error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h
+ rm -fr /tmp/install-deps.999737
Build step 'Execute shell' marked build as failure```

@rishabh-d-dave
Copy link
Contributor

Looks good otherwise.

@dparmar18
Copy link
Contributor Author

@dparmar18 - https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63017//consoleFull

error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h
+ rm -fr /tmp/install-deps.999737
Build step 'Execute shell' marked build as failure```

@vshankar What spec file do I need to add this file path to? Also what way should I test it after adding the file-path in the respective spec file?

@lxbsz
Copy link
Member

lxbsz commented Jun 8, 2022

@dparmar18 - https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63017//consoleFull

error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h
+ rm -fr /tmp/install-deps.999737
Build step 'Execute shell' marked build as failure```

@vshankar What spec file do I need to add this file path to? Also what way should I test it after adding the file-path in the respective spec file?

I think you can refer to what the %{_includedir}/cephfs/metrics/Types.h does in ceph.spec.in.

@rishabh-d-dave
Copy link
Contributor

@dparmar18 - https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63017//consoleFull

error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h
+ rm -fr /tmp/install-deps.999737
Build step 'Execute shell' marked build as failure```

@vshankar What spec file do I need to add this file path to? Also what way should I test it after adding the file-path in the respective spec file?

I think you can refer to what the %{_includedir}/cephfs/metrics/Types.h does in ceph.spec.in.

@lxbsz True. I and Dhairya figured the same yesterday but any idea how to re-run that job to test that change? And is possible to do it locally without too much hassle?

@lxbsz
Copy link
Member

lxbsz commented Jun 8, 2022

@dparmar18 - https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63017//consoleFull

error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h
+ rm -fr /tmp/install-deps.999737
Build step 'Execute shell' marked build as failure```

@vshankar What spec file do I need to add this file path to? Also what way should I test it after adding the file-path in the respective spec file?

I think you can refer to what the %{_includedir}/cephfs/metrics/Types.h does in ceph.spec.in.

@lxbsz True. I and Dhairya figured the same yesterday but any idea how to re-run that job to test that change? And is possible to do it locally without too much hassle?

Maybe you can test this locally by using the docker, please see src/test/centos-8/. Or you can push this PR to the https://github.com/ceph/ceph-ci, it should fire a job to build the rpm.

@dparmar18
Copy link
Contributor Author

@dparmar18 - https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63017//consoleFull

error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h
+ rm -fr /tmp/install-deps.999737
Build step 'Execute shell' marked build as failure```

@vshankar What spec file do I need to add this file path to? Also what way should I test it after adding the file-path in the respective spec file?

I think you can refer to what the %{_includedir}/cephfs/metrics/Types.h does in ceph.spec.in.

@lxbsz I can also see ceph.spec.in for centos. fedora and opensuse under src/test, Do i also need to add the file path to those spec files?

@lxbsz
Copy link
Member

lxbsz commented Jun 8, 2022

@dparmar18 - https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63017//consoleFull

error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h
+ rm -fr /tmp/install-deps.999737
Build step 'Execute shell' marked build as failure```

@vshankar What spec file do I need to add this file path to? Also what way should I test it after adding the file-path in the respective spec file?

I think you can refer to what the %{_includedir}/cephfs/metrics/Types.h does in ceph.spec.in.

@lxbsz I can also see ceph.spec.in for centos. fedora and opensuse under src/test, Do i also need to add the file path to those spec files?

The others are all link files, you can check it. So no need.

@dparmar18
Copy link
Contributor Author

@dparmar18 - https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63017//consoleFull

error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h
+ rm -fr /tmp/install-deps.999737
Build step 'Execute shell' marked build as failure```

@vshankar What spec file do I need to add this file path to? Also what way should I test it after adding the file-path in the respective spec file?

I think you can refer to what the %{_includedir}/cephfs/metrics/Types.h does in ceph.spec.in.

@lxbsz I can also see ceph.spec.in for centos. fedora and opensuse under src/test, Do i also need to add the file path to those spec files?

The others are all link files, you can check it. So no need.

Thanks!

@dparmar18
Copy link
Contributor Author

@dparmar18 - https://jenkins.ceph.com/job/ceph-dev-new-build/ARCH=x86_64,AVAILABLE_ARCH=x86_64,AVAILABLE_DIST=centos8,DIST=centos8,MACHINE_SIZE=gigantic/63017//consoleFull

error: Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h


RPM build errors:
    Installed (but unpackaged) file(s) found:
   /usr/include/cephfs/types.h
+ rm -fr /tmp/install-deps.999737
Build step 'Execute shell' marked build as failure```

@vshankar What spec file do I need to add this file path to? Also what way should I test it after adding the file-path in the respective spec file?

I think you can refer to what the %{_includedir}/cephfs/metrics/Types.h does in ceph.spec.in.

@lxbsz True. I and Dhairya figured the same yesterday but any idea how to re-run that job to test that change? And is possible to do it locally without too much hassle?

Maybe you can test this locally by using the docker, please see src/test/centos-8/. Or you can push this PR to the https://github.com/ceph/ceph-ci, it should fire a job to build the rpm.

Okay, but the build process would again take a few hours too, right?

@lxbsz
Copy link
Member

lxbsz commented Jun 8, 2022

...

Maybe you can test this locally by using the docker, please see src/test/centos-8/. Or you can push this PR to the https://github.com/ceph/ceph-ci, it should fire a job to build the rpm.

Okay, but the build process would again take a few hours too, right?

It seems.

mdstypes.h contains both MDS-exclusive and client-shared structs.
This PR splits it up into "mdstypes.h" and "sharedtypes.h".

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

Signed-off-by: Dhairya Parmar <dparmar@redhat.com>
Signed-off-by: Dhairya Parmar <dparmar@redhat.com>
@dparmar18
Copy link
Contributor Author

@vshankar looks like the builds are getting generated successfully in https://shaman.ceph.com/builds/ceph/ after I pushed my latest changes. Build with config that failed previously i.e. centos-x86_64 is built successfully now. PS: build name is 'wip-mds_split'

Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

@vshankar vshankar merged commit c34fd00 into ceph:main Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
8 participants