From f915b8a1beb876340f5035952c38dc5374c37415 Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Wed, 13 Jun 2018 17:15:18 +0200 Subject: [PATCH 1/4] Ensure that migration of files is commited before deleting files When moving attachments from the filestore to an object storage, the filesystem files will be deleted only after the commit, so if the transaction is rollbacked, we still have the local files for another try. --- attachment_s3/models/ir_attachment.py | 49 +++++++++++++++++---------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/attachment_s3/models/ir_attachment.py b/attachment_s3/models/ir_attachment.py index 9a4ae0e..2db77c3 100644 --- a/attachment_s3/models/ir_attachment.py +++ b/attachment_s3/models/ir_attachment.py @@ -27,6 +27,25 @@ except ImportError: _logger.debug("Cannot 'import boto'.") +def clean_fs(files): + _logger.info('cleaning old files from filestore') + for full_path in files: + if os.path.exists(full_path): + try: + os.unlink(full_path) + except OSError: + _logger.info( + "_file_delete could not unlink %s", + full_path, exc_info=True + ) + except IOError: + # Harmless and needed for race conditions + _logger.info( + "_file_delete could not unlink %s", + full_path, exc_info=True + ) + + class IrAttachment(models.Model): _inherit = "ir.attachment" @@ -271,22 +290,7 @@ class IrAttachment(models.Model): # on assets 'mimetype': self.mimetype}) _logger.info('moved %s on the object storage', fname) - full_path = self._full_path(fname) - _logger.info('cleaning fs self') - if os.path.exists(full_path): - try: - os.unlink(full_path) - except OSError: - _logger.info( - "_file_delete could not unlink %s", - full_path, exc_info=True - ) - except IOError: - # Harmless and needed for race conditions - _logger.info( - "_file_delete could not unlink %s", - full_path, exc_info=True - ) + return self._full_path(fname) elif self.db_datas: _logger.info('moving on the object storage from database') self.write({'datas': self.datas}) @@ -313,6 +317,7 @@ class IrAttachment(models.Model): with self.do_in_new_env(new_cr=new_cr) as new_env: attachment_model_env = new_env['ir.attachment'] ids = attachment_model_env.search(domain).ids + files_to_clean = [] for attachment_id in ids: try: with new_env.cr.savepoint(): @@ -333,11 +338,21 @@ class IrAttachment(models.Model): # attachments on each loop. new_env.clear() attachment = attachment_model_env.browse(attachment_id) - attachment._move_attachment_to_s3() + path = attachment._move_attachment_to_s3() + if path: + files_to_clean.append(path) except psycopg2.OperationalError: _logger.error('Could not migrate attachment %s to S3', attachment_id) + def clean(): + clean_fs(files_to_clean) + + # delete the files from the filesystem once we know the changes + # have been committed in ir.attachment + if files_to_clean: + new_env.cr.after('commit', clean) + @contextmanager def do_in_new_env(self, new_cr=False): """ Context manager that yields a new environment From ddb7656abd4169a81923ee7c0bd0d550a19a0cd6 Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Wed, 13 Jun 2018 17:15:45 +0200 Subject: [PATCH 2/4] Fix attachments stored in FS instead of object storage Assume the following situation: * We have installed addons base, sale and attachment_s3 (hence base_attachment_object_storage as dependency) * All attachments are in S3 already * We run an upgrade of the 'base' addon, 'sale' is upgraded before attachment_s3 in the order of loading. * Sale updates the icon of the Sale menu * As attachment_s3 is not loaded yet, the attachment is created in the filestore Now if we don't persist the filestore or use different servers, we'll lose the images of the menus (or any attachment loaded by the install/upgrade of an addon). The implemented solution is to move the attachments from the filestore to the object storage at the loading of the module. However, this operation can take time and it shouldn't be run by 2 processes at the same time, so we want to detect if the module is loaded during a normal odoo startup or when some addons have been upgraded. There is nothing anymore at this point which allow us to know that modules just have been upgraded except... in the caller frame (load_modules). We have to rely on the inpect module and get the caller frame, which is not recommended, but seems the only way, besides, it's not called often and if _register_hook was called from another place, it would have no effect (unless the other place has a variable 'update_module' too). --- attachment_s3/models/ir_attachment.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/attachment_s3/models/ir_attachment.py b/attachment_s3/models/ir_attachment.py index 2db77c3..272e78f 100644 --- a/attachment_s3/models/ir_attachment.py +++ b/attachment_s3/models/ir_attachment.py @@ -4,6 +4,7 @@ import base64 +import inspect import logging import os import xml.dom.minidom @@ -91,6 +92,31 @@ class IrAttachment(models.Model): continue self._data_set('datas', attach.datas, None) + def _register_hook(self, cr): + super(IrAttachment, self)._register_hook(cr) + curframe = inspect.currentframe() + calframe = inspect.getouterframes(curframe, 2) + # the caller of _register_hook is 'load_modules' in + # odoo/modules/loading.py + # We have to go up 2 stacks because of the old api wrapper + load_modules_frame = calframe[2][0] + # 'update_module' is an argument that 'load_modules' receives with a + # True-ish value meaning that an install or upgrade of addon has been + # done during the initialization. We need to move the attachments that + # could have been created or updated in other addons before this addon + # was loaded + update_module = load_modules_frame.f_locals.get('update_module') + + # We need to call the migration on the loading of the model because + # when we are upgrading addons, some of them might add attachments. + # To be sure they are migrated to the storage we need to call the + # migration here. + # Typical example is images of ir.ui.menu which are updated in + # ir.attachment at every upgrade of the addons + if update_module: + env = api.Environment(cr, openerp.SUPERUSER_ID, {}) + env['ir.attachment']._force_storage_s3() + @api.multi def _store_in_db_when_s3(self): """ Return whether an attachment must be stored in db From c3d9aceb0fa31e805bb4aa4992be14ec33a142cd Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Wed, 13 Jun 2018 17:16:07 +0200 Subject: [PATCH 3/4] Document a weird domain which is there for a reason The reason being: https://github.com/odoo/odoo/blob/9032617120138848c63b3cfa5d1913c5e5ad76db/odoo/addons/base/ir/ir_attachment.py#L344-L347 I nearly deleted this domain but it was too weird to be there for no reason. A comment explaining the issue was really missing. --- attachment_s3/models/ir_attachment.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/attachment_s3/models/ir_attachment.py b/attachment_s3/models/ir_attachment.py index 272e78f..2fe55a4 100644 --- a/attachment_s3/models/ir_attachment.py +++ b/attachment_s3/models/ir_attachment.py @@ -332,6 +332,11 @@ class IrAttachment(models.Model): if storage != 's3': return _logger.info('migrating files to the object storage') + # The weird "res_field = False OR res_field != False" domain + # is required! It's because of an override of _search in ir.attachment + # which adds ('res_field', '=', False) when the domain does not + # contain 'res_field'. + # https://github.com/odoo/odoo/blob/9032617120138848c63b3cfa5d1913c5e5ad76db/odoo/addons/base/ir/ir_attachment.py#L344-L347 domain = ['!', ('store_fname', '=like', 's3://%'), '|', ('res_field', '=', False), From d1bb60fc114632eb053c82013d72c5b3bf607653 Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Wed, 13 Jun 2018 17:26:08 +0200 Subject: [PATCH 4/4] attachment_s3: bump 1.3.0 --- attachment_s3/__openerp__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/attachment_s3/__openerp__.py b/attachment_s3/__openerp__.py index a0971ff..e938f93 100644 --- a/attachment_s3/__openerp__.py +++ b/attachment_s3/__openerp__.py @@ -5,7 +5,7 @@ {'name': 'Attachments on S3 storage', 'summary': 'Store assets and attachments on a S3 compatible object storage', - 'version': '9.0.1.2.0', + 'version': '9.0.1.3.0', 'author': 'Camptocamp,Odoo Community Association (OCA)', 'license': 'AGPL-3', 'category': 'Knowledge Management',