Skip to content

mgr/cephadm: Add migration to keep the service names consistent#36284

Merged
sebastian-philipp merged 2 commits intoceph:masterfrom
sebastian-philipp:cephadm_service_id_migration
Jul 31, 2020
Merged

mgr/cephadm: Add migration to keep the service names consistent#36284
sebastian-philipp merged 2 commits intoceph:masterfrom
sebastian-philipp:cephadm_service_id_migration

Conversation

@sebastian-philipp
Copy link
Contributor

@sebastian-philipp sebastian-philipp commented Jul 24, 2020

mgr/cephadm: Add migratoin to keep the service names consistent

After 15.2.4, we unified some service IDs: MONs, MGRs etc no longer have a service id.
Which means, the service names changed:

mon.foo -> mon
mgr.foo -> mgr

This fixes the data structure consistency

This is a follow up on #35839 Especially #35839 (comment)

Signed-off-by: Sebastian Wagner sebastian.wagner@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 dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@sebastian-philipp sebastian-philipp requested a review from a team as a code owner July 24, 2020 14:22
@sebastian-philipp sebastian-philipp force-pushed the cephadm_service_id_migration branch from c0c52a0 to 8e51ed5 Compare July 24, 2020 14:23
mgfritch
mgfritch reviewed Jul 24, 2020
View reviewed changes
Copy link
Contributor

@mgfritch mgfritch left a comment

Choose a reason for hiding this comment

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

seems like this PR applies to all specs? when it should only be this set of services:

ceph/src/python-common/ceph/deployment/service_spec.py

Line 375 in f412106

REQUIRES_SERVICE_ID = 'iscsi mds nfs osd rgw'.split()

src/pybind/mgr/cephadm/migrations.py
Comment on lines +144 to +147
bad_specs = {}
for name, spec in self.mgr.spec_store.specs.items():
if name != spec.service_name():
bad_specs[name] = (spec.service_name(), spec)
Copy link
Contributor

@mgfritch mgfritch Jul 24, 2020

Choose a reason for hiding this comment

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

hmm, because of #35839 (comment)

the bad specs won't be available via the spec_store when the mgr loads ..?

Copy link
Contributor Author

@sebastian-philipp sebastian-philipp Jul 24, 2020

Choose a reason for hiding this comment

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

I explicitly call load():

https://github.com/ceph/ceph/pull/36284/files#diff-d16ae9373a672ea79e66829f5093667fR64

Devp00l
Devp00l reviewed Jul 27, 2020
View reviewed changes
@sebastian-philipp sebastian-philipp changed the title mgr/cephadm: Add migratoin to keep the service names consistent mgr/cephadm: Add migration to keep the service names consistent Jul 27, 2020
@sebastian-philipp sebastian-philipp force-pushed the cephadm_service_id_migration branch from 8e51ed5 to e1bc519 Compare July 27, 2020 13:50
@sebastian-philipp
Copy link
Contributor Author

sebastian-philipp commented Jul 27, 2020

seems like this PR applies to all specs? when it should only be this set of services:

ceph/src/python-common/ceph/deployment/service_spec.py

Line 375 in f412106

REQUIRES_SERVICE_ID = 'iscsi mds nfs osd rgw'.split()

more like the other way around: all services that don't require an ID. Nevertheless. I think we're ok when running this for all specs

@jschmid1
Copy link
Contributor

jschmid1 commented Jul 28, 2020

Setting any affected spec to unmanaged=True feels wrong. There is no mechanism of telling a user what's happening or am I missing something?

@sebastian-philipp
Copy link
Contributor Author

sebastian-philipp commented Jul 28, 2020

Setting any affected spec to unmanaged=True feels wrong. There is no mechanism of telling a user what's happening or am I missing something?

You're right. Do you have any other idea?

@jschmid1
Copy link
Contributor

jschmid1 commented Jul 28, 2020

You're right. Do you have any other idea?

I think this comes down the recurring topic of "How to display error/warning messages to the user?"

But this is again another topic :) I'll set this on the agenda of the next weekly meeting

@sebastian-philipp
Copy link
Contributor Author

sebastian-philipp commented Jul 28, 2020

You're right. Do you have any other idea?

I think this comes down the recurring topic of "How to display error/warning messages to the user?"

But this is again another topic :) I'll set this on the agenda of the next weekly meeting

ok. are you ok with the PR for now?

@jschmid1 jschmid1 self-requested a review July 28, 2020 12:19
jschmid1
jschmid1 approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@jschmid1 jschmid1 left a comment

Choose a reason for hiding this comment

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

ok. are you ok with the PR for now?

yes

@sebastian-philipp
Copy link
Contributor Author

sebastian-philipp commented Jul 30, 2020

https://pulpito.ceph.com/swagner-2020-07-30_07:43:54-rados:cephadm-wip-swagner3-testing-2020-07-29-1512-distro-basic-smithi/

@sebastian-philipp
mgr/cephadm: minor cleanup in test_migration
775d5be 
Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
@sebastian-philipp
mgr/cephadm: Add migration to keep the service names consistent
f2c0b2e 
After 15.2.4, we unified some service IDs: MONs, MGRs etc no longer have a service id.
Which means, the service names changed:

mon.foo -> mon
mgr.foo -> mgr

This fixes the data structure consistency

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
@sebastian-philipp sebastian-philipp force-pushed the cephadm_service_id_migration branch from e1bc519 to f2c0b2e Compare July 30, 2020 12:31
@sebastian-philipp sebastian-philipp merged commit 5c38185 into ceph:master Jul 31, 2020
@sebastian-philipp sebastian-philipp mentioned this pull request Aug 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgfritch mgfritch mgfritch left review comments

+2 more reviewers

@Devp00l Devp00l Devp00l left review comments

@jschmid1 jschmid1 jschmid1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cephadm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sebastian-philipp @jschmid1 @mgfritch @Devp00l