ceph-disk: do not always fail when re-using a partition

[JiYou]

1. fix set_data_partition() when partitions provided.
2. fix set_or_create_partition() for lockbox.
3. fix spelling mistake geattr -> getattr
4. args variable in PrepareData.set_type() should be class member.

Signed-off-by: You Ji youji@ebay.com

[tchaikov]

why not just let it throw if the name is not in x? as it is supposed to be in x.

[JiYou]

@tchaikov

As my understanding, it acts as two steps:

• use get_ready_by_name to get all UUIDs list.
• check the given UUID is in this returned list or not?

get_read_by_name(name) is return all UUIDs for given name. If some PTYPE does not contains this name () (such as luks, plain do not contains lockbox) I think should skip them. The usage of get_ready_by_name() in main.py is that after get all UUIDs list, it then check whether the given UUID is located in the list.

Case 1

3716         (tag, uuid) = name.split('.')
3717
3718         if tag in Ptype.get_ready_by_name('osd'):  # <- check tag in all uuids list?
3719
3720             if Ptype.is_dmcrypt(tag, 'osd'):
3721                 path = os.path.join('/dev/mapper', uuid)
3722             else:
3723                 path = os.path.join(dir, name)

Case 2

4075             ptype = get_partition_type(dev)
4076             LOG.debug("main_list: " + dev +
4077                       " ptype = " + str(ptype) +
4078                       " uuid = " + str(part_uuid))
4079             if ptype in Ptype.get_ready_by_name('osd'):  # check ptype in all uuids list?
4080                 if Ptype.is_dmcrypt(ptype, 'osd'):
4081                     holders = is_held(dev)
4082                     if len(holders) != 1:

Example

PTYPE = {
    'regular': {
        ...
        'osd': {
            'ready': '4fbd7e29-9d25-41b8-afd0-062c0ceff05d',
            'tobe': '89c57f98-2fe5-4dc0-89c1-f3ad0ceff2be',
        },
        'lockbox': {
            'ready': 'fb3aabf9-d25f-47cc-bf5e-721d1816496b',
            'tobe': 'fb3aabf9-d25f-47cc-bf5e-721d181642be',
        },
    },
    'luks': {
        ...
        'osd': {
            'ready': '4fbd7e29-9d25-41b8-afd0-35865ceff05d',
            'tobe': '89c57f98-2fe5-4dc0-89c1-5ec00ceff2be',
        },
    },
    'plain': {
        ...
        'osd': {
            'ready': '4fbd7e29-9d25-41b8-afd0-5ec00ceff05d',
            'tobe': '89c57f98-2fe5-4dc0-89c1-5ec00ceff2be',
        },
    },
    'mpath': {
        ...
        'osd': {
            'ready': '4fbd7e29-8ae0-4982-bf9d-5a8d867af560',
            'tobe': '89c57f98-8ae0-4982-bf9d-5a8d867af560',
        },
        'lockbox': {
            'ready': '7f4a666a-16f3-47a2-8445-152ef4d03f6c',
            'tobe': '7f4a666a-16f3-47a2-8445-152ef4d032be',
        },
    },
}

print get_ready_by_name('osd')
print get_ready_by_name('lockbox')

The output should be:

['4fbd7e29-9d25-41b8-afd0-062c0ceff05d', '4fbd7e29-9d25-41b8-afd0-35865ceff05d', '4fbd7e29-9d25-41b8-afd0-5ec00ceff05d', '4fbd7e29-8ae0-4982-bf9d-5a8d867af560']  # <-- all osd UUIDs.
['fb3aabf9-d25f-47cc-bf5e-721d1816496b', '7f4a666a-16f3-47a2-8445-152ef4d03f6c']  # <-- all lockbox UUIDs.

For lockbox usage:

            if ptype not in ready:   # <-- Here will check whether UUID is in the list.
                LOG.warning('incorrect partition UUID: %s, expected %s'
                            % (ptype, str(ready)))

[JiYou]

@tchaikov I've one question about this:

The original code:

