From 788ac9d982e69df3c2d23f1202686eefeaa407ff Mon Sep 17 00:00:00 2001
From: Gorka Eguileor <geguileo@redhat.com>
Date: Thu, 14 Jan 2016 15:35:15 +0100
Subject: [PATCH] Fix ChunkedBackupDriver _create_container

Current _create_container method in ChunkedBackupDriver is not behaving
as expected.

According to update_container_name docstring the method should return
None if the container name that comes in to the driver is to be used,
but code was using backup_default_container instead.

Also, if None is returned by update_container_name the logging will
report that it will be using None instead of the name that it will be
really using.

If update_container_name returns the same value we already have in the
DB, driver will unnecessarily update the DB.

This patch fixes all this.

Change-Id: I2aeac37f5533b6bf0c10dd6bfe45224a25f39ea6
Closes-Bug: #1534182
---
 cinder/backup/chunkeddriver.py         | 18 ++++++++++---
 cinder/tests/unit/test_backup_swift.py | 35 +++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/cinder/backup/chunkeddriver.py b/cinder/backup/chunkeddriver.py
index a617beed4..c77536109 100644
--- a/cinder/backup/chunkeddriver.py
+++ b/cinder/backup/chunkeddriver.py
@@ -159,12 +159,24 @@ class ChunkedBackupDriver(driver.BackupDriver):
         return
 
     def _create_container(self, context, backup):
-        backup.container = self.update_container_name(backup, backup.container)
+        # Container's name will be decided by the driver (returned by method
+        # update_container_name), if no change is required by the driver then
+        # we'll use the one the backup object already has, but if it doesn't
+        # have one backup_default_container will be used.
+        new_container = self.update_container_name(backup, backup.container)
+        if new_container:
+            # If the driver is not really changing the name we don't want to
+            # dirty the field in the object and save it to the DB with the same
+            # value.
+            if new_container != backup.container:
+                backup.container = new_container
+        elif backup.container is None:
+            backup.container = self.backup_default_container
+
         LOG.debug('_create_container started, container: %(container)s,'
                   'backup: %(backup_id)s.',
                   {'container': backup.container, 'backup_id': backup.id})
-        if backup.container is None:
-            backup.container = self.backup_default_container
+
         backup.save()
         self.put_container(backup.container)
         return backup.container
diff --git a/cinder/tests/unit/test_backup_swift.py b/cinder/tests/unit/test_backup_swift.py
index 9218b261a..e1ddf48dd 100644
--- a/cinder/tests/unit/test_backup_swift.py
+++ b/cinder/tests/unit/test_backup_swift.py
@@ -253,7 +253,8 @@ class BackupSwiftTestCase(test.TestCase):
         backup = objects.Backup.get_by_id(self.ctxt, 123)
         service.backup(backup, self.volume_file)
 
-    def test_backup_default_container(self):
+    @mock.patch.object(db, 'backup_update', wraps=db.backup_update)
+    def test_backup_default_container(self, backup_update_mock):
         volume_id = '9552017f-c8b9-4e4e-a876-00000053349c'
         self._create_backup_db_entry(volume_id=volume_id,
                                      container=None)
@@ -263,6 +264,38 @@ class BackupSwiftTestCase(test.TestCase):
         service.backup(backup, self.volume_file)
         backup = objects.Backup.get_by_id(self.ctxt, 123)
         self.assertEqual('volumebackups', backup['container'])
+        self.assertEqual(3, backup_update_mock.call_count)
+
+    @mock.patch.object(db, 'backup_update', wraps=db.backup_update)
+    def test_backup_db_container(self, backup_update_mock):
+        volume_id = '9552017f-c8b9-4e4e-a876-00000053349c'
+        self._create_backup_db_entry(volume_id=volume_id,
+                                     container='existing_name')
+        service = swift_dr.SwiftBackupDriver(self.ctxt)
+        self.volume_file.seek(0)
+        backup = objects.Backup.get_by_id(self.ctxt, 123)
+
+        service.backup(backup, self.volume_file)
+        backup = objects.Backup.get_by_id(self.ctxt, 123)
+        self.assertEqual('existing_name', backup['container'])
+        # Make sure we are not making a DB update when we are using the same
+        # value that's already in the DB.
+        self.assertEqual(2, backup_update_mock.call_count)
+
+    @mock.patch.object(db, 'backup_update', wraps=db.backup_update)
+    def test_backup_driver_container(self, backup_update_mock):
+        volume_id = '9552017f-c8b9-4e4e-a876-00000053349c'
+        self._create_backup_db_entry(volume_id=volume_id,
+                                     container=None)
+        service = swift_dr.SwiftBackupDriver(self.ctxt)
+        self.volume_file.seek(0)
+        backup = objects.Backup.get_by_id(self.ctxt, 123)
+        with mock.patch.object(service, 'update_container_name',
+                               return_value='driver_name'):
+            service.backup(backup, self.volume_file)
+        backup = objects.Backup.get_by_id(self.ctxt, 123)
+        self.assertEqual('driver_name', backup['container'])
+        self.assertEqual(3, backup_update_mock.call_count)
 
     @mock.patch('cinder.backup.drivers.swift.SwiftBackupDriver.'
                 '_send_progress_end')
-- 
2.45.2