Skip to content

ceph-volume: introduce class hierachy for strategies#25238

Merged
alfredodeza merged 3 commits intoceph:masterfrom
jan--f:c-v-refactor-strategies-into-hierachy
Dec 12, 2018
Merged

ceph-volume: introduce class hierachy for strategies#25238
alfredodeza merged 3 commits intoceph:masterfrom
jan--f:c-v-refactor-strategies-into-hierachy

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

tchaikov commented Nov 25, 2018

retest this please

@alfredodeza
Copy link
Contributor

alfredodeza 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

@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

alfredodeza commented Dec 7, 2018

@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

alfredodeza commented Dec 11, 2018

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

alfredodeza commented Dec 12, 2018

@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

alfredodeza commented Dec 17, 2018

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