From 3c7a71e60778d721074ca307a641aba0e8555d02 Mon Sep 17 00:00:00 2001
From: Gorka Eguileor <geguileo@redhat.com>
Date: Tue, 7 Jun 2016 11:14:51 +0200
Subject: [PATCH] Add error messages to conditional updates devref

There was no mention in conditional updates devref on what kind of error
messages are we expected to return with conditional updates and why we
decided to go with generic errors instead of specific errors like we had
before.

Change-Id: I079a815a656ce5a5c0e05e2e20f26df6ea700547
---
 doc/source/devref/api_conditional_updates.rst | 71 +++++++++++++++++--
 1 file changed, 66 insertions(+), 5 deletions(-)

diff --git a/doc/source/devref/api_conditional_updates.rst b/doc/source/devref/api_conditional_updates.rst
index b77afd2bcd5..3c62ec5d480 100644
--- a/doc/source/devref/api_conditional_updates.rst
+++ b/doc/source/devref/api_conditional_updates.rst
@@ -158,7 +158,7 @@ Basic Usage
          'consistencygroup_id': None
      }
 
-      volume.conditional_update(values, expected_values)
+     volume.conditional_update(values, expected_values)
 
 - **Filters**
 
@@ -179,7 +179,7 @@ Basic Usage
 
   .. code:: python
 
-    def volume_has_snapshots_filter():volume_has_snapshots_filter
+    def volume_has_snapshots_filter():
         return IMPL.volume_has_snapshots_filter()
 
   And finally used in the API (notice how we are negating the filter at the
@@ -199,6 +199,67 @@ Basic Usage
 
      volume.conditional_update(values, expected_values, filters)
 
+Returning Errors
+----------------
+
+The most important downside of using conditional updates to remove API races is
+the inherent uncertainty of the cause of failure resulting in more generic
+error messages.
+
+When we use the `conditional_update` method we'll use returned value to
+determine the success of the operation, as a value of 0 indicates that no rows
+have been updated and the conditions were not met.  But we don't know which
+one, or which ones, were the cause of the failure.
+
+There are 2 approaches to this issue:
+
+- On failure we go one by one checking the conditions and return the first one
+  that fails.
+
+- We return a generic error message indicating all conditions that must be met
+  for the operation to succeed.
+
+It was decided that we would go with the second approach, because even though
+the first approach was closer to what we already had and would give a better
+user experience, it had considerable implications such as:
+
+- More code was needed to do individual checks making operations considerable
+  longer and less readable.  This was greatly alleviated using helper methods
+  to return the errors.
+
+- Higher number of DB queries required to determine failure cause.
+
+- Since there could be races because DB contents could be changed between the
+  failed update and the follow up queries that checked the values for the
+  specific error, a loop would be needed to make sure that either the
+  conditional update succeeds or one of the condition checks fails.
+
+- Having such a loop means that a small error in the code could lead to an
+  endless loop in a production environment.  This coding error could be an
+  incorrect conditional update filter that would always fail or a missing or
+  incorrect condition that checked for the specific issue to return the error.
+
+A simple example of a generic error can be found in `begin_detaching` code:
+
+.. code:: python
+
+   @wrap_check_policy
+   def begin_detaching(self, context, volume):
+       # If we are in the middle of a volume migration, we don't want the
+       # user to see that the volume is 'detaching'. Having
+       # 'migration_status' set will have the same effect internally.
+       expected = {'status': 'in-use',
+                   'attach_status': 'attached',
+                   'migration_status': self.AVAILABLE_MIGRATION_STATUS}
+
+       result = volume.conditional_update({'status': 'detaching'}, expected)
+
+       if not (result or self._is_volume_migrating(volume)):
+           msg = _("Unable to detach volume. Volume status must be 'in-use' "
+                   "and attach_status must be 'attached' to detach.")
+           LOG.error(msg)
+           raise exception.InvalidVolume(reason=msg)
+
 Building filters on the API
 ---------------------------
 
@@ -374,7 +435,7 @@ Code would look like this:
        return sql.exists().where(and_(
            ~model.deleted,
            model.status == 'creating',
-           conditions.append(model.source_cgid == cg_id))
+           conditions.append(model.source_cgid == cg_id)))
 
 While this will work in SQLite and PostgreSQL, it will not work on MySQL and an
 error will be raised when the query is executed: "You can't specify target
@@ -382,8 +443,8 @@ table 'consistencygroups' for update in FROM clause".
 
 To solve this we have 2 options:
 
-- Create a specific query for MySQL using a feature only available in MySQL,
-  which is an update with a left self join.
+- Create a specific query for MySQL engines using an update with a left self
+  join, which is a feature only available in MySQL.
 - Use a trick -using a select subquery- that will work on all DBs.
 
 Considering that it's always better to have only 1 way of doing things and that