From 6bb0c4fdabb98f168c530617b7c7a8f9396075fc Mon Sep 17 00:00:00 2001
From: Alexandre Arents <alexandre.arents@corp.ovh.com>
Date: Tue, 9 Jun 2020 14:17:37 +0000
Subject: [PATCH] Limit the number of concurrent snapshots

This change introduces new [DEFAULT]/max_concurrent_snapshots parameter
in order to limit by default compute resource overuse related
to snapshot.

Implements: blueprint max-concurrent-snapshots

Change-Id: I40b8caf06ed525e97e871cf381776164208461fb
---
 nova/compute/manager.py                       | 10 ++++--
 nova/conf/compute.py                          | 14 ++++++++
 nova/tests/unit/compute/test_compute_mgr.py   | 34 +++++++++++++++++++
 ...concurrent-snapshots-21a0a437dbe1044a.yaml | 12 +++++++
 4 files changed, 68 insertions(+), 2 deletions(-)
 create mode 100644 releasenotes/notes/max-concurrent-snapshots-21a0a437dbe1044a.yaml

diff --git a/nova/compute/manager.py b/nova/compute/manager.py
index a46e2e6ef0..1bc8727355 100644
--- a/nova/compute/manager.py
+++ b/nova/compute/manager.py
@@ -542,6 +542,11 @@ class ComputeManager(manager.Manager):
                 CONF.max_concurrent_builds)
         else:
             self._build_semaphore = compute_utils.UnlimitedSemaphore()
+        if CONF.max_concurrent_snapshots > 0:
+            self._snapshot_semaphore = eventlet.semaphore.Semaphore(
+                CONF.max_concurrent_snapshots)
+        else:
+            self._snapshot_semaphore = compute_utils.UnlimitedSemaphore()
         if CONF.max_concurrent_live_migrations > 0:
             self._live_migration_executor = futurist.GreenThreadPoolExecutor(
                 max_workers=CONF.max_concurrent_live_migrations)
@@ -3819,8 +3824,9 @@ class ComputeManager(manager.Manager):
                       instance=instance)
             return
 
-        self._snapshot_instance(context, image_id, instance,
-                                task_states.IMAGE_SNAPSHOT)
+        with self._snapshot_semaphore:
+            self._snapshot_instance(context, image_id, instance,
+                                    task_states.IMAGE_SNAPSHOT)
 
     def _snapshot_instance(self, context, image_id, instance,
                            expected_task_state):
diff --git a/nova/conf/compute.py b/nova/conf/compute.py
index 4656ac0df4..98ce74ec07 100644
--- a/nova/conf/compute.py
+++ b/nova/conf/compute.py
@@ -661,6 +661,20 @@ Possible Values:
 
 * 0 : treated as unlimited.
 * Any positive integer representing maximum concurrent builds.
+"""),
+    cfg.IntOpt('max_concurrent_snapshots',
+        default=5,
+        min=0,
+        help="""
+Maximum number of instance snapshot operations to run concurrently.
+This limit is enforced to prevent snapshots overwhelming the
+host/network/storage and causing failure. This value can be set per
+compute node.
+
+Possible Values:
+
+* 0 : treated as unlimited.
+* Any positive integer representing maximum concurrent snapshots.
 """),
     cfg.IntOpt('max_concurrent_live_migrations',
         default=1,
diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py
index 450bdc9bfc..4a5957fd9b 100644
--- a/nova/tests/unit/compute/test_compute_mgr.py
+++ b/nova/tests/unit/compute/test_compute_mgr.py
@@ -748,6 +748,40 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
         self.assertIsInstance(compute._build_semaphore,
                               compute_utils.UnlimitedSemaphore)
 
+    @mock.patch('nova.objects.Instance.save')
+    @mock.patch('nova.compute.manager.ComputeManager.'
+                '_snapshot_instance')
+    def _test_max_concurrent_snapshots(self, mock_si, mock_inst_save):
+
+        with mock.patch.object(self.compute,
+                               '_snapshot_semaphore') as mock_sem:
+            instance = objects.Instance(uuid=uuidutils.generate_uuid())
+            for i in (1, 2, 3):
+                self.compute.snapshot_instance(self.context,
+                                               mock.sentinel.image,
+                                               instance)
+            self.assertEqual(3, mock_sem.__enter__.call_count)
+
+    def test_max_concurrent_snapshots_limited(self):
+        self.flags(max_concurrent_snapshots=2)
+        self._test_max_concurrent_snapshots()
+
+    def test_max_concurrent_snapshots_unlimited(self):
+        self.flags(max_concurrent_snapshots=0)
+        self._test_max_concurrent_snapshots()
+
+    def test_max_concurrent_snapshots_semaphore_limited(self):
+        self.flags(max_concurrent_snapshots=123)
+        self.assertEqual(123,
+                         manager.ComputeManager()._snapshot_semaphore.balance)
+
+    def test_max_concurrent_snapshots_semaphore_unlimited(self):
+        self.flags(max_concurrent_snapshots=0)
+        compute = manager.ComputeManager()
+        self.assertEqual(0, compute._snapshot_semaphore.balance)
+        self.assertIsInstance(compute._snapshot_semaphore,
+                              compute_utils.UnlimitedSemaphore)
+
     def test_nil_out_inst_obj_host_and_node_sets_nil(self):
         instance = fake_instance.fake_instance_obj(self.context,
                                                    uuid=uuids.instance,
diff --git a/releasenotes/notes/max-concurrent-snapshots-21a0a437dbe1044a.yaml b/releasenotes/notes/max-concurrent-snapshots-21a0a437dbe1044a.yaml
new file mode 100644
index 0000000000..62aab634bb
--- /dev/null
+++ b/releasenotes/notes/max-concurrent-snapshots-21a0a437dbe1044a.yaml
@@ -0,0 +1,12 @@
+---
+features:
+  - |
+    A new configuration option, ``[DEFAULT]/max_concurrent_snapshots``,
+    has been added. This allow operator to configure maximum concurrent
+    snapshots on a compute host and prevent resource overuse related
+    to snapshot.
+upgrade:
+  - |
+    Previously, the number of concurrent snapshots was unlimited, now it is
+    limited via ``[DEFAULT]/max_concurrent_snapshots``, which currently
+    defaults to 5.