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.

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: don't use container classes in api/lvm.py #32493

Merged
merged 6 commits into from Jun 30, 2020
Merged

ceph-volume: don't use container classes in api/lvm.py #32493

merged 6 commits into from Jun 30, 2020

Conversation

rishabh-d-dave
Copy link
Contributor

Use get_lvs, get_vgs and get_pvs instead. The hallmark difference
between usage of these methods and container classes is that these methods
use option -S to filter LVM devs. See lvs -S --help for more details on
these filters.

Changes to api/lvm.py will be merged separately; see PR #32231. Only
devices/lvm/listing.py is an exception in this PR since refactoring on this
file happens on a separate PR (see #31700).

These containers classes will be removed from api/lvm.py on a separate PR
after this and related PR merges.

Note: not using these container classes means LVM devs would no more be
cached by ceph-volume.

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 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

@rishabh-d-dave rishabh-d-dave requested a review from a team as a code owner January 6, 2020 06:18
@rishabh-d-dave rishabh-d-dave mentioned this pull request Jan 6, 2020
Merged
3 tasks
@rishabh-d-dave
Copy link
Contributor Author

Rebased the PR, unit tests are passing locally. Please review the PR. @jan--f

jan--f
jan--f previously requested changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@jan--f jan--f left a comment

Choose a reason for hiding this comment

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

Same comments as in #31700 apply. Otherwise look to be on a good way.

@jan--f
Copy link
Contributor

jan--f commented Jan 22, 2020

retest this please

@jan--f
Copy link
Contributor

jan--f commented Jan 22, 2020

jenkins test ceph-volume tox

@rishabh-d-dave
Copy link
Contributor Author

Just a rebase. Unit tests were passing locally.

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume tox

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume tox

@jan--f
Copy link
Contributor

jan--f commented Jan 27, 2020

Sorry...couldn't avoid those conflicts.

@rishabh-d-dave
Copy link
Contributor Author

No worries.

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume tox

@rishabh-d-dave
Copy link
Contributor Author

@jan--f Unit tests passed.

@jan--f
Copy link
Contributor

jan--f commented Jan 28, 2020

Same issue as in #31700. But one of these two PRs needs a rebase after the other merged anyway.

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume tox

@jan--f
Copy link
Contributor

jan--f commented Jun 29, 2020

jenkins test ceph-volume tox

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume all

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume tox

1 similar comment
@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume tox

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jun 29, 2020 •
edited

Can't reproduce the error on tox job, tried fedora 31 as well as centos 8. See https://gist.github.com/rishabh-d-dave/78e433fb53d58b891c3c5434f7d4b540

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume tox

@rishabh-d-dave
Copy link
Contributor Author

@jan--f @andrewschoen tox tests too passed.

@rishabh-d-dave
Copy link
Contributor Author

Rebased my branch since "make check" wasn't passing and the failure isn't related to this PR.

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume tox

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume all

@rishabh-d-dave
ceph-volume: don't use class api.lvm.Volumes
2f5c10c 
Replace code using class Volumes by methods get_lvs() and
get_first_lv(). Also, update the tests accordingly.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
ceph-volume: don't use api.lvm.get_lv_from_argument
5301c49 
Use api.lvm.get_lvs() and api.lvm.get_first_lv() instead and update
tests.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
ceph-volume: don't use api.lvm.get_lv
b225405 
Use api.lvm.get_lvs() and api.lvm.get_first_lv() instead and update the
tests.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
ceph-volume: don't use api.lvm.PVolumes
a6e3c69 
Update api.lvm.get_first_pv() and api.lvm.get_pvs() instead and update
tests.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
ceph-volume: don't use api.lvm.VolumeGroups
dfe6b1b 
Use api.lvm.get_vgs() and api.lvm.get_first_vg() instead and update
tests.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
ceph-volume: delete devices.lvm.prepare.Prepare.get_lv
ca1d3e6 
Since it calls get_lv_from_argument() internally and use get_first_lv()
instead and update tests.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume tox

@rishabh-d-dave
Copy link
Contributor Author

jenkins test ceph-volume all

@jan--f jan--f dismissed their stale review June 30, 2020 12:56

addressed

jan--f
jan--f approved these changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@jan--f jan--f left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jan--f jan--f merged commit 2650550 into ceph:master Jun 30, 2020
This was referenced Jul 1, 2020
Merged
Merged
This was referenced Jul 2, 2020
Closed
Closed
Merged
@rishabh-d-dave rishabh-d-dave deleted the cv-use-dash-S-everywhere branch September 9, 2020 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jan--f jan--f jan--f approved these changes

@andrewschoen andrewschoen Awaiting requested review from andrewschoen

Assignees
No one assigned
Labels
ceph-volume cleanup needs-review
Projects
None yet
Milestone
No milestone
3 participants
@rishabh-d-dave @jan--f @andrewschoen