[9.0] add module_checksum_upgrade and refactor module_auto_update by sbidoul · Pull Request #1176 · OCA/server-tools

sbidoul · 2018-03-01T11:16:52Z

Following the very interesting conversation in #1151 I've had a deeper look into module_auto_update. The idea is indeed brilliant. However I came to the conclusion that module_auto_update is not adequate for our needs for the following reasons:

it does too many things automatically and is intrusive in the regular Odoo upgrade process
in v9, it goes into autocommit mode during the upgrade process
once installed, it cannot be uninstalled (tested in v9), due to a deadlock when removing the stored field on ir.module.module

In general by looking at the history of module_auto_update, we can see that it started with a simple and elegant idea, but the resulting implementation taking into account of the edge cases has become very complex and difficult to understand and looks fragile. I believe this comes from the fact that it tries to immediately update the installed hash whatever the mean of installing a module or running the upgrade.

So I came up with this new module module_checksum_upgrade that provides the basic scriptable building blocks (ie computing hashes, and saving them, and upgrading the changed modules only).

The approach is a little bit different from module_auto_update as it saves the checksums in a ir.config_parameter instead as a field on the module (this is necessary for the uninstall to work, at least in 9).

We intend to port this module from 8 to 11.

If there is interest, it is easy to adapt module_auto_update to depend on this new module, therefore fixing the uninstall issue in module_auto_update, and avoiding code duplication. I've a commit ready that does just that.

legalsylvain · 2018-03-01T11:27:20Z

+# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl).
+
+{
+    'name': 'Module Auto Update Base',


name and summary does'nt seems to fit with the objective of the module. don't you think ?

@legalsylvain I fixed the name. How would you suggest to change the summary?

sebalix · 2018-03-01T11:27:49Z

+
+    def test_saved_checksums(self):
+        Imm = self.env['ir.module.module']
+        base_module = self.env['ir.module.module'].search([


You can use Imm

@sebalix indeed. Done.

pedrobaeza · 2018-03-01T11:41:00Z

The problem with this is that in OCA we can't have several things for the same goal. We should work to have a common ground, not to duplicate modules.

sbidoul · 2018-03-01T11:56:29Z

@pedrobaeza that is why I propose to make module_auto_update depend on this one, so there is no code duplication. This would look like this: acsone@63b5fc9.

This way people can choose to use module_checksum_upgrade and script it, or have the automatic module_auto_update.

pedrobaeza · 2018-03-01T12:04:59Z

But I don't want to have 2 modules for performing the same that I'm doing now with one. Integrate your solution in the other.

sbidoul · 2018-03-01T12:11:12Z

I don't see how to integrate them without breaking backward compatibility. The point is that we don't want the auto part of module_auto_update.

What is the problem of having one module that provides a base functionality, and another one that extends it and make it automatic?

pedrobaeza · 2018-03-01T12:13:44Z

The problem is that I'm rising my complexity, number of modules and so on for no benefit in my approach. The auto part in module_auto_update can be disabled if you want or making parametric.

sbidoul · 2018-03-01T12:15:11Z

Let's see what other say. I can maintain this outside of OCA. That would be a pity.

sbidoul · 2018-03-01T13:21:26Z

Regarding the complexity argument, I argue that a two modules approach is less complex and much cleaner than littering module_auto_update with tests on a parameter that completely disables it's function.

andreparames · 2018-03-01T13:27:46Z

@@ -0,0 +1,6 @@
+This directory contains eggs that were downloaded by setuptools to build, test, and run plug-ins.


Is this file needed?

Oops. Well spotted. Removed.

andreparames · 2018-03-01T13:33:17Z

The code LGTM 👍

ThomasBinsfeld

(Nitpicking)
Code and functional reviews 👍

ThomasBinsfeld · 2018-03-01T12:30:25Z

+===========
+
+Bugs are tracked on `GitHub Issues
+<https://github.com/OCA/{project_repo}/issues>`_. In case of trouble, please


@sbidoul s/{project_repo}/server-tools

ThomasBinsfeld · 2018-03-01T12:41:01Z

+        ], limit=1))
+
+    @api.model
+    def _modules_with_changed_checksum(self):


