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.

Sign up for GitHub

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

Jump to bottom

ceph-volume: introduce class hierachy for strategies #25238

Merged
merged 3 commits into from
Dec 12, 2018
Merged

ceph-volume: introduce class hierachy for strategies #25238

merged 3 commits into from
Dec 12, 2018

Conversation

jan--f
Copy link
Contributor

@jan--f jan--f commented Nov 23, 2018 •
edited
Loading

Removes duplicate code.

Signed-off-by: Jan Fajerski jfajerski@suse.com

Fixes: http://tracker.ceph.com/issues/37389

@tchaikov
Copy link
Contributor

retest this please

@alfredodeza
Copy link
Contributor

This needs a ticket in the tracker so that it can be associated with it, and I think it would be useful to split these commits

@jan--f jan--f force-pushed the c-v-refactor-strategies-into-hierachy branch 2 times, most recently from 4a32a5a to 5d1196e Compare November 26, 2018 13:04
@jan--f
Copy link
Contributor Author

jan--f commented Nov 26, 2018

This needs a ticket in the tracker so that it can be associated with it, and I think it would be useful to split these commits

Tracker ticket added and commits split (though I'm still not sure how this improves things).

@jan--f jan--f force-pushed the c-v-refactor-strategies-into-hierachy branch from 5d1196e to fb24cfd Compare November 26, 2018 13:08
@jan--f
Copy link
Contributor Author

jan--f commented Dec 3, 2018

@alfredodeza anything blocking this?

@jan--f
Copy link
Contributor Author

jan--f commented Dec 7, 2018

@alfredodeza @andrewschoen @ErwanAliasr1 ping

leseb
leseb approved these changes Dec 7, 2018
View reviewed changes
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

Thanks @jan--f, it's a good initial change. What I'd like to see in the future is a better distinction between SSDs and NVMe, so basically we should try something else than rotational to verify that.

@jan--f
Copy link
Contributor Author

jan--f commented Dec 7, 2018

@leseb Yes, the extension to batch (https://github.com/jan--f/ceph/tree/c-v-extend-batch) will be a step in that direction I believe. It'll refactor the strategies so that data and wl/db volumes/devices can be explicitly specified. Other code (or even a user) can then classify devices as needed.

@alfredodeza
Copy link
Contributor

@jan--f we are trying to get pr #25354 done before this. I'm thinking next week at the latest

@jan--f jan--f force-pushed the c-v-refactor-strategies-into-hierachy branch from fb24cfd to 664f7c0 Compare December 11, 2018 09:04
@jan--f
Copy link
Contributor Author

jan--f commented Dec 11, 2018

jenkins test ceph-volume lvm all

@jan--f
Copy link
Contributor Author

jan--f commented Dec 11, 2018

Functional tests fail due to regression fixed by #25469

@alfredodeza
Copy link
Contributor

Mind a quick rebase so that we can verify once again before merging? Thanks!

@jan--f jan--f force-pushed the c-v-refactor-strategies-into-hierachy branch 2 times, most recently from 4a7ba5b to 0df03b9 Compare December 12, 2018 12:43
@jan--f
Copy link
Contributor Author

jan--f commented Dec 12, 2018

@alfredodeza rebased

@jan--f
Copy link
Contributor Author

jan--f commented Dec 12, 2018

jenkins test ceph-volume lvm all

Jan Fajerski added 3 commits December 12, 2018 15:02
ceph-volume: add strategies.py to for shared code
b38987e 
Signed-off-by: Jan Fajerski <jfajerski@suse.com>
ceph-volume: bluestore strategy use strategy class hierarchy
302334a 
Signed-off-by: Jan Fajerski <jfajerski@suse.com>
ceph-volume: filestore strategy use strategy class hierarchy
34df12e 
Signed-off-by: Jan Fajerski <jfajerski@suse.com>
@jan--f jan--f force-pushed the c-v-refactor-strategies-into-hierachy branch from 0df03b9 to 34df12e Compare December 12, 2018 14:03
@jan--f
Copy link
Contributor Author

jan--f commented Dec 12, 2018

jenkins test ceph-volume lvm all

@jan--f
Copy link
Contributor Author

jan--f commented Dec 12, 2018

All tests fail with stat: cannot stat '/var/run/ceph/test-mon*.asok': No such file or directory. Seems unrelated, any idea @alfredodeza?

@alfredodeza
Copy link
Contributor

@jan--f yeah ceph-ansible changes caused this regression ceph/ceph-ansible#3440

I'm going to just merge this, otherwise this would need to wait another day or so while we fix this for master

@alfredodeza alfredodeza merged commit d178a5c into ceph:master Dec 12, 2018
This was referenced Dec 14, 2018
Merged
Merged
@alfredodeza
Copy link
Contributor

This PR had valid tox failures. Left an unused import behind in stragegies/bluestore.py

The fix is being addressed with this commit: d127ae3

@jan--f
Copy link
Contributor Author

jan--f commented Dec 19, 2018

Sorry my bad. That was left over from the last rebase.

@jan--f jan--f mentioned this pull request Jan 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leseb leseb leseb approved these changes

@andrewschoen andrewschoen Awaiting requested review from andrewschoen

@alfredodeza alfredodeza Awaiting requested review from alfredodeza

@ErwanAliasr1 ErwanAliasr1 Awaiting requested review from ErwanAliasr1

Assignees
No one assigned
Labels
ceph-volume cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jan--f @tchaikov @alfredodeza @leseb