OSD Migration for changed disk slots #1302

agraul · 2018-08-23T09:10:49Z

Please have a look already, there are still a few things to do (I've noted them below) but the core business logic should stay more or less the same (unless you find some problems 😉)

Description:
OSD Migration currently only works if the same slot is used. This works
because the generation of proposals is deterministic, so if all disks
are in the same position we end up with the same proposal.

This commit changes that behaviour: instead of creating a new proposal,
the old one is used and adapted. This happens in the following steps:

When salt-run replace.osd X is used and a proposal file (e.g.
"data1.ceph.yml" in /srv/pillar/proposals/...) gets the "-replace"
suffix, a new attribute is written for the disks that get replaced
When salt-run proposals.populate is used (e.g. in stage.1), all
proposal files that have the "-replace" suffix get parsed and the
disks in the proposal are compared to the currently installed disks
on the node.
Disks in the old proposal that have the attribute inserted in 1) and
are still present (i.e. replaced in place) plus new disks (disks
that are not in the old proposal but present) are considered free.
The disks in the old proposal get interated over and if they are
labeled for replacement they are swapped with one of the free disks.
This modified proposal gets saved in the place of the original one
and can be used by later stages

Things that should still be done:

Have a way to roll the "destroyed" mark in the CRUSH map back if renaming and inserting of "replace: true" in proposal file fails
Clean up and add more unittests
Add functional tests

Checklist:

Added unittests ~~and or functional tests~~
Adapted documentation
Referenced issues or internal bugtracker
Ran integration tests successfully ( trigger with @susebot run teuthology )

agraul · 2018-08-23T09:13:29Z

srv/modules/runners/proposal.py

+        3. Remove proposal file with "-replace" in its name
+        """
+
+        # import ipdb; ipdb.set_trace()


I'll remove this, that should not be in here

agraul · 2018-08-23T09:27:16Z

Adapting "replacement procedure"

jan--f

Just from reading through it I think this is on a good way. Some docs on how this is expected to be used never hurt. Looking forward to giving this a spin!

jan--f · 2018-08-23T09:21:50Z

srv/modules/runners/proposal.py

+    :param profile_dir: Profile directory, e.g. "/srv/pillar/ceph/proposals/profile-default"
+    """
+    ToReplace = namedtuple('ToReplace', ['fullpath', 'filename'])
+    dir = '{}/stack/default/ceph/minions'.format(profile_dir)


dir is a python built-in function. maybe use dir_ or _dir to avoid confusion?

oh, right! Will change that.

jan--f · 2018-08-23T09:25:06Z

srv/modules/runners/proposal.py

