Bug 1373242 - serialization of command parameters allows serialization of immutable objects.
Summary: serialization of command parameters allows serialization of immutable objects.
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: ovirt-engine
Classification: oVirt
Component: BLL.Infra
Version: 3.6.2
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ovirt-4.1.0-alpha
: 4.1.0.2
Assignee: Ravi Nori
QA Contact: Jiri Belka
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-09-05 15:22 UTC by Martin Mucha
Modified: 2017-02-01 14:47 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2017-02-01 14:47:27 UTC
oVirt Team: Infra
Embargoed:
rule-engine: ovirt-4.1+
rule-engine: planning_ack+
mperina: devel_ack+
pstehlik: testing_ack+


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
oVirt gerrit 64061 0 master MERGED engine : serialization of command parameters allows serialization of immutable objects 2016-09-19 19:28:14 UTC

Description Martin Mucha 2016-09-05 15:22:28 UTC
Description of problem:
During startup there's deserialization exception. See bug #1372950.
Analysis: probably during org.ovirt.engine.core.bll.tasks.CommandExecutor#executeAsyncCommand
command parameters are serialized to json and stored into db. Parameters classes are not prepared to handle serialization into json, so they makes no effort making that simple and working (and they shouldn't do that). But utils performing such serialization makes also no effort finding that out, which ends in situation, where immutable is being serialized into db.

Deserialization of such data fails, because immutable objects does not have default constructor.


Actual results:


Expected results:
as we cannot get command parameters classes to cooperation CommandEntityDaoImpl needs to altered scanning for immutable objects, in our case namely collections. Such collections should be either fixed or command rejected as is.

Also(!) whole engine halts because of once un-deserializable command/some tasks?? I'd consider that as a far bigger problem that everything above.

Additional info:

Comment 1 Juan Hernández 2016-09-06 09:56:30 UTC
We should also consider to avoid storing serialized things in the database. It is nearly impossible to guarantee that those things can be de-serialized after an upgrade.

Comment 2 Martin Mucha 2016-09-06 11:08:30 UTC
well — *for sure* — that's true. However many our features (some of them patented) are dependent on usage of this anti-pattern. So there isn't a way (at least not an easy one) how to avoid this.

But if you use jackson with default configuration, then assuming you're serializing some 'libraries' as I do in my example, you end up with:

{"name":null,"shelves":[{"name":null,"books":[{"name":"L'Écume des jours"}]},{"name":null,"books":[{"name":"Brave new world"}]}]}

which, as you realize, does not suffer from any deserialization issues - at all. You need to manually configure json serializer, to include types, using: 

mapper.enableDefaultTyping()

to get:

{"name":null,"shelves":["java.util.ArrayList",[{"name":null,"books":["java.util.ArrayList",[{"name":"L'Écume des jours"}]]},{"name":null,"books":["java.util.Collections$SingletonSet",[{"name":"Brave new world"}]]}]]}

which cannot be deserialized. (As a little surprise for me is that singletonList is serialized as ArrayList, but that's not that important now). And we do just that in org.ovirt.engine.core.utils.serialization.json.JsonObjectSerializer  (nicely in static initializer). We're actively asking jacksons, to produce output, which might be one, which we won't be able to deserialize. 

But if we drop that line, we might have other problems with deserializing certain parameters classes(where there are interfaces used).

Comment 3 Oved Ourfali 2016-09-13 12:15:37 UTC
Martin - please consider the options here, and have someone working on that during 4.1.

Comment 4 Martin Mucha 2016-11-16 11:33:19 UTC
adding CodeChange keyword. 

Deserialization issue was happening in coco framework and not in engine, while reason for coco failure was in engine code. To verify it, specific command would have to be invoked, but such command would have to be altered so that it fail leaving db in invalid state for next engine startup. While this is actually testable, it's probably not that easy to do so, and change isn't marginal flow so it's sufficiently proven by unit tests and everyday work of developers/testers.

Comment 5 Sandro Bonazzola 2016-12-12 13:54:34 UTC
The fix for this issue should be included in oVirt 4.1.0 beta 1 released on December 1st. If not included please move back to modified.

Comment 6 Jiri Belka 2016-12-14 08:05:35 UTC
ok, based on #4


Note You need to log in before you can comment on or make changes to this bug.