[Kimchi-devel] [PATCHv2 3/3] Fix: Add rollback to update repository

[Aline Manera]

On 09/22/2014 06:32 AM, lvroyce at linux.vnet.ibm.com wrote:
> From: Royce Lv <lvroyce at linux.vnet.ibm.com>
>
> When update repository, we remove a old repository first then add
> a new one, if error occurs when adding new one, old one was removed
> which will cause problem.
>
> Fix by recovering removed old repository after failure of adding new one.
>
> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
> ---
>   src/kimchi/repositories.py | 32 ++++++++++++++++++--------------
>   1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/src/kimchi/repositories.py b/src/kimchi/repositories.py
> index 3ac0e9c..9d665d5 100644
> --- a/src/kimchi/repositories.py
> +++ b/src/kimchi/repositories.py
> @@ -17,6 +17,7 @@
>   # License along with this library; if not, write to the Free Software
>   # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
>
> +import copy
>   import os
>   import time
>   import urlparse
> @@ -29,7 +30,6 @@ from kimchi.exception import InvalidOperation, InvalidParameter
>   from kimchi.exception import OperationFailed, NotFoundError, MissingParameter
>   from kimchi.utils import validate_repo_url
>
> -
>   class Repositories(object):
>       __metaclass__ = Singleton
>
> @@ -446,7 +446,6 @@ class AptRepo(object):
>               kimchiLock.release()
>               raise OperationFailed("KCHREPOS0026E", {'err': e.message})
>           kimchiLock.release()
> -
>           return self._get_repo_id(source_entry)
>
>       def toggleRepo(self, repo_id, enable):
> @@ -490,22 +489,27 @@ class AptRepo(object):
>           """
>           Update a given repository in repositories.Repositories() format
>           """
> -        r = self._get_source_entry(repo_id)
> -        if r is None:
> -            raise NotFoundError("KCHREPOS0012E", {'repo_id': repo_id})
> -
> -        info = {'enabled': not r.disabled,
> -                'baseurl': params.get('baseurl', r.uri),
> -                'config': {'type': 'deb', 'dist': r.dist,
> -                           'comps': r.comps}}
> +        old_info = self.getRepo(repo_id)
> +        updated_info = copy.deepcopy(old_info)
> +        updated_info['baseurl'] = params.get('baseurl', updated_info['baseurl'])
>
>           if 'config' in params.keys():
>               config = params['config']
> -            info['config']['dist'] = config.get('dist', r.dist)
> -            info['config']['comps'] = config.get('comps', r.comps)
> +            updated_info['config']['dist'] = config.get('dist', r.dist)
> +            updated_info['config']['comps'] = config.get('comps', r.comps)
>
> -        self.removeRepo(repo_id)
> -        return self.addRepo(info)

> +        try:
> +            new_id = None
> +            self.removeRepo(repo_id)
> +            removed = True
> +            new_id = self.addRepo(updated_info)
> +            return new_id

> +        except:
> +            if removed:
> +                self.addRepo(old_info)

If an exception occurs on self.removeRepo() the 'removed' variable will 
be undefined

> +            if new_id:
> +                self.removeRepo(new_id)

What is the case to remove the updated repo?
if 'new_id' has a value the update occurs with success.

> +            raise

What about?

# remove repo may not be in the try/except block as we want to raise the 
error if any occurs
self.removeRepo(repo_id)

try:
     self.addRepo(updated_info)
except:
     self.addRepo(old_info)
     raise OperationFailed(...)

>       def removeRepo(self, repo_id):
>           """