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

[Royce Lv]

On 2014年09月22日 23:27, Aline Manera wrote:
>
> 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(...)
Good point
>
>> def removeRepo(self, repo_id):
>> """
>