Skip to content

rbdmap: replace rbdmap with systemd unit generator#40844

Open
huww98 wants to merge 8 commits intoceph:mainfrom
SMIL-Infra:rbdmap-gen
Open

rbdmap: replace rbdmap with systemd unit generator#40844
huww98 wants to merge 8 commits intoceph:mainfrom
SMIL-Infra:rbdmap-gen

Conversation

@huww98
Copy link
Contributor

@huww98 huww98 commented Apr 14, 2021

Replace original rbdmap bash script with rbdmap-generator, which is a systemd unit generator that generates one rbdmap@.service for each RBD image on boot or systemctl daemon-reload.

This generator is written in Python3, and its behavior closely follows systemd-cryptsetup-generator(8). It is designed to be fully compatible with the original /etc/ceph/rbdmap config file.

Apart from original functions, this generator supports several new features:

  • Support adding systemd unit options (e.g. x-systemd.requires=) from /etc/ceph/rbdmap config file. The design closely follows systemd-fstab-generator(8)

  • Support noauto, nofail options. They have the same semantic as in systemd-cryptsetup-generator(8)

  • Previous "id=admin,options='lock_on_read,queue_depth=1024'" can now be written as "id=admin,lock_on_read,queue_depth=1024", which is more nature.

We have some additional advantages compared with the original script:

  • Other units may acquire dependency to a specific RBD device, may through the rbdmap@.service units, or the standard .device and blockdev@.target units.

  • We do not have to deal with mount and umount in the script. The mount unit created by systemd-fstab-generator(8) will automatically pull in and wait until RBD device mapping finished.

  • Furthermore, we may support on-demand RBD mapping. For example, when working with automount, the mount unit triggered by access to the mount point will in turn trigger the RBD device mapping automatically.

However, there are still some breaking changes:

  • Manually invoke rbdmap command is not supported. They may be replaced with systemctl start|stop rbdmap@*.service. rbdmap unmap-all has no replacement.

  • There is no easy way to override the config file path.

Hooks in /etc/ceph/rbd.d/ are still supported but should be considered deprecated. Administrators should use systemd unit file to deploy hooks.

Signed-off-by: 胡玮文 huww98@outlook.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

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 api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@tchaikov
Copy link
Contributor

@wjwithagen hi Willem, does the FreeBSD port use/support rbdmap?

@wjwithagen
Copy link
Contributor

I would prefer to keep this script as well.
I think it could still be usefull (as basic template) for other platforms.
Do not know if you really have to replace/remove it?

@huww98
Copy link
Contributor Author

huww98 commented Apr 14, 2021

@wjwithagen rbdmap and rbdmap-generator now both parse /etc/ceph/rbdmap file, but I've added some new features that would break rbdmap once used. So I removed the rbdmap script to indicate this is an upgrade, and they may not be used interchangeably in the future.

But I think keeping the rbdmap script is also OK. We could state the above point in the man page. And do not distribute rbdmap on Linux platform by default. And I agree it is useful for other platforms.

@wjwithagen
Copy link
Contributor

@huww98
Perhaps document the expected structure in /etc/ceph/rbdmap by yhe script.
And I could imagine running this script on FreeBSD on hypervisors to actually load images into the system with rbd-ggate
Something I never considered doing automated yet, but is of course needed

@trociny trociny requested a review from idryomov April 14, 2021 09:56
@trociny
Copy link
Contributor

trociny commented Apr 14, 2021

I am just wondering, while we are here, couldn't be made more generic to also support other device types (nbd), as it is described in the feature request [1]?

Note, right now rbd map|unmap commands are just aliases to more generic rbd device map|unmap --device-type kernel. It would be nice if we could use this more generic variant, specifying the device-type (kernel,nbd) somewhere in the config.

[1] https://tracker.ceph.com/issues/20762

@huww98
Copy link
Contributor Author

huww98 commented Apr 14, 2021

@trociny

I am just wondering, while we are here, couldn't be made more generic to also support other device types (nbd), as it is described in the feature request [1]?

I've just tested that it just works when you add device-type=nbd to the options list. rbd map also works with other device types.

In my case:

rbd_mixed/ImageNet id=admin,read-only,nofail,noauto,device-type=nbd

Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

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

I left some comments but haven't looked at the new script itself yet.

In the cover letter you say

It is designed to be fully compatible with the original /etc/ceph/rbdmap config file.

but then a few lines below

Previous "id=admin,options='lock_on_read,queue_depth=1024'" can now be written as "id=admin,lock_on_read,queue_depth=1024", which is more nature.

Can the new script handle the old options=... syntax?

In a later comment you say

I've added some new features that would break rbdmap once used.

Can you enumerate what those are, precisely?

@idryomov
Copy link
Contributor

I've just tested that it just works when you add device-type=nbd to the options list. rbd map also works with other device types.

In my case:

rbd_mixed/ImageNet id=admin,read-only,nofail,noauto,device-type=nbd

I wonder we should try to make device-type more prominent. Perhaps move it into its own column? Just a thought, not sure if it's actually worth it.

@huww98
Copy link
Contributor Author

huww98 commented Apr 14, 2021

@idryomov

Can the new script handle the old options=... syntax?

Yes, it can.