+    """
+    ToReplace = namedtuple('ToReplace', ['fullpath', 'filename'])
+    dir = '{}/stack/default/ceph/minions'.format(profile_dir)
+    files = [f for f in os.listdir(dir) if isfile(join(dir, f))]


You could add the f.endswith('-replace') here and thus only iterate once over all profile files. Somehting like
files = [f for f in os.listdir(dir) if isfile(join(dir, f)) and f.endswith('-replace')]

Good catch, that does not need two iterations over all profiles

jan--f · 2018-08-23T09:32:39Z

srv/modules/runners/replace.py

    return ""


+def _strip_enddigits(string):


Doe this do somehting different then string.rstrip('0123456789')? If I'm not missing anything this method is unnecessary.

No, this is a case of "do before properly looking if there is an built-in that does the same" 😆 I'll adapt the code.

jan--f · 2018-08-23T09:34:03Z

srv/modules/runners/proposal.py

+        for minion in minions_to_replace:
+            ReplaceDiskOn(minion).replace()
+        return True
+    else:


Does this mean that if a replacement is ongoing no new proposals are created? Wondering if that'll bite us at some point when someone replaces a disk and adds a new node in one go...

That is actually a good point I did not think about. I guess we could remove the minions in minions_to_replace from the targets and run proposal.generate on the rest.

jschmid1 · 2018-08-24T08:09:10Z

srv/modules/runners/replace.py

@@ -52,23 +53,24 @@ def osd(*args, **kwargs):

    local = salt.client.LocalClient()
    host_osds = local.cmd('I@roles:storage', 'osd.list', tgt_type='compound')
+    assert isinstance(host_osds, list)


local.cmd('I@roles:storage', 'osd.list', tgt_type='compound') returns a dict of host: list_of_osds

jschmid1 · 2018-08-24T10:25:37Z

srv/salt/_modules/osd.py

@@ -1793,8 +1793,7 @@ def partitions(self, osd_id):

        for device_type in ['journal', 'block', 'block.db', 'block.wal', 'block_dmcrypt']:
            result = self._uuid_device("{}/{}".format(mount_dir, device_type))
-            if result:
-                _partitions[device_type] = result
+            _partitions[device_type] = result


the if result: is still needed as the partitions method also feeds the grains.

If no /dev/disk/by-id symlink is found, e.g. because that path does not exist (as it is a case in VMs), the devicename that was provided in the first place is returned. Signed-off-by: Alexander Graul <[email protected]>

jschmid1 · 2018-08-24T12:00:17Z

Tried:

salt-run replace.osd 1
stage 1,2,3
got the expected result
salt-run replace.osd 1 5(5 being on another host)
stage 1,2,3
got the expected result
salt-run replace.osd 1 5(5 being on another host)
a node is about to be added ( not deployed, no proposal generated )
stage 1,2,3
got the expected result

jschmid1 · 2018-08-24T12:00:59Z

lgtm after those two flapping unittests are fixed.

jschmid1 · 2018-08-24T17:30:07Z

Real hardware yields some problems. In-depth explanation following

OSD Migration currently only works if the same slot is used. This works because the generation of proposals is deterministic, so if all disks are in the same position we end up with the same proposal. This commit changes that behaviour: instead of creating a new proposal, the old one is used and adapted. This happens in the following steps: 1) When `salt-run replace.osd X` is used and a proposal file (e.g. "data1.ceph.yml" in /srv/pillar/proposals/...) gets the "-replace" suffix, a new attribute is written for the disks that get replaced 2) When `salt-run proposals.populate` is used (e.g. in stage.1), all proposal files that have the "-replace" suffix get parsed and the disks in the proposal are compared to the currently installed disks on the node. 3) Disks in the old proposal that have the attribute inserted in 1) and are still present (i.e. replaced in place) plus new disks (disks that are not in the old proposal but present) are considered free. 4) The disks in the old proposal get interated over and if they are labeled for replacement they are swapped with one of the free disks. 5) This modified proposal gets saved in the place of the original one and can be used by later stages Once the replacement in the proposal went through, new proposals are generated on all other nodes in case replacements and node additions happen at once. Signed-off-by: Alexander Graul <[email protected]>

Signed-off-by: Alexander Graul <[email protected]>

jschmid1 · 2018-08-27T15:09:44Z

@susebot run teuthology

agraul · 2018-08-27T15:13:38Z

retest this please

susebot · 2018-08-27T15:49:07Z

Commit 02858eb is NOT OK for suite suse:tier1.
Check tests results in the Jenkins job: http://158.69.90.90:8080/job/deepsea-pr/98/

jschmid1 · 2018-08-27T15:51:20Z

8/9 failures are infrastructure related

agraul added the priority label Aug 23, 2018

agraul requested review from jan--f, tserong, swiftgist and jschmid1 August 23, 2018 09:10

agraul commented Aug 23, 2018

View reviewed changes

jan--f reviewed Aug 23, 2018

View reviewed changes

agraul force-pushed the wip-sorting-proposal branch 2 times, most recently from 840041f to 0c930cc Compare August 23, 2018 13:46

jschmid1 reviewed Aug 24, 2018

View reviewed changes

Return devicename if no /dev/disk/by-id path

3c26439

If no /dev/disk/by-id symlink is found, e.g. because that path does not exist (as it is a case in VMs), the devicename that was provided in the first place is returned. Signed-off-by: Alexander Graul <[email protected]>

agraul force-pushed the wip-sorting-proposal branch 2 times, most recently from 0de3628 to f0c2e89 Compare August 24, 2018 11:35

jschmid1 added the needs-backport label Aug 24, 2018

agraul force-pushed the wip-sorting-proposal branch from f0c2e89 to bd6f3f5 Compare August 24, 2018 11:54

agraul force-pushed the wip-sorting-proposal branch from bd6f3f5 to b27e1b9 Compare August 24, 2018 12:03

agraul mentioned this pull request Aug 24, 2018

[backport][SES5] OSD Migration for changed disk slots #1311

Merged

4 tasks

agraul force-pushed the wip-sorting-proposal branch from 931f4e0 to 2a372b2 Compare August 24, 2018 17:06

agraul force-pushed the wip-sorting-proposal branch from 2a372b2 to fb52800 Compare August 24, 2018 18:11

jschmid1 mentioned this pull request Aug 27, 2018

[RFC] Test determinism for proposal.generate #1260

Closed

agraul force-pushed the wip-sorting-proposal branch from fb52800 to d18ac8f Compare August 27, 2018 14:47

agraul added 2 commits August 27, 2018 16:54

Add test for OSD migration (different slots)

02858eb

Signed-off-by: Alexander Graul <[email protected]>

agraul force-pushed the wip-sorting-proposal branch from d18ac8f to 02858eb Compare August 27, 2018 14:54

jschmid1 merged commit 549caff into master Aug 27, 2018

agraul mentioned this pull request Aug 27, 2018

Fix and extend tests for OSD Migration #1324

Closed

jschmid1 mentioned this pull request Sep 6, 2018

remove.osd and replace.osd interfere [tracker][wip] #1357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSD Migration for changed disk slots #1302

OSD Migration for changed disk slots #1302

agraul commented Aug 23, 2018 •

edited by jschmid1

Loading

agraul Aug 23, 2018

agraul commented Aug 23, 2018

jan--f left a comment

jan--f Aug 23, 2018

agraul Aug 23, 2018

jan--f Aug 23, 2018

agraul Aug 23, 2018

jan--f Aug 23, 2018

agraul Aug 23, 2018

jan--f Aug 23, 2018

agraul Aug 23, 2018

jschmid1 Aug 24, 2018

jschmid1 Aug 24, 2018

jschmid1 commented Aug 24, 2018

jschmid1 commented Aug 24, 2018

jschmid1 commented Aug 24, 2018

jschmid1 commented Aug 27, 2018

agraul commented Aug 27, 2018

susebot commented Aug 27, 2018

jschmid1 commented Aug 27, 2018

OSD Migration for changed disk slots #1302

OSD Migration for changed disk slots #1302

Conversation

agraul commented Aug 23, 2018 • edited by jschmid1 Loading

Choose a reason for hiding this comment

agraul commented Aug 23, 2018

jan--f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschmid1 commented Aug 24, 2018

jschmid1 commented Aug 24, 2018

jschmid1 commented Aug 24, 2018

jschmid1 commented Aug 27, 2018

agraul commented Aug 27, 2018

susebot commented Aug 27, 2018

jschmid1 commented Aug 27, 2018

agraul commented Aug 23, 2018 •

edited by jschmid1

Loading