@sbidoul s/_modules_with_changed_checksum/_get_modules_with_changed_checksum?

@ThomasBinsfeld, done!

pedrobaeza · 2018-03-01T14:32:01Z

Another thing to have in mind is that currently the other module doesn't work in v9. For v10 and v11 it does. You have to check that ones.

sbidoul · 2018-03-01T15:07:16Z

I looked at the code of 10 and 11 before coding this. It is very similar and does not change my reasoning.

yajo

I like this new design.

To be honest, at the end of the day we were not using the cron because of its problems. Just manual upgrading. Yikes! odoo is so anti-HA by design... 🤦‍♂️

Well, I added some comments, and before we continue I think we really need to test the upgrade_changed_checksum method. As you said, we've found so many corner cases that I really feel we need that, please.

About having 2 addons for the same thing... That part I don't like it TBH. This could become an upgrade to the original one.

I'd appreciate having some UI exposing this to the user too. Not a blocker.

yajo · 2018-03-07T11:41:13Z

+               'automatically',
+    'version': '9.0.1.0.0',
+    'category': 'Extra Tools',
+    'website': 'https://odoo-community.org/',


server-tools

yajo · 2018-03-07T11:41:24Z

+        'base',
+    ],
+    'data': [
+    ],


yajo · 2018-03-07T11:41:30Z

+Installation
+============
+
+There are no specific installation instructions.


yajo · 2018-03-07T11:41:34Z

+Configuration
+=============
+
+There is nothing to configure.


yajo · 2018-03-07T11:42:11Z

+This is by design, priviledging safety. Should this be an issue,
+the method ``_save_installed_checksums`` can be invoked in a situation
+where one is sure all modules on disk are installed and up-to-date in the
+database.


How can you call the method before installing the addon? 🤔

You don't. Installing this module does nothing until upgrade_changed_checksum is called.

Oh sorry I understood bad; it's OK then

yajo · 2018-03-07T11:47:45Z

+
+        _logger.info("Upgrade successful, updating checksums...")
+        self._save_installed_checksums()
+        self.env.cr.commit()  # pylint: disable=invalid-commit


Could you explain why the commits instead of savepoints? I'm pretty sure it makes sense, but a comment would be quite useful. 😉

What would be the benefit of savepoints?

Savepoints produce the same effect as a commit, but just for the current cursor. At the end, the cursor is committed automatically by Odoo following its usual rules.

Please read this section: https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#never-commit-the-transaction

Savepoint are not the same as commits: https://www.postgresql.org/docs/9.6/static/sql-savepoint.html.

I'm aware of the risks of commits in normal business logic. Here I really want to commit at each step (to be sure also to have the same behaviour with each Odoo version). There is no risk of corruption because this method is not intended to be invoked as part of a larger transaction. I'll update the docstring on this topic.

Docstring updated

yajo · 2018-03-07T11:51:33Z

+        """It should exclude .pyc/.pyo extensions from checksum
+        calculations"""
+        if not self.own_writeable:
+            self.skipTest("Own directory not writeable")


The original addon test does it differently.

Instead, it writes a false checksum before checking the upgrade behavior:

server-tools/module_auto_update/tests/test_module_upgrade.py

Line 24 in 83bce19

self.own_module.checksum_installed = 'test'

It makes addon testable everywhere; please do that instead. 😊

This particular test comes unchanged from the original module:

server-tools/module_auto_update/tests/test_module.py

Lines 58 to 59 in 3ed2a55

if not self.own_writeable:

self.skipTest("Own directory not writeable")

yajo · 2018-03-07T11:51:50Z

+        """It should return a different value when a non-.pyc/.pyo file is
+        added to the module directory"""
+        if not self.own_writeable:
+            self.skipTest("Own directory not writeable")