2536     def set_data_partition(self):
2537         if is_partition(self.args.data):
2538             LOG.debug('OSD data device %s is a partition',
2539                       self.args.data)
2540             self.partition = DevicePartition.factory(
2541                 path=None, dev=self.args.data, args=self.args)
2542             ptype = partition.get_ptype()
2543             ready = Ptype.get_ready_by_type('osd')
2544             if ptype not in ready:
2545                 LOG.warning('incorrect partition UUID: %s, expected %s'
2546                             % (ptype, str(ready)))

2238     def set_or_create_partition(self):
2239         if is_partition(self.args.lockbox):
2240             LOG.debug('OSD lockbox device %s is a partition',
2241                       self.args.lockbox)
2242             self.partition = DevicePartition.factory(
2243                 path=None, dev=self.args.lockbox, args=self.args)
2244             ptype = partition.get_ptype()
2245             ready = Ptype.get_ready_by_type('lockbox')
2246             if ptype not in ready:
2247                 LOG.warning('incorrect partition UUID: %s, expected %s'
2248                             % (ptype, str(ready)))

I think for data protection purpose, if find if ptype not in ready:, here should raise error raise Error(xxxx). If the user really wants to use this disk as ceph journal/data. We may add LOG.warning mentions how to setup the ceph partition. Such as:

/sbin/sgdisk --new=2:0:+5120M --change-name=2:ceph journal --partition-guid=2:81accb1a-8650-42f8-9534-1888c47b3090 --typecode=2:45b0969e-9b03-4f30-b4c6-b4b80ceff106 --mbrtogpt -- /dev/sdx
/sbin/sgdisk --largest-new=1 --change-name=1:ceph data --partition-guid=1:920ec801-433d-47fc-9872-c7449eb1901d --typecode=1:89c57f98-2fe5-4dc0-89c1-f3ad0ceff2be --mbrtogpt -- /dev/sdx

etc. or given some doc links.

[tchaikov]

-        return [x[name]['ready'] for x in PTYPE.values()]
+        return [x[name]['ready'] for x in PTYPE.values() if x.get(name, None)]

maybe we can put something like

return [x[name]['ready'] for x in PTYPE.values() if name in x]

instead. more pythonic, imho =)

[tchaikov]

makes sense... but i am not sure i understand ceph-disk enough to comment on this. @dachary what do you think?

[tchaikov]

use get_ready_by_name to get all UUIDs list.

ahh, sorry i missed this change. yeah. we should not error out if it's missing.

[ghost]

good catch ! http://tracker.ceph.com/issues/15450 meaning there is no coverage for this code path

[ghost]

@JiYou this looks great, thanks for catching and fixing this !

[ghost]

the make check is stuck ( http://tracker.ceph.com/issues/15452 to track this ) running it manually

[ghost]

passed make check run manually

[JiYou]

Thanks for your comments and valuable suggestions. @tchaikov @dachary

[xiaoxichen]

@dachary ,any test case can be enhanced here? as we even miss to test out the spell mistake :(

[ghost]

@xiaoxichen yes, there is a need for unit tests and integration tests

[JiYou]

@dachary @xiaoxichen got it.

[JiYou]

@dachary to double-check with you.

1. for unit tests, added to ceph/src/ceph-disk/tests?
2. for integration test, is the same place?

[ghost]

@JiYou the unit tests are at ceph/src/ceph-disk/tests indeed. The integration tests are in https://github.com/ceph/ceph/tree/master/qa/workunits/ceph-disk and run via https://github.com/ceph/ceph-qa-suite/tree/master/suites/ceph-disk/basic . The most convenient way to run integration tests is via http://ceph-workbench.readthedocs.org/, provided you have access to an OpenStack tenant.

[JiYou]

@dachary thanks very much.

[JiYou]

@dachary in #8647

1. add test case for PrepareData.set_data_partition()
2. fix some issue in qa/workunits
3. fix some issue in ceph-disk/tests/test_main.py

BTW, I want to add test case for Lockbox.set_or_create_partition() function. But I've tried several commands with lockbox, such as:

ceph-disk prepare --lockbox /dev/vdd1 --lockbox-uuid `uuidgen` --fs-type xfs /dev/vdc1 /dev/vdc2

But seems can not walk into Lockbox's code.
Can you show me the command? thanks.