mgr/cephadm: rework osd removals/replacements#36151
Merged
sebastian-philipp merged 5 commits intoceph:masterfrom Jul 31, 2020
Merged
mgr/cephadm: rework osd removals/replacements#36151sebastian-philipp merged 5 commits intoceph:masterfrom
sebastian-philipp merged 5 commits intoceph:masterfrom
Conversation
a279e10 to
6b1d0ad
Compare
Contributor
|
Contributor
Author
currently in the process of working on the tests. note that this is a |
2275ad5 to
4485daf
Compare
Contributor
|
sigh. would be easier to review, if there were multiple commits in this PR |
sebastian-philipp
suggested changes
Jul 24, 2020
9019f20 to
00f6903
Compare
9 tasks
00f6903 to
8261c9c
Compare
Contributor
Author
|
jenkins render docs |
Collaborator
|
Doc render available at http://docs.ceph.com/ceph-prs/36151/ |
Contributor
Author
Contributor
Author
|
jenkins test make check |
Contributor
Contributor
RPM will likely fail, too: https://shaman.ceph.com/builds/ceph/wip-swagner-testing-2020-07-28-1211/3b57d22029fee37a1a4eca44065089f9847e4b5b/default/220075/ |
Contributor
Author
RPMs seem to build just fine. I'll dig into the deb part |
774380c to
9c436dc
Compare
Devp00l
reviewed
Jul 28, 2020
Devp00l
left a comment
There was a problem hiding this comment.
Just tested it, it works fine, but I get a strange result when passing it to JSON.
# ceph orch osd rm status json
["{\"osd_id\": 10, \"started\": true, \"draining\": true, \"stopped\": false, \"replace\": true, \"force\": false, \"nodename\": \"osd0\", \"drain_started_at\": \"2020-07-28T14:55:51.173164\", \"drain_stopped_at\": null, \"drain_done_at\": null, \"process_started_at\": \"2020-07-28T14:55:46.989791\"}"]
# ceph orch osd rm status json | jq
[
"{\"osd_id\": 10, \"started\": true, \"draining\": true, \"stopped\": false, \"replace\": true, \"force\": false, \"nodename\": \"osd0\", \"drain_started_at\": \"2020-07-28T14:55:51.173164\", \"drain_stopped_at\": null, \"drain_done_at\": null, \"process_started_at\": \"2020-07-28T14:55:46.989791\"}"
]
But what I actually wanted to see is this:
# ceph orch osd rm status json | sed 's/"{/{/;s/}"/}/;s/\\"/"/g' | jq
[
{
"osd_id": 10,
"started": true,
"draining": true,
"stopped": false,
"replace": true,
"force": false,
"nodename": "osd0",
"drain_started_at": "2020-07-28T14:55:51.173164",
"drain_stopped_at": null,
"drain_done_at": null,
"process_started_at": "2020-07-28T14:55:46.989791"
}
]
Contributor
Author
|
ceph/src/pybind/mgr/orchestrator/module.py Lines 32 to 41 in 1a8828a Is responsible for creating json objects. I'm passing List[OSD] in and this is what we get. I agree though, the formatting is pretty much broken |
Contributor
|
can you run a cephadm smoke test as well? |
Contributor
Author
right, right.. I used the wrong command from my history :> |
Contributor
Author
|
1/31 failed with: which seems unrelated |
added 2 commits
July 31, 2020 10:11
Signed-off-by: Joshua Schmid <jschmid@suse.de>
Fixes: https://tracker.ceph.com/issues/44548 Fixes: https://tracker.ceph.com/issues/45594 Signed-off-by: Joshua Schmid <jschmid@suse.de>
9c436dc to
d83f7b2
Compare
added 3 commits
July 31, 2020 10:23
Signed-off-by: Joshua Schmid <jschmid@suse.de>
Signed-off-by: Joshua Schmid <jschmid@suse.de>
Signed-off-by: Joshua Schmid <jschmid@suse.de>
570546f to
8ff2824
Compare
Contributor
sebastian-philipp
suggested changes
Jul 31, 2020
Comment on lines
+608
to
+623
| def to_json(self) -> str: | ||
| out = dict() | ||
| out['osd_id'] = self.osd_id | ||
| out['started'] = self.started | ||
| out['draining'] = self.draining | ||
| out['stopped'] = self.stopped | ||
| out['replace'] = self.replace | ||
| out['force'] = self.force | ||
| out['nodename'] = self.nodename # type: ignore | ||
|
|
||
| for k in ['drain_started_at', 'drain_stopped_at', 'drain_done_at', 'process_started_at']: | ||
| if getattr(self, k): | ||
| out[k] = getattr(self, k).strftime(DATEFMT) | ||
| else: | ||
| out[k] = getattr(self, k) | ||
| return json.dumps(out) |
Contributor
There was a problem hiding this comment.
Suggested change
| def to_json(self) -> str: | |
| out = dict() | |
| out['osd_id'] = self.osd_id | |
| out['started'] = self.started | |
| out['draining'] = self.draining | |
| out['stopped'] = self.stopped | |
| out['replace'] = self.replace | |
| out['force'] = self.force | |
| out['nodename'] = self.nodename # type: ignore | |
| for k in ['drain_started_at', 'drain_stopped_at', 'drain_done_at', 'process_started_at']: | |
| if getattr(self, k): | |
| out[k] = getattr(self, k).strftime(DATEFMT) | |
| else: | |
| out[k] = getattr(self, k) | |
| return json.dumps(out) | |
| def to_json(self) -> dict: | |
| out = dict() | |
| out['osd_id'] = self.osd_id | |
| out['started'] = self.started | |
| out['draining'] = self.draining | |
| out['stopped'] = self.stopped | |
| out['replace'] = self.replace | |
| out['force'] = self.force | |
| out['nodename'] = self.nodename # type: ignore | |
| for k in ['drain_started_at', 'drain_stopped_at', 'drain_done_at', 'process_started_at']: | |
| if getattr(self, k): | |
| out[k] = getattr(self, k).strftime(DATEFMT) | |
| else: | |
| out[k] = getattr(self, k) | |
| return out |
Let's keep the types aligned with the rest of the orchestrator. This is the reason, the output looks so wrong.
Contributor
Author
There was a problem hiding this comment.
ah, right! Let's fix this with a followup PR
sebastian-philipp
approved these changes
Jul 31, 2020
Contributor
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: https://tracker.ceph.com/issues/44548
Fixes: https://tracker.ceph.com/issues/45594
Also removes the
osd_supportmodule to simplify the removal logic.ceph orch osd rm stopceph orch osd rm statusTODO:
notify()mechanismSigned-off-by: Joshua Schmid jschmid@suse.de
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard backendjenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox