ceph-volume: revert partition as disk by jan--f · Pull Request #25390 · ceph/ceph

jan--f · 2018-12-04T11:02:40Z

This reverts two commits from PR #25330. Really sorry I didn't catch this in the review. The main reason for this revert is, that it breaks ceph-volume inventory as it is not prepared to deal with partitions being reported as disks (and the dict carrying a different set of keys...see example output below).

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

Furthermore I think there're other arguments to be made against this change:

It dramatically changes what disk.get_devices() has done in the past and is very different from what the name suggests.
It simply duplicates the information. As illustrated in the output below, the exact same partition information is already part of the device itself. Why does it need to be duplicated?
So far I could not find where this additional data is used? Other then the test case (also reverted here) this new functionality seems not used. I'm sure I just didn't look closely enough but I'm certain, that the same functionality can be implemented based on the existing partition reporting done by disk.get_devices()

{'/dev/vda': {'human_readable_size': '40.00 GB',
              'locked': 1,
              'model': '',
              'nr_requests': '256',
              'partitions': {'vda1': {'holders': [],
                                      'sectors': '83884032',
                                      'sectorsize': 512,
                                      'size': '40.00 GB',
                                      'start': '2048'}},
              'path': '/dev/vda',
              'removable': '0',
              'rev': '',
              'ro': '0',
              'rotational': '1',
              'sas_address': '',
              'sas_device_handle': '',
              'scheduler_mode': 'mq-deadline',
              'sectors': 0,
              'sectorsize': '512',
              'size': 42949672960.0,
              'support_discard': '',
              'vendor': '0x1af4'},
 '/dev/vda1': {'holders': [],
               'sectors': '83884032',
               'sectorsize': 512,
               'size': '40.00 GB',
               'start': '2048'},
}

alfredodeza · 2018-12-04T12:04:44Z

Mind expanding how it breaks? There aren't details of this in the ticket either.

The information is needed because the way the Device() class gets populated with metadata is with get_devices(), this information is then used throughout the object to detect things like if the partition is encrypted (and if it is a partition). This PR can't simply revert commits like this because it breaks zapping.

The functional tests in tests/functional/lvm will probably report breakage on any scenario

andrewschoen · 2018-12-04T14:23:52Z

jenkins test ceph-volume lvm all

andrewschoen · 2018-12-04T14:24:19Z

jenkins test ceph-volume batch all

andrewschoen

The CI is finding failures with lvm zap because of this change. I think the correct thing to do here is modify ceph-volume inventory to handle this new format. However, if you still feel strongly about changing the structure then you'll also need to provide a fix for lvm zap to accommodate that along with the revert.

jan--f · 2018-12-04T15:25:40Z

So far I could not find where this additional data is used? Other then the test case (also reverted here) this new functionality seems not used. I'm sure I just didn't look closely enough but I'm certain, that the same functionality can be implemented based on the existing partition reporting done by disk.get_devices()

As I said I'm sure the zap functionality can be done with the existing code. I'll take a closer look.

jan--f · 2018-12-04T16:07:22Z

jenkins test ceph-volume lvm all

jan--f · 2018-12-04T16:26:57Z

Mind expanding how it breaks? There aren't details of this in the ticket either.

It tries to print the human_readable_size, but this key doesn' exist for partitions.

The information is needed because the way the Device() class gets populated with metadata is with get_devices(), this information is then used throughout the object to detect things like if the partition is encrypted (and if it is a partition). This PR can't simply revert commits like this because it breaks zapping.

I'm wondering why the existing partition property on a device can not be used for that. It's the same information after all.

andrewschoen · 2018-12-04T16:52:18Z

It tries to print the human_readable_size, but this key doesn' exist for partitions.

Seems like inventory could handle a missing key pretty easily. Or I'm sure we could add that key if that's all you're missing.

I'm wondering why the existing partition property on a device can not be used for that. It's the same information after all.

I suspect the partition was added to the top-level to make it easy to access it's information. In lvm zap it'll be common to zap a partition. So instead of taking a partition as input, finding it's parent device and then traversing it's information to find the partition it can just access it directly.

jan--f · 2018-12-04T21:51:44Z

jenkins test ceph-volume lvm all

jan--f · 2018-12-05T07:45:48Z

jenkins test ceph-volume lvm all

jan--f · 2018-12-05T08:32:18Z

@andrewschoen What do you think of this?

Seems like inventory could handle a missing key pretty easily. Or I'm sure we could add that key if that's all you're missing.

Right, that would be straight forward. However I'd rather not see disk.get_devices() change though. I think it's a clear abstraction as it was and adding partitions as top level devices seems confusing and redundant.

I suspect the partition was added to the top-level to make it easy to access it's information. In lvm zap it'll be common to zap a partition. So instead of taking a partition as input, finding it's parent device and then traversing it's information to find the partition it can just access it directly.

