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

mgr/rbd_support: store global schedule without localized prefix #37864

Merged
merged 2 commits into from Oct 30, 2020

Conversation

trociny
Copy link
Contributor

@trociny trociny commented Oct 27, 2020

so it is still used after mgr failover

Fixes: https://tracker.ceph.com/issues/48020
Signed-off-by: Mykola Golub mgolub@suse.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

@trociny
Copy link
Contributor Author

trociny commented Oct 27, 2020

@dillaman It still uses get_localized_module_option when loading the schedule.

I suppose get_module_option would be more correct here but it would break loading schedules stored (incorrectly) with an older version.

@dillaman
Copy link

Can we have load issue a get_module_option call as well? i.e. attempt to load the global version and if it exists, use it. Otherwise, fall back to the localized version?

so it is still used after mgr failover

Fixes: https://tracker.ceph.com/issues/48020
Signed-off-by: Mykola Golub <mgolub@suse.com>
Signed-off-by: Mykola Golub <mgolub@suse.com>
@trociny
Copy link
Contributor Author

trociny commented Oct 28, 2020

@dillaman updated

@dillaman
Copy link

Unrelated to this PR, but it looks like the make check hung at TestLibRBD.QuiesceWatchError [1]

[1] https://jenkins.ceph.com/job/ceph-pull-requests/62641/consoleFull

if schedule_cfg:
self.handler.module.set_module_option(
self.handler.MODULE_OPTION_NAME, schedule_cfg)
self.handler.module.set_localized_module_option(

Choose a reason for hiding this comment

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

Nit: perhaps put this under the if clause at line 354 after moving it to the non-localized module options. There is technically a possibility of a MGR crash that will leave the old config under the localized section, but it seems unlikely enough and eventually we can remove all this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally put this out of the if clause, so it always had a chance to clean the old option. Consider a case when an old localized option exists for both mgr.x and mgr.y (mgr/rbd_support/x/mirror_snapshot_schedule and mgr/rbd_support/y/mirror_snapshot_schedule). After upgrade the active mgr.x "fixes" mgr/rbd_support/x/mirror_snapshot_schedule making it mgr/rbd_support/mirror_snapshot_schedule. Then mgr.y becomes active, and if it is under if clause it will not remove mgr/rbd_support/y/mirror_snapshot_schedule because mgr/rbd_support/mirror_snapshot_schedule exists. And then if the user decides to remove the global schedule (mgr/rbd_support/mirror_snapshot_schedule) the mgr.y will mysteriously reuse mgr/rbd_support/y/mirror_snapshot_schedule.

@trociny
Copy link
Contributor Author

trociny commented Oct 28, 2020

Unrelated to this PR, but it looks like the make check hung at TestLibRBD.QuiesceWatchError

Thanks, I will try to reproduce this locally.

@dillaman
Copy link

jenkins test make check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants