From 87fb94cfe78d1881a68f015623a9a2531de82a0c Mon Sep 17 00:00:00 2001
From: Jan Fajerski <jfajerski@suse.com>
Date: Tue, 31 Mar 2020 16:07:45 +0200
Subject: [PATCH] ceph-volume: add and delete lvm tags in a single lvchange
 call.

Otherwise we can end up in race-y situations when a concurrent c-v calls
sees only one tag but expects all tags to be present. Say if the
ceph.type tag is present, c-v expects ceph.osd_id to be present. By
setting/deleting tags in bulk, we use lvchange (and lvms internal
locking) as a sync mechanism.

Fixes: https://tracker.ceph.com/issues/44852

Signed-off-by: Jan Fajerski <jfajerski@suse.com>
(cherry picked from commit 20ecc309371e53fda5d6a5b6cf6de6110dbe5497)
---
 src/ceph-volume/ceph_volume/api/lvm.py        | 29 ++++++++++++++----
 .../ceph_volume/tests/api/test_lvm.py         | 30 +++++++++----------
 2 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/src/ceph-volume/ceph_volume/api/lvm.py b/src/ceph-volume/ceph_volume/api/lvm.py
index b8932f38d4df6..ffe2ad7aa0b82 100644
--- a/src/ceph-volume/ceph_volume/api/lvm.py
+++ b/src/ceph-volume/ceph_volume/api/lvm.py
@@ -6,6 +6,7 @@ set of utilities for interacting with LVM.
 import logging
 import os
 import uuid
+from itertools import repeat
 from math import floor
 from ceph_volume import process, util
 from ceph_volume.exceptions import (
@@ -952,12 +953,27 @@ class Volume(object):
             report[type_uuid] = self.tags['ceph.{}'.format(type_uuid)]
             return report
 
-    def clear_tags(self):
+    def _format_tag_args(self, op, tags):
+        tag_args = ['{}={}'.format(k, v) for k, v in tags.items()]
+        # weird but efficient way of ziping two lists and getting a flat list
+        return list(sum(zip(repeat(op), tag_args), ()))
+
+    def clear_tags(self, keys=None):
         """
-        Removes all tags from the Logical Volume.
+        Removes all or passed tags from the Logical Volume.
         """
-        for k in list(self.tags):
-            self.clear_tag(k)
+        if not keys:
+            keys = self.tags.keys()
+
+        del_tags = {k: self.tags[k] for k in keys if k in self.tags}
+        if not del_tags:
+            # nothing to clear
+            return
+        del_tag_args = self._format_tag_args('--deltag', del_tags)
+        # --deltag returns successful even if the to be deleted tag is not set
+        process.call(['lvchange'] + del_tag_args + [self.lv_path])
+        for k in del_tags.keys():
+            del self.tags[k]
 
 
     def set_tags(self, tags):
@@ -972,8 +988,11 @@ class Volume(object):
         At the end of all modifications, the tags are refreshed to reflect
         LVM's most current view.
         """
+        self.clear_tags(tags.keys())
+        add_tag_args = self._format_tag_args('--addtag', tags)
+        process.call(['lvchange'] + add_tag_args + [self.lv_path])
         for k, v in tags.items():
-            self.set_tag(k, v)
+            self.tags[k] = v
 
 
     def clear_tag(self, key):
diff --git a/src/ceph-volume/ceph_volume/tests/api/test_lvm.py b/src/ceph-volume/ceph_volume/tests/api/test_lvm.py
index 4270a43b1ea6d..363fa10734e2a 100644
--- a/src/ceph-volume/ceph_volume/tests/api/test_lvm.py
+++ b/src/ceph-volume/ceph_volume/tests/api/test_lvm.py
@@ -670,18 +670,16 @@ class TestTags(object):
         assert self.foo_volume.tags == tags
 
         expected = [
-            ['lvchange', '--deltag', 'ceph.foo0=bar0', '/path'],
-            ['lvchange', '--addtag', 'ceph.foo0=bar0', '/path'],
-            ['lvchange', '--deltag', 'ceph.foo1=bar1', '/path'],
-            ['lvchange', '--addtag', 'ceph.foo1=baz1', '/path'],
-            ['lvchange', '--deltag', 'ceph.foo2=bar2', '/path'],
-            ['lvchange', '--addtag', 'ceph.foo2=baz2', '/path'],
-            ['lvchange', '--deltag', 'ceph.foo1=baz1', '/path'],
-            ['lvchange', '--addtag', 'ceph.foo1=other1', '/path'],
+            sorted(['lvchange', '--deltag', 'ceph.foo0=bar0', '--deltag',
+                    'ceph.foo1=bar1', '--deltag', 'ceph.foo2=bar2', '/path']),
+            sorted(['lvchange', '--deltag', 'ceph.foo1=baz1', '/path']),
+            sorted(['lvchange', '--addtag', 'ceph.foo0=bar0', '--addtag',
+                    'ceph.foo1=baz1', '--addtag', 'ceph.foo2=baz2', '/path']),
+            sorted(['lvchange', '--addtag', 'ceph.foo1=other1', '/path']),
         ]
         # The order isn't guaranted
         for call in capture.calls:
-            assert call['args'][0] in expected
+            assert sorted(call['args'][0]) in expected
         assert len(capture.calls) == len(expected)
 
     def test_clear_tags(self, monkeypatch, capture):
@@ -695,16 +693,16 @@ class TestTags(object):
         assert self.foo_volume_clean.tags == {}
 
         expected = [
-            ['lvchange', '--addtag', 'ceph.foo0=bar0', '/pathclean'],
-            ['lvchange', '--addtag', 'ceph.foo1=bar1', '/pathclean'],
-            ['lvchange', '--addtag', 'ceph.foo2=bar2', '/pathclean'],
-            ['lvchange', '--deltag', 'ceph.foo0=bar0', '/pathclean'],
-            ['lvchange', '--deltag', 'ceph.foo1=bar1', '/pathclean'],
-            ['lvchange', '--deltag', 'ceph.foo2=bar2', '/pathclean'],
+            sorted(['lvchange', '--addtag', 'ceph.foo0=bar0', '--addtag',
+                    'ceph.foo1=bar1', '--addtag', 'ceph.foo2=bar2',
+                    '/pathclean']),
+            sorted(['lvchange', '--deltag', 'ceph.foo0=bar0', '--deltag',
+                    'ceph.foo1=bar1', '--deltag', 'ceph.foo2=bar2',
+                    '/pathclean']),
         ]
         # The order isn't guaranted
         for call in capture.calls:
-            assert call['args'][0] in expected
+            assert sorted(call['args'][0]) in expected
         assert len(capture.calls) == len(expected)
 
 
-- 
2.39.5