Since the number of devices that need to be traversed is bounded (I expect we'll not see nodes with > 100 devices attached any time soon) I think this traversing overhead is bearable in exchange for a clean abstraction in disk.get_devices().

alfredodeza

This addition doesn't have unit tests to go along with it, please add those. Also, it was mentioned that the inventory command broke, so I think a call to inventory should be done in the functional tests (located in tests/functional/). There are a few calls to ceph-volume lvm list for example, inventory could be run after that, or before too. That way a non-zero exit status will make our functional tests fail and we can catch this problem

src/ceph-volume/ceph_volume/util/device.py

jan--f · 2018-12-07T09:01:37Z

jenkins test ceph-volume lvm all

jan--f · 2018-12-07T14:08:45Z

@alfredodeza ping

src/ceph-volume/ceph_volume/tests/functional/lvm/playbooks/test_bluestore.yml

alfredodeza · 2018-12-07T14:46:49Z

src/ceph-volume/ceph_volume/util/device.py

        if not sys_info.devices:
            sys_info.devices = disk.get_devices()
        self.sys_api = sys_info.devices.get(self.abspath, {})
+        if not self.sys_api:


this looks great, thanks for updating

jan--f · 2018-12-11T09:01:25Z

@alfredodeza Can we please get this merged?

jschmid1 · 2018-12-11T12:46:59Z

Is this blocked by anything? I also ran into this bug and can confirm this as a valid solution.

alfredodeza · 2018-12-11T14:21:55Z

@jan--f @jschmid1 we had some other PRs that needed to get in, we are getting them merged as we review them. @jan--f if you wouldn't mind rebasing this one more time. I believe this is good to go, should probably be merging this today

… in get_devices" This reverts commit 7f3c359. Fixes: http://tracker.ceph.com/issues/37506 Signed-off-by: Jan Fajerski <jfajerski@suse.com>

…vel keys" This reverts commit 6dc0177. Signed-off-by: Jan Fajerski <jfajerski@suse.com>

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

jan--f · 2018-12-11T14:31:41Z

@alfredodeza rebased

alfredodeza · 2018-12-11T20:17:51Z

jenkins test ceph-volume lvm all

discussed over IRC, agreed to merge

andrewschoen · 2019-01-29T17:24:42Z

@jan--f this doesn't look to be backported to luminous or mimic, could you please do so? Thanks!

jan--f added bug-fix ceph-volume revert labels Dec 4, 2018

jan--f requested review from ErwanAliasr1, alfredodeza and andrewschoen December 4, 2018 11:02

andrewschoen previously requested changes Dec 4, 2018

View reviewed changes

jan--f force-pushed the c-v-revert-partition-as-disk branch from 875a00a to 1879ba7 Compare December 4, 2018 16:05

jan--f force-pushed the c-v-revert-partition-as-disk branch from 1879ba7 to b233973 Compare December 4, 2018 21:51

alfredodeza suggested changes Dec 5, 2018

View reviewed changes

src/ceph-volume/ceph_volume/util/device.py Outdated Show resolved Hide resolved

src/ceph-volume/ceph_volume/util/device.py Outdated Show resolved Hide resolved

jan--f force-pushed the c-v-revert-partition-as-disk branch from b233973 to 1c03776 Compare December 7, 2018 09:01

alfredodeza reviewed Dec 7, 2018

View reviewed changes

src/ceph-volume/ceph_volume/tests/functional/lvm/playbooks/test_bluestore.yml Show resolved Hide resolved

alfredodeza reviewed Dec 7, 2018

View reviewed changes

jan--f force-pushed the c-v-revert-partition-as-disk branch from 1c03776 to e724879 Compare December 7, 2018 15:02

sebastian-philipp mentioned this pull request Dec 10, 2018

mgr/orchestrator: Add test orchestrator #25236

Merged

2 tasks

Jan Fajerski added 4 commits December 11, 2018 15:31

Revert "ceph-volume util.disk include partitions as top level devices…

50bc44f

… in get_devices" This reverts commit 7f3c359. Fixes: http://tracker.ceph.com/issues/37506 Signed-off-by: Jan Fajerski <jfajerski@suse.com>

Revert "ceph-volume tests.util ensure partitions are listed as top-le…

3c1c32c

…vel keys" This reverts commit 6dc0177. Signed-off-by: Jan Fajerski <jfajerski@suse.com>

ceph-volume: add partition support to Device

05ac680

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

ceph-volume: add inventory call to functional tests

a183a28

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

jan--f force-pushed the c-v-revert-partition-as-disk branch from e724879 to a183a28 Compare December 11, 2018 14:31

alfredodeza approved these changes Dec 12, 2018

View reviewed changes

alfredodeza merged commit 1dcbf2f into ceph:master Dec 12, 2018

This was referenced Feb 6, 2019

[mimic] ceph-volume: revert partition as disk #26294

Merged

[luminous] ceph-volume: revert partition as disk #26295

Merged

Conversation

jan--f commented Dec 4, 2018

Uh oh!

alfredodeza commented Dec 4, 2018

Uh oh!

andrewschoen commented Dec 4, 2018

Uh oh!

andrewschoen commented Dec 4, 2018

Uh oh!

andrewschoen left a comment

Choose a reason for hiding this comment

Uh oh!

jan--f commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan--f commented Dec 4, 2018

Uh oh!

jan--f commented Dec 4, 2018

Uh oh!

andrewschoen commented Dec 4, 2018

Uh oh!

jan--f commented Dec 4, 2018

Uh oh!

jan--f commented Dec 5, 2018

Uh oh!

jan--f commented Dec 5, 2018

Uh oh!

alfredodeza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jan--f commented Dec 7, 2018

Uh oh!

jan--f commented Dec 7, 2018

Uh oh!

Uh oh!

alfredodeza Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

jan--f commented Dec 11, 2018

Uh oh!

jschmid1 commented Dec 11, 2018

Uh oh!

alfredodeza commented Dec 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan--f commented Dec 11, 2018

Uh oh!

alfredodeza commented Dec 11, 2018

Uh oh!

andrewschoen commented Jan 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jan--f commented Dec 4, 2018 •

edited

Loading

alfredodeza commented Dec 11, 2018 •

edited

Loading