[12.0] Backport of 13.0 multiple improvements #333
Conversation
etobella
force-pushed
the
12.0-bprt
branch
2 times, most recently
from
e8323cb to
fd5b8ac
Compare
etobella
changed the title
etobella
changed the title
|
Hi @sbidoul, @gurneyalex, Runbot fails because of this on the PRs for 12.0: Do you happen to know why? 😌 |
|
Oh maybe I'm looking at the wrong logs and the problem would be phantom-js. |
|
Could it be the new(ish) docker hub rate limits that kick in ? |
queue_job/job.py
Outdated
| from .exception import (NoSuchJobError, | ||
| FailedJobError, | ||
| RetryableJobError) | ||
| from .exception import FailedJobError, NoSuchJobError, RetryableJobError |
There was a problem hiding this comment.
What do you think about undoing the changes from python-black in this PR? It would make it a lot easier to review.
|
|
||
| return "" | ||
|
|
||
| @http.route("/queue_job/create_test_job", type="http", auth="user") |
There was a problem hiding this comment.
This is strange. In 13.0, this code is added by #265 by @nilshamerlinck (and as such also present in #330), but in your git history it is attributed to @guewen as part of 7385fd5.
I'm getting the feeling that maybe these backports should be split up in separate PRs.
There was a problem hiding this comment.
I think I did something wrong with the commits. I will recheck thanks 😉
Context
-------
The queue_job module adds a new type of field, with the type
"job_serialized", which is close to the base "serialized" field,
adding support of serialization for recordsets and a couple of common
types such as datetime and date.
This field type is used in several fields of the "queue.job" model.
In "ir.model.fields", each field is related to a "ttype", which is
a selection containing every types of fields supported by the current
database.
The "job_serialized" field was never registered as a possible value for
the ttype selection field. It seems it had never been an issue before
Odoo 13.0. With Odoo 13.0, we may have this error on upgrades:
INFO sample odoo.modules.loading: 297 modules loaded in 529.51s, 0 queries
INFO sample odoo.addons.base.models.ir_model: Deleting 4@ir.model.fields.selection (base.selection__ir_model_fields__ttype__job_serialized)
ERROR sample odoo.sql_db: bad query: UPDATE "ir_model_fields" SET "ttype"=NULL WHERE "ttype"='job_serialized'
ERROR: null value in column "ttype" violates not-null constraint
DETAIL: Failing row contains (4296, record_ids, null, queue.job, null, null, null, 266, Record, null, null, t, null, null, f, t, f, f, null, base, null, null, t, null, null, null, null, null, t, null, null, 1, 2020-08-11 13:03:09.913127, null, null).
2020-08-11 13:10:53,168 167 WARNING sample odoo.modules.loading: Transient module states were reset
2020-08-11 13:10:53,170 167 ERROR sample odoo.modules.registry: Failed to load registry
Traceback (most recent call last):
File "/odoo/src/odoo/modules/registry.py", line 86, in new
odoo.modules.load_modules(registry._db, force_demo, status, update_module)
File "/odoo/src/odoo/modules/loading.py", line 472, in load_modules
env['ir.model.data']._process_end(processed_modules)
File "/odoo/src/odoo/addons/base/models/ir_model.py", line 1990, in _process_end
record.unlink()
File "/odoo/src/odoo/addons/base/models/ir_model.py", line 1208, in unlink
self.env.cr.execute(query, [selection.value])
File "/odoo/src/odoo/sql_db.py", line 168, in wrapper
return f(self, *args, **kwargs)
File "/odoo/src/odoo/sql_db.py", line 245, in execute
res = self._obj.execute(query, params)
psycopg2.IntegrityError: null value in column "ttype" violates not-null constraint
DETAIL: Failing row contains (4296, record_ids, null, queue.job, null, null, null, 266, Record, null, null, t, null, null, f, t, f, f, null, base, null, null, t, null, null, null, null, null, t, null, null, 1, 2020-08-11 13:03:09.913127, null, null).
Explanation
-----------
In 13.0, a model "ir.model.fields.selection" storing the allowed
selection values for a field has been added
(odoo/odoo@7593b88).
When the "job_serialized" fields for queue_job are created, they are
properly created in "ir.model.fields" with "job_serialized" as "ttype".
It creates the entry in "ir.model.fields.selection".
When we update the module, if it does not find "job_serialized" as a
possible value for IrModelFields.ttype, it unlinks the entry in
"ir.model.fields.selection". When this happens, an override of the
"unlink()" method in "ir.model.fields.selection" forces a NULL value in
the selection fields (here IrModelFields.ttype).
Fixes: OCA#227
Add a field in the "queue_job" model to store the worker pid. No usage for new besides "logging" for debugging (helps to find which process to investigate in case a job is stuck). Later, it may be useful to improve the recovery of job that stay in the wrong state when the job runner restarts. Closes OCA#177
* @job and @related_action are still supported but emit warnings and will be removed at some point (code to remove has a TODO with :job-no-decorator:) * the options previously set by the decorators are now stored on "queue.job.function" records, which can be explicitly created as XML records (and then editable!) * any method can be used with "with_delay()", if no "queue.job.function" exists, default values are used (root channel, default related action, ...)
As previously, the job functions and channels were created automatically by "_register_job" on methods decorated by `@job`, databases have records without xmlids. To prevent duplicates (or unique constraint error) in case of channels, the create method automatically merges the new record with the existing record. This behavior happens only when installing/upgrading a module, records created manually behave normally. If 2 modules adds the same channel or job function, they'll be merged together with 2 xmlids, on uninstallation of one of the module, the record will be kept thanks to the second xmlid.
As we can delay a job on any method, and the queue.job model is accessible from RPC (as any model), prevent to: * create a queue.job using RPC * write on protected fields (e.g. method name) using RPC Admittedly, the risk is low since users need have Queue Job Manager access to create/write on jobs, but it would allow these users to call internal methods. The check is done using a context key that must be equal to a sentinel object, which is impossible to pass through RPC.
A many2one to select the model + a char for the method name is more convenient and error-proof than having to type the expected format correctly.
Users shouldn't need access to queue.job.function. Read config as sudo().
This patch method has to be called in ``_register_hook``. When a method is patched, any call to the method will not directly execute the method's body, but will instead enqueue a job. When a ``context_key`` is set when calling ``_patch_job_auto_delay``, the patched method is automatically delayed only when this key is ``True`` in the caller's context. It is advised to patch the method with a ``context_key``, because making the automatic delay *in any case* can produce nasty and unexpected side effects (e.g. another module calls the method and expects it to be computed before doing something else, expecting a result, ...). A typical use case is when a method in a module we don't control is called synchronously in the middle of another method, and we'd like all the calls to this method become asynchronous. It relies on OCA#274 that deprecates the `@job` decorator.
It makes the runbot builds fail. There is no urgency to remove the decorator even if we should incentivize the removal because it is no longer supported, hence the info log. Also, show only the XML snippet when debug logs are active, because it is too verbose.
These fields contain the current model and record ids and user id the job works on. Storing them as separate fields does not allow to take benefits on the "JobSerialized" field, which keeps the original user, and later the 'su' flag. We could store the context there as well (or a cleaned one). Use computed fields to keep the model_name, record_ids, user_id fields for backward compatibility. The model_name should stay anyway for the searches on the UI, and the user_id for the same reason + possibility to edit the user.
Which means that the record the job is working on, or any record argument of the job keeps the 'su' flag.
|
Here's the correction for the issue I mentioned before https://github.com/OCA/queue/pull/333/files#r626409959 |
|
Hi @StefanRijnhart, do you still have concerns or can we merge the backports? |
|
For me this is too much to review at once, and to assess the impact of. But I do trust you fully with that, so don't mind me ;-) |
queue_job/views/queue_job_views.xml
Outdated
| <field name="date_created"/> | ||
| <field name="eta"/> | ||
| <field name="date_done"/> | ||
| <field name="exec_time" widget="float_time" /> |
The float_time widget shows hours seconds, we only want seconds. The widget had been removed on the form view, but not on the tree view.
|
@etobella do you run this on production already? |
|
I did the backport in order to backport |
No problem, I only asked to assess the risks ;) |
|
Hi, fyi I've been running this branch in production for 2 weeks without issue. |
Thanks for your feedback @nilshamerlinck! Thanks @etobella for the backport :) /ocabot merge major |
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at 80335af. Thanks a lot for contributing to OCA. ❤️ |
|
Since the merge of this, I get continuous logs with this problem, and no job is executed: DetailsCan you point which can be the problem? |
|
@pedrobaeza can you paste the func_string, method_name, and records, args, kwargs json fields of one of these jobs? |
|
Thanks for the quick reply. It's this function: and the job is this way:
This is before switching to new version. Seeing 13.0 version, it seems to not be any difference: |
|
Was this job created before you updated the code of the module and ran the module upgrade? It looks like https://github.com/OCA/queue/pull/333/files#diff-1c5ee7d63e3224474074dbbdaa46e8d7a838199d2b4d4182726de22fd6e627d1R15-R16 might have been better as an end-migration script, because it might depend on the models and the methods being loaded at the time of migration. If you query the database for the columns @guewen mentions, we can check this hypothesis. -- edit -- that method looks safe though https://github.com/OCA/queue/blob/12.0/queue_job/models/queue_job.py#L563-L573 |
|
Yeah, it can be. For now, I have go back with the previous commit. As soon as I have a slot, I will propose a PR changing it to end-migration and try again to see if that's the problem. |
|
@pedrobaeza not sure that that's it. I have an environment with a lot of jobs on which I can check the migration, so I will post back about that as well. |
|
According to the screenshot above, the queue/queue_job/migrations/12.0.2.1.0/pre-migration.py Lines 20 to 26 in ad28e41 records field. The default value being the base model: Lines 33 to 35 in ad28e41 So:
|
10 participants
Backport of #305
@simahawk @guewen