yajo · 2018-03-07T11:51:59Z

+            'sha1',
+            excluded_extensions=['pyc', 'pyo'],
+        )
+        self.own_writeable = os.access(self.own_dir_path, os.W_OK)


Not needed, see below

yajo · 2018-03-07T12:02:04Z

+
+        excluded_extensions = self.env["ir.config_parameter"].get_param(
+            PARAM_EXCLUDED_EXTENSIONS,
+            'pyc,pyo',


could we add .pot too?

Yes, makes sense. Would be nice to have a mechanism to exclude/include languages too.

pedrobaeza · 2018-03-07T12:08:11Z

@yajo @sbidoul that's what I'm saying since beginning: if there's something incorrect in current module, let's change it, but not to create an alternative module.

sbidoul · 2018-03-07T12:57:55Z

@yajo thanks for the detailed review.

sbidoul · 2018-03-07T14:22:33Z

Regarding the duplicate, may I suggest deprecating module_auto_update. Again I don't see the benefit of upgrading it. Fixing while keeping it backward compatible is impossible.

Actuallly, I'm more and more convinced module_auto_update is dangerous. If you update the code then run the upgrade without restarting the server, I can imagine situations where a migration runs with a previous version of the code in memory.

pedrobaeza · 2018-03-07T18:50:52Z

@sbidoul please tell me which compatibility do you break doing this on the existing module. I think that can be reasonable if the new method is better as said.

sbidoul · 2018-03-07T20:24:40Z

@pedrobaeza

which compatibility do you break doing this on the existing module

As pointed out #1151 a restart is needed to have the new python code. Since python code maybe needed in upgrades involving migrations scripts, the risk is too high to run an incorrect upgrade.

So I want to get rid of everything that is potentially unsafe in the existing module.

the cron
the UI that triggers the checksum upgrade
all overrides of the standard Odoo methods
the mechanisme to store checksums (the fields added on ir.module.module prevent uninstall)

To sum up: everything. Except the fundamental idea which is brilliant as I said, but needs to be implemented differently to be safe.

yajo · 2018-03-08T08:36:36Z

To sum up: everything. Except the fundamental idea which is brilliant as I said, but needs to be implemented differently to be safe.

Lol, quite right! 😆 OK, let me see if we can get to an agreement 😊

the cron

The cron job is currently buggy, as you specified, because it doesn't restart the server.

However it's also true that some people might find useful run unattended upgrades. Just upgrade code and, when you know the upgrade is not dangerous, keep it running until it gets upgraded. In fact, that was my initial thought, although we "rolled back" the idea internally due to the obvious problems it has.

So we have 2 paths:

Delete the cron data. It's buggy, and it's buggy by design, so we drop it.
Disable the cron by default. It's under a noupdate=1 block, so old installs will keep it as it was, and new ones will have it disabled.

I think option 1 is better. It's a breaking change, but after all it wasn't working fine in the first place. I'd merge that happily.

the UI that triggers the checksum upgrade

all overrides of the standard Odoo methods

The real problem is not having a UI, but instead:

The UI is not obvious, because it reuses standard Odoo methods.
The UI doesn't work perfectly fine because it doesn't force a server restart.

However, overriding the standard Odoo methods has its advantage: it forces consistency. If you're installing or upgrading an addon, it makes sure all other addons are currently up to date before doing that. That's a good thing IMHO, although in some situations it seems a little bit excessive. For instance, if you're installing a new addon and have to upgrade all others, it could take quite a bit of time.

Also, providing an UI also empowers less technical users to use this good feature. Flattening the learn curve is always a good thing.

Then the best solution would be:

Yes, we provide a wizard, but it's explicit and obvious.
Yes, we override standard Odoo methods, but do not do that by default. Just a system parameter that enables or not the feature. The same autoupdate wizard should have a checkbox that allows the sysadmin to force module consistency when performing module changes that should enable the standard odoo methods overriding.
We restart the server after upgrades. Odoo does so when hitting any upgrade button, so there must be a way to do it.

the mechanisme to store checksums (the fields added on ir.module.module prevent uninstall)

This is also true, although it's something that can be easily achieved with a premigration script.

So, after looking at this with perspective, I agree with both:

@sbidoul, you're right: the design needs to change dramatically.
@pedrobaeza, you're also right: the module should be upgraded instead of adding a new one.

@sbidoul In case you were worried about breaking changes not being merged, don't worry, I won't block those if they make sense. 😉

So, do we all agree now?

pedrobaeza · 2018-03-08T08:42:52Z

Yes, of course.

sbidoul · 2018-03-08T11:21:04Z

@pedrobaeza @yajo I think you missed my main point.

What I say is that the server must be restarted BEFORE the upgrade, not after. Indeed, when a migration runs, it may invoke methods that already loaded in memory and therefore can potentially run with the previous version of the code.

That is why I say this method must not have a UI.

Moreover, if we agree the current module is broken in subtle ways, and we don't need a UI why taking the burden of maintaining it?

pedrobaeza · 2018-03-08T11:24:30Z

What I don't want is to change the module name to ease the transition to this new method. That's why I insist to change current module.

yajo · 2018-03-08T12:03:19Z

Yes, @sbidoul, I didn't get that point 😉

However, I also think you didn't get ours: you're right, the addon was broken, so go on and break backwards compatibility, we won't block that. We don't want to maintain a broken addon, but we also don't want to maintain 2; it's just that.

yajo · 2018-03-14T11:44:42Z

Of course it seems not much useful to you because your target is to not install module_auto_update, but we are using it since long ago and my target is to fix it.

I understand you don't want to work for me, so I did it here: #1190. Basically, all of your job in a single addon, with a clear deprecation path: the best of both worlds 😊

yajo · 2018-03-15T13:37:13Z

 * Juan José Scarafía <jjs@adhoc.com.ar>
 * Jairo Llopis <jairo.llopis@tecnativa.com>

+The main idea of this module comes from `module_auto_update` by LasLabs Inc.


Please reword to avoid confusing readers: you can credit LasLabs for their initial implementation, but the idea comes from somebody else. 😉

I did not know the idea was yours. Credits updated.

This code comes from the module_checksum_upgrade proposal at OCA#1176. * [ADD] module_checksum_upgrade It provides the core mechanism of module_auto_update without the cron nor any change to the standard upgrade mechanism. Instead it provides an API on which module_auto_update can build, as well as a method which can be called from a script to run the upgrade of modules for which the checksum has changed. * [IMP] refactor module_auto_update Make it depend on module_checksum_upgrade which provides the core mechanisms of managing the checksums. module_auto_update makes it automatic. * [IMP] module_checksum_upgrade: better exclusion mechanism Ignore files based on exclude patterns. Ignore uninstalled languages. Better default for patterns to ignore (*.pyc,*.pyo,*.pot,static/*) For better control on the hashing mechanism implement our own: it's quite easy, and the checksumdir module used previously had no test. * [MIG] module_auto_update: adapt to new checksum mechanism * [IMP] module_checksum_upgrade: raise in case of incomplete upgrade * [IMP] module_checksum_upgrade: improve default exclusion pattern * [IMP] module_checksum_upgrade: control translations overwrite * [IMP] module_checksum_upgrade: one more test * [IMP] module_checksum_upgrade: credits [ci skip]

sbidoul · 2018-03-16T12:58:39Z

Moved to #1190

This code comes from the module_checksum_upgrade proposal at OCA#1176. * [ADD] module_checksum_upgrade It provides the core mechanism of module_auto_update without the cron nor any change to the standard upgrade mechanism. Instead it provides an API on which module_auto_update can build, as well as a method which can be called from a script to run the upgrade of modules for which the checksum has changed. * [IMP] refactor module_auto_update Make it depend on module_checksum_upgrade which provides the core mechanisms of managing the checksums. module_auto_update makes it automatic. * [IMP] module_checksum_upgrade: better exclusion mechanism Ignore files based on exclude patterns. Ignore uninstalled languages. Better default for patterns to ignore (*.pyc,*.pyo,*.pot,static/*) For better control on the hashing mechanism implement our own: it's quite easy, and the checksumdir module used previously had no test. * [MIG] module_auto_update: adapt to new checksum mechanism * [IMP] module_checksum_upgrade: raise in case of incomplete upgrade * [IMP] module_checksum_upgrade: improve default exclusion pattern * [IMP] module_checksum_upgrade: control translations overwrite * [IMP] module_checksum_upgrade: one more test * [IMP] module_checksum_upgrade: credits [ci skip]

Syncing from upstream OCA/server-tools (16.0)

legalsylvain reviewed Mar 1, 2018

View reviewed changes

sebalix reviewed Mar 1, 2018

View reviewed changes

sbidoul mentioned this pull request Mar 1, 2018

[module_auto_update] Server is not restarted after update? #1151

Closed

andreparames reviewed Mar 1, 2018

View reviewed changes

ThomasBinsfeld approved these changes Mar 1, 2018

View reviewed changes

andreparames approved these changes Mar 1, 2018

View reviewed changes

sbidoul mentioned this pull request Mar 5, 2018

[11.0] module_checksum_upgrade #1180

Closed

yajo requested changes Mar 7, 2018

View reviewed changes

sbidoul added the needs review label Mar 7, 2018

sbidoul force-pushed the 9.0-module_auto_update_base-sbi branch from 842304a to 69da932 Compare March 7, 2018 18:33

sbidoul added 5 commits March 13, 2018 20:04

[MIG] module_auto_update: adapt to new checksum mechanism

64800c6

[IMP] module_checksum_upgrade: raise in case of incomplete upgrade

e0a38f3

[IMP] module_checksum_upgrade: improve default exclusion pattern

02761f3

[IMP] module_checksum_upgrade: control translations overwrite

65d9e5d

[IMP] module_checksum_upgrade: one more test

595b762

sbidoul force-pushed the 9.0-module_auto_update_base-sbi branch from 9fd031c to 595b762 Compare March 13, 2018 19:05

yajo mentioned this pull request Mar 14, 2018

[REF] module_auto_update: Deprecate buggy features, add new API #1190

Merged

yajo reviewed Mar 15, 2018

View reviewed changes

[IMP] module_checksum_upgrade: credits [ci skip]

d67619c

sbidoul force-pushed the 9.0-module_auto_update_base-sbi branch from d0ca7f7 to d67619c Compare March 15, 2018 15:20

sbidoul closed this Mar 16, 2018

sbidoul deleted the 9.0-module_auto_update_base-sbi branch March 16, 2018 12:58

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024

Merge pull request OCA#1176 from OCA/16.0

097fd13

Syncing from upstream OCA/server-tools (16.0)

		@@ -0,0 +1,6 @@
		This directory contains eggs that were downloaded by setuptools to build, test, and run plug-ins.

	if not self.own_writeable:
	self.skipTest("Own directory not writeable")

Uh oh!

Uh oh!

Conversation

sbidoul commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrobaeza commented Mar 1, 2018

Uh oh!

sbidoul commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrobaeza commented Mar 1, 2018

Uh oh!

sbidoul commented Mar 1, 2018

Uh oh!

pedrobaeza commented Mar 1, 2018

Uh oh!

sbidoul commented Mar 1, 2018

Uh oh!

sbidoul commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreparames commented Mar 1, 2018

Uh oh!

ThomasBinsfeld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrobaeza commented Mar 1, 2018

Uh oh!

sbidoul commented Mar 1, 2018

Uh oh!

yajo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbidoul Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbidoul commented Mar 1, 2018 •

edited

Loading

sbidoul commented Mar 1, 2018 •

edited

Loading

sbidoul commented Mar 1, 2018 •

edited

Loading

sbidoul Mar 7, 2018 •

edited

Loading