Skip to content

cephadm: validate fsid during cephadm shell command#40015

Merged
liewegas merged 1 commit intoceph:masterfrom
Daniel-Pivonka:cephadmshellfsidvalidate
Mar 26, 2021
Merged

cephadm: validate fsid during cephadm shell command#40015
liewegas merged 1 commit intoceph:masterfrom
Daniel-Pivonka:cephadmshellfsidvalidate

Conversation

@Daniel-Pivonka
Copy link

@Daniel-Pivonka Daniel-Pivonka commented Mar 10, 2021

--fsid would take any string and still drop you into a ceph shell that fully worked

verifying the fsid matches the one in the ceph conf

Fixes: https://tracker.ceph.com/issues/49724
Signed-off-by: Daniel Pivonka [email protected]

@Daniel-Pivonka Daniel-Pivonka requested a review from a team as a code owner March 10, 2021 23:08
sebastian-philipp
sebastian-philipp reviewed Mar 11, 2021
View reviewed changes
@Daniel-Pivonka Daniel-Pivonka force-pushed the cephadmshellfsidvalidate branch 3 times, most recently from c5e097b to e0b7148 Compare March 12, 2021 13:33
@sebastian-philipp sebastian-philipp added the wip-swagner-testing My Teuthology tests label Mar 12, 2021
@sebastian-philipp
Copy link
Contributor

sebastian-philipp commented Mar 17, 2021 

2021-03-16T09:49:01.988 INFO:tasks.workunit.client.0.smithi053.stdout:ceph version 16.1.0-736-g8191ac78 (8191ac7873f4253a74b5e91d2a1344df8e2a591c) pacific (rc)
2021-03-16T09:49:02.013 INFO:tasks.workunit.client.0.smithi053.stderr:+ sudo CEPHADM_IMAGE=quay.ceph.io/ceph-ci/ceph:pacific /home/ubuntu/cephtest/clone.client.0/qa/workunits/cephadm/../../../src/cephadm/cephadm version
2021-03-16T09:49:02.013 INFO:tasks.workunit.client.0.smithi053.stderr:+ grep 'ceph version 16'
2021-03-16T09:49:03.639 INFO:tasks.workunit.client.0.smithi053.stdout:ceph version 16.1.0-736-g8191ac78 (8191ac7873f4253a74b5e91d2a1344df8e2a591c) pacific (rc)
2021-03-16T09:49:03.640 INFO:tasks.workunit.client.0.smithi053.stderr:+ grep 'ceph version'
2021-03-16T09:49:03.640 INFO:tasks.workunit.client.0.smithi053.stderr:+ sudo /home/ubuntu/cephtest/clone.client.0/qa/workunits/cephadm/../../../src/cephadm/cephadm --image quay.ceph.io/ceph-ci/ceph:master version
2021-03-16T09:49:59.455 INFO:tasks.workunit.client.0.smithi053.stdout:ceph version 17.0.0-2061-gc62bd5e0 (c62bd5e05ea72e884b14fd3f2555fbc9d4755b33) quincy (dev)
2021-03-16T09:49:59.456 INFO:tasks.workunit.client.0.smithi053.stderr:+ systemctl status docker
2021-03-16T09:49:59.466 INFO:tasks.workunit.client.0.smithi053.stderr:+ sudo /home/ubuntu/cephtest/clone.client.0/qa/workunits/cephadm/../../../src/cephadm/cephadm --image quay.ceph.io/ceph-ci/ceph:master --docker version
2021-03-16T09:49:59.466 INFO:tasks.workunit.client.0.smithi053.stderr:+ grep 'ceph version'
2021-03-16T09:50:27.897 INFO:tasks.workunit.client.0.smithi053.stdout:ceph version 17.0.0-2061-gc62bd5e0 (c62bd5e05ea72e884b14fd3f2555fbc9d4755b33) quincy (dev)
2021-03-16T09:50:27.898 INFO:tasks.workunit.client.0.smithi053.stderr:+ sudo /home/ubuntu/cephtest/clone.client.0/qa/workunits/cephadm/../../../src/cephadm/cephadm --image quay.ceph.io/ceph-ci/ceph:master shell --fsid 00000000-0000-0000-0000-0000deadbeef -- ceph -v
2021-03-16T09:50:27.899 INFO:tasks.workunit.client.0.smithi053.stderr:+ grep 'ceph version'
2021-03-16T09:50:28.226 INFO:tasks.workunit.client.0.smithi053.stderr:ERROR: fsid does not match ceph conf

https://pulpito.ceph.com/swagner-2021-03-16_08:50:44-rados:cephadm-wip-swagner-testing-2021-03-12-1502-distro-basic-smithi/5970558

This was referenced Mar 17, 2021
Closed
Merged
Merged
Merged
@sebastian-philipp sebastian-philipp removed the wip-swagner-testing My Teuthology tests label Mar 17, 2021
liewegas
liewegas reviewed Mar 18, 2021
View reviewed changes
liewegas
liewegas reviewed Mar 18, 2021
View reviewed changes
liewegas
liewegas reviewed Mar 18, 2021
View reviewed changes
@Daniel-Pivonka Daniel-Pivonka force-pushed the cephadmshellfsidvalidate branch from e0b7148 to 8b18344 Compare March 18, 2021 19:20
@Daniel-Pivonka
Copy link
Author

Daniel-Pivonka commented Mar 18, 2021

changed the logic around per sages suggestions. also added a few checks to make sure files exist before trying to open them to avoid trackbacks that are possible in rare situations. test should pass now

@Daniel-Pivonka
Copy link
Author

Daniel-Pivonka commented Mar 19, 2021

jenkins test make check

ktdreyer
ktdreyer reviewed Mar 19, 2021
View reviewed changes
@Daniel-Pivonka Daniel-Pivonka force-pushed the cephadmshellfsidvalidate branch 2 times, most recently from c87d0ad to c5f816f Compare March 24, 2021 14:05
ktdreyer
ktdreyer reviewed Mar 24, 2021
View reviewed changes
liewegas
liewegas reviewed Mar 24, 2021
View reviewed changes
@Daniel-Pivonka Daniel-Pivonka force-pushed the cephadmshellfsidvalidate branch from c5f816f to 9118b08 Compare March 25, 2021 14:26
jmolmo
jmolmo reviewed Mar 25, 2021
View reviewed changes
src/cephadm/cephadm
def command_shell(ctx):
# type: (CephadmContext) -> int
if fsid_conf_mismatch(ctx):
raise Error('fsid does not match ceph conf')
Copy link
Member

@jmolmo jmolmo Mar 25, 2021

Choose a reason for hiding this comment

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

The message is completely clear for us .. but maybe a little bit cryptic for an user..

What about: "fsid provided does not match with fsid found in ceph conf"

@liewegas liewegas merged commit 671ba83 into ceph:master Mar 26, 2021
@liewegas liewegas mentioned this pull request Mar 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liewegas liewegas liewegas approved these changes

@jmolmo jmolmo jmolmo approved these changes

+2 more reviewers

@ktdreyer ktdreyer ktdreyer left review comments

@sebastian-philipp sebastian-philipp sebastian-philipp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cephadm needs-qa wip-sage-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Daniel-Pivonka @sebastian-philipp @liewegas @ktdreyer @jmolmo