In a later comment you say

I've added some new features that would break rbdmap once used.

Can you enumerate what those are, precisely?

Basically everything listed in "new features" from cover-letter: x-systemd things, noauto, nofail, and passing options without write options='a,b'

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Jul 12, 2024
@tchaikov
Copy link
Contributor

@huww98 pls rebase

@huww98 i rebase your change. but looks like your PR does not allow the edit of maintainers. so i cannot update it. could you rebase your change?

@idryomov ping?

@tchaikov tchaikov removed the stale label Jul 12, 2024
huww98 added 8 commits July 15, 2024 15:58
Replace original rbdmap bash script with rbdmap-generator, which is a systemd
unit generator that generates one rbdmap@.service for each RBD image on boot or
`systemctl daemon-reload`.

This generator is written in Python3, and its behavior closely follows
systemd-cryptsetup-generator(8). It is designed to be fully compatible with the
original /etc/ceph/rbdmap config file.

Apart from original functions, this generator supports several new features:

* Support adding systemd unit options (e.g. x-systemd.requires=) from
  /etc/ceph/rbdmap config file. The design closely follows
  systemd-fstab-generator(8)

* Support noauto, nofail options. They have the same semantic as in
  systemd-cryptsetup-generator(8)

* Previous "id=admin,options='lock_on_read,queue_depth=1024'" can now be written
  as "id=admin,lock_on_read,queue_depth=1024", which is more nature.

We have some additional advantages compared with the original script:

* Other units may acquire dependency to a specific RBD device, may through the
  rbdmap@.service units, or the standard .device and blockdev@.target units.

* We do not have to deal with mount and umount in the script. The mount unit
  created by systemd-fstab-generator(8) will automatically pull in and wait
  until RBD device mapping finished.

* Furthermore, we may support on-demand RBD mapping. For example, when working
  with automount, the mount unit triggered by access to the mount point will in
  turn trigger the RBD device mapping automatically.

However, there are still some breaking changes:

* Manually invoke `rbdmap` command is not supported. They may be replaced with
  `systemctl start|stop rbdmap@*.service`. `rbdmap unmap-all` has no
  replacement.

* There is no easy way to override the config file path.

Hooks in /etc/ceph/rbd.d/ are still supported but should be considered
deprecated. Administrators should use systemd unit file to deploy hooks.

Signed-off-by: 胡玮文 <huww98@outlook.com>
replaced with rbdmap-generator

Signed-off-by: 胡玮文 <huww98@outlook.com>
Located at `/etc/ceph/rbdtab`, similar to `/etc/crypttab`. If it is not present,
fallback to old rbdmap.

Signed-off-by: 胡玮文 <huww98@outlook.com>
Signed-off-by: 胡玮文 <huww98@outlook.com>
The hook usage are undocumented. With the current generator approach, using systemd unit override should be a lot easier
than these hooks.

Signed-off-by: 胡玮文 <huww98@outlook.com>
Add deprecated warning for rbdmap
Improve error when the same image is specifiled multiple times
Don't print stack trace for known invalid config

Signed-off-by: 胡玮文 <huww98@outlook.com>
The default pool used by 'rbd device map' command depends on 'rbd_default_pool' configuration. And the actual config
value in effect is hard to acquire.

For capability reason, if rbdmap config is in use, the default pool name is still hard-coded to 'rbd'.

Signed-off-by: 胡玮文 <huww98@outlook.com>
Also add back some content from removed rbdmap(8) to new rbdmap(5)
Also refined examples to include placeholder for optional fields, namespace.

Signed-off-by: 胡玮文 <huww98@outlook.com>
@huww98 huww98 requested a review from a team as a code owner July 15, 2024 07:59
@huww98
Copy link
Contributor Author

huww98 commented Jul 15, 2024

Rebased. Thanks. @tchaikov

@idryomov
Copy link
Contributor

@idryomov ping?

My apologies, I'm swamped with higher-priority work which is intended to be backported to squid right now. I'll try to take a look at this as time allows in the coming weeks. My main concern is that we don't have test coverage (neither for the old way nor for the new way of doing mappings), so this is something that would need to be play-tested manually.

I would like to share our experience of using this on ~20 servers.

  • We use automount to trigger the rbd device map on Ubuntu. fstrim would unintentionally trigger the map(fstrim: ignore automount util-linux/util-linux#1463), which seems fixed, but not backported to Ubuntu 20.04.

  • The rbd device disappeared once (maybe due to a network issue), but the systemd unit remains active. Admin needs to restart the unit manually to recover.

@huww98 Are you continuing to use this production? Anything new to share?

@huww98
Copy link
Contributor Author

huww98 commented Jul 24, 2024

We deploy this in our college. I have graduated now, so I'm not aware of the current status.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Sep 22, 2024
@idryomov idryomov removed the stale label Sep 23, 2024
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Nov 22, 2024
@idryomov idryomov removed the stale label Nov 22, 2024
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Jan 21, 2025
@idryomov idryomov added pinned Use this label if you want to exempt a PR from being stalled and removed stale labels Jan 22, 2025
@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build/ops documentation needs-qa needs-rebase pinned Use this label if you want to exempt a PR from being stalled pybind rbd tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants