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 Persist non-lv devices for journals #17403

Merged
merged 15 commits into from Sep 8, 2017
Merged

ceph-volume Persist non-lv devices for journals #17403

merged 15 commits into from Sep 8, 2017

Conversation

alfredodeza
Copy link
Contributor

To be able to persist regular (non-lv) devices we will have to require partitions so that the PARTUUID can be stored in the OSD lv and later queried via blkid

The following changes will survive a system reboot that changes device names (e.g. /dev/sda to /dev/sdb)

See: https://bugzilla.redhat.com/show_bug.cgi?id=1485011

Alfredo Deza added 13 commits August 24, 2017 16:37
ceph-volume lvm.api add a helper to create pvs
00f3fa4 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume lvm.api create the PVolume class
1725a5f 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume lvm.api create the PVolumes class and helpers
f2bc125 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume lvm.prepare make a journal a pv, use uuids always
913cef2 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume exceptions create a specifc error for multiple pvs
73315c4 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume lvm.activate retrieve the journal uuid if journal is a de…
a368fa1 
…vice

Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume lvm.activate always update the link to the journal
6a1de41 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume lvm.api include lv_uuid as output fields
d85656d 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume tests add pv* related unit tests
7c22743 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume tests update the ansible version for functional/tox.ini
007655a 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume lvm.activate use the partuuid of a partition to link the …
a1080f8 
…journal

Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume lvm.prepare store the blkid uuid of a partition journal
5160c53 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume util create a disk utility for blkid operations
a19e128 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
@alfredodeza
Copy link
Contributor Author

jenkins test ceph-volume tox

@andrewschoen andrewschoen mentioned this pull request Aug 31, 2017
Merged
@alfredodeza
Copy link
Contributor Author

This PR has already been tested by ceph-ansible, via:

https://2.jenkins.ceph.com/job/ceph-ansible-scenario/12

Since those changes have not been merged yet

andrewschoen
andrewschoen requested changes Aug 31, 2017
View reviewed changes
Copy link
Contributor

@andrewschoen andrewschoen left a comment

Choose a reason for hiding this comment

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

If we're not allowing a raw device for --journal anymore we should update documentation about that. Also, we might want to mention in the docs that a partition used for --journal must have a PARTUUID or it can't be used.

src/ceph-volume/ceph_volume/devices/lvm/prepare.py
uuid = disk.get_partuuid(argument)
if not uuid:
terminal.error('blkid could not detect a PARTUUID for device: %s' % argument)
raise RuntimeError('unable to use device for a journal')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention the 'why' it can't be used as a journal at all here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might get a bit verbose though, I am inclined to document this rather than adding to the error message. Or where you thinking something succinct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the terminal.error print right there in stdout with the RuntimeError? If so maybe that's enough context. If it was only 'unable to use device for a journal' I wouldn't know why or what to do next.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, anything terminal.*goes to stdout including the RuntimeError. But still, I need to follow up with docs

src/ceph-volume/ceph_volume/tests/functional/centos7/create/group_vars/all Outdated
@@ -8,12 +8,19 @@ monitor_interface: eth1
journal_size: 100
osd_objectstore: "filestore"
osd_scenario: lvm
ceph_stable_release: mimic
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need this anymore.

src/ceph-volume/ceph_volume/tests/functional/xenial/create/group_vars/all Outdated
@@ -8,12 +8,19 @@ monitor_interface: eth1
journal_size: 100
osd_objectstore: "filestore"
osd_scenario: lvm
ceph_stable_release: mimic
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need this

@andrewschoen
Copy link
Contributor

jenkins test ceph-volume xenial-create

@andrewschoen
Copy link
Contributor

jenkins test ceph-volume centos7-create

Alfredo Deza added 2 commits August 31, 2017 13:26
ceph-volume tests add new ceph_* ansible dev variables required (xenial)
ebb48c5 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
ceph-volume tests add new ceph_* ansible dev variables required (centos)
d3abfd4 
Signed-off-by: Alfredo Deza <adeza@redhat.com>
@andrewschoen
Copy link
Contributor

jenkins test ceph-volume xenial-create

@andrewschoen
Copy link
Contributor

jenkins test ceph-volume centos7-create

@alfredodeza
Copy link
Contributor Author

repos were not ready since I re-pushed

@alfredodeza
Copy link
Contributor Author

jenkins test ceph-volume centos7-create

@alfredodeza
Copy link
Contributor Author

jenkins test ceph-volume xenial-create

1 similar comment
@alfredodeza
Copy link
Contributor Author

jenkins test ceph-volume xenial-create

@alfredodeza
Copy link
Contributor Author

ceph-ansible somehow requires we say this mimic, but only for xenial.

The conditional check 'ceph_release_num.{{ ceph_release }} >= ceph_release_num.luminous' failed. The error was: error while evaluating conditional (ceph_release_num.{{ ceph_release }} >= ceph_release_num.luminous): 'dict object' has no attribute 'dummy'

@alfredodeza alfredodeza mentioned this pull request Aug 31, 2017
Merged
@andrewschoen
Copy link
Contributor

jenkins test ceph-volume xenial-create

@andrewschoen andrewschoen merged commit 9693f86 into master Sep 8, 2017
@liewegas liewegas deleted the wip-bz1485011 branch April 12, 2018 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewschoen andrewschoen andrewschoen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@alfredodeza @andrewschoen