From 896aec5976e1db3a013bd1115b47610b89b30302 Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Wed, 13 Jun 2018 15:59:16 +0200 Subject: [PATCH 1/3] 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. --- .../models/ir_attachment.py | 51 ++++++++++++------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/base_attachment_object_storage/models/ir_attachment.py b/base_attachment_object_storage/models/ir_attachment.py index 9063cc6..22899b5 100644 --- a/base_attachment_object_storage/models/ir_attachment.py +++ b/base_attachment_object_storage/models/ir_attachment.py @@ -15,6 +15,25 @@ from odoo import api, exceptions, models, _ _logger = logging.getLogger(__name__) +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' @@ -177,22 +196,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}) @@ -222,11 +226,12 @@ class IrAttachment(models.Model): with self.do_in_new_env(new_cr=new_cr) as new_env: model_env = new_env['ir.attachment'] ids = model_env.search(domain).ids + files_to_clean = [] for attachment_id in ids: try: with new_env.cr.savepoint(): # check that no other transaction has - # locked the row, don't send a file to S3 + # locked the row, don't send a file to storage # in that case self.env.cr.execute("SELECT id " "FROM ir_attachment " @@ -242,11 +247,21 @@ class IrAttachment(models.Model): # ALL the attachments on each loop. new_env.clear() attachment = model_env.browse(attachment_id) - attachment._move_attachment_to_store() + path = attachment._move_attachment_to_store() + 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) + def _get_stores(self): """ To get the list of stores activated in the system """ return [] From 5d2fdeafb1af81e3046f3c80a860ebc61a852699 Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Wed, 13 Jun 2018 16:00:57 +0200 Subject: [PATCH 2/3] 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). --- .../models/ir_attachment.py | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/base_attachment_object_storage/models/ir_attachment.py b/base_attachment_object_storage/models/ir_attachment.py index 22899b5..a801a98 100644 --- a/base_attachment_object_storage/models/ir_attachment.py +++ b/base_attachment_object_storage/models/ir_attachment.py @@ -2,7 +2,7 @@ # Copyright 2017 Camptocamp SA # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html) - +import inspect import logging import os import psycopg2 @@ -39,6 +39,33 @@ class IrAttachment(models.Model): _local_fields = ('image_small', 'image_medium', 'web_icon_data') + @api.cr + def _register_hook(self): + super(IrAttachment, self)._register_hook() + # ignore if we are not using an object storage + if self._storage() not in self._get_stores(): + return + curframe = inspect.currentframe() + calframe = inspect.getouterframes(curframe, 2) + # the caller of _register_hook is 'load_modules' in + # odoo/modules/loading.py + load_modules_frame = calframe[1][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: + self.env['ir.attachment'].sudo()._force_storage_to_object_storage() + @api.multi def _save_in_db_anyway(self): """ Return whether an attachment must be stored in db From 5039499b5be2f7e5c83625bdc9feeac2f0d91381 Mon Sep 17 00:00:00 2001 From: Guewen Baconnier Date: Wed, 13 Jun 2018 16:01:22 +0200 Subject: [PATCH 3/3] 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. --- base_attachment_object_storage/models/ir_attachment.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/base_attachment_object_storage/models/ir_attachment.py b/base_attachment_object_storage/models/ir_attachment.py index a801a98..319e2bf 100644 --- a/base_attachment_object_storage/models/ir_attachment.py +++ b/base_attachment_object_storage/models/ir_attachment.py @@ -242,6 +242,11 @@ class IrAttachment(models.Model): def _force_storage_to_object_storage(self, new_cr=False): _logger.info('migrating files to the object storage') storage = self._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', '{}://%'.format(storage)), '|', ('res_field', '=', False),