[oslo][nova] Revert of oslo.messaging JSON serialization change
Hi,
I've just proposed https://review.opendev.org/#/c/685724/ which reverts a change that recently went in to make the fake driver in oslo.messaging use jsonutils for message serialization instead of json.dumps.
As explained in the commit message on the revert, this is problematic because the rabbit driver uses kombu's default serialization method, which is json.dumps. By changing the fake driver to use jsonutils we've made it more lenient than the most used real driver which opens us up to merging broken changes in consumers of oslo.messaging.
We did have some discussion of whether we should try to override the kombu default and tell it to use jsonutils too, as a number of other drivers do. The concern with this was that the jsonutils handler for things like datetime objects is not tz-aware, which means if you send a datetime object over RPC and don't explicitly handle it you could lose important information.
I'm open to being persuaded otherwise, but at the moment I'm leaning toward less magic happening at the RPC layer and requiring projects to explicitly handle types that aren't serializable by the standard library json module. If you have a different preference, please share it here.
Thanks.
-Ben
On Mon, Sep 30, 2019 at 4:45 PM, Ben Nemec openstack@nemebean.com wrote:
Hi,
I've just proposed https://review.opendev.org/#/c/685724/ which reverts a change that recently went in to make the fake driver in oslo.messaging use jsonutils for message serialization instead of json.dumps.
As explained in the commit message on the revert, this is problematic because the rabbit driver uses kombu's default serialization method, which is json.dumps. By changing the fake driver to use jsonutils we've made it more lenient than the most used real driver which opens us up to merging broken changes in consumers of oslo.messaging.
We did have some discussion of whether we should try to override the kombu default and tell it to use jsonutils too, as a number of other drivers do. The concern with this was that the jsonutils handler for things like datetime objects is not tz-aware, which means if you send a datetime object over RPC and don't explicitly handle it you could lose important information.
I'm open to being persuaded otherwise, but at the moment I'm leaning toward less magic happening at the RPC layer and requiring projects to explicitly handle types that aren't serializable by the standard library json module. If you have a different preference, please share it here.
Hi,
I might me totally wrong here and please help me understand how the RabbitDriver works. What I did when I created the original patch that I looked at each drivers how they handle sending messages. The oslo_messaging._drivers.base.BaseDriver defines the interface with a send() message. The oslo_messaging._drivers.amqpdriver.AMQPDriverBase implements the BaseDriver interface's send() method to call _send(). Then _send() calls rpc_commom.serialize_msg which then calls jsonutils.dumps.
The oslo_messaging._drivers.impl_rabbit.RabbitDriver driver inherits from AMQPDriverBase and does not override send() or _send() so I think the AMQPDriverBase ._send() is called that therefore jsonutils is used during sending a message with RabbitDriver.
Cheers, gibi
[1] https://github.com/openstack/oslo.messaging/blob/7734ac1376a1a9285c8245a91cf...
Thanks.
-Ben
On Mon, Sep 30, 2019 at 5:35 PM, Balázs Gibizer balazs.gibizer@est.tech wrote:
On Mon, Sep 30, 2019 at 4:45 PM, Ben Nemec openstack@nemebean.com wrote:
Hi,
I've just proposed https://review.opendev.org/#/c/685724/ which reverts a change that recently went in to make the fake driver in oslo.messaging use jsonutils for message serialization instead of json.dumps.
As explained in the commit message on the revert, this is problematic because the rabbit driver uses kombu's default serialization method, which is json.dumps. By changing the fake driver to use jsonutils we've made it more lenient than the most used real driver which opens us up to merging broken changes in consumers of oslo.messaging.
We did have some discussion of whether we should try to override the kombu default and tell it to use jsonutils too, as a number of other drivers do. The concern with this was that the jsonutils handler for things like datetime objects is not tz-aware, which means if you send a datetime object over RPC and don't explicitly handle it you could lose important information.
I'm open to being persuaded otherwise, but at the moment I'm leaning toward less magic happening at the RPC layer and requiring projects to explicitly handle types that aren't serializable by the standard library json module. If you have a different preference, please share it here.
Hi,
I might me totally wrong here and please help me understand how the RabbitDriver works. What I did when I created the original patch that I looked at each drivers how they handle sending messages. The oslo_messaging._drivers.base.BaseDriver defines the interface with a send() message. The oslo_messaging._drivers.amqpdriver.AMQPDriverBase implements the BaseDriver interface's send() method to call _send(). Then _send() calls rpc_commom.serialize_msg which then calls jsonutils.dumps.
The oslo_messaging._drivers.impl_rabbit.RabbitDriver driver inherits from AMQPDriverBase and does not override send() or _send() so I think the AMQPDriverBase ._send() is called that therefore jsonutils is used during sending a message with RabbitDriver.
I did some tracing in devstack to prove my point. See the result in https://review.opendev.org/#/c/685724/1//COMMIT_MSG@11
Cheers, gibi
Cheers, gibi
[1] https://github.com/openstack/oslo.messaging/blob/7734ac1376a1a9285c8245a91cf...
Thanks.
-Ben
Sorry I'm late to the party....
At the risk of stating the obvious I wouldn't put much faith in the fact that the Kafka and Amqp1 drivers use jsonutils. The use of jsonutils in these drivers is simply a cut-n-paste from the way old qpidd driver. Why jsonutils was used there... I dunno.
IMHO the RabbitMQ driver is the authoritative source for correct driver implementation - the Fake driver (and the others) should use the same serialization as the rabbitmq driver if possible.
-K
On Tue, Oct 1, 2019 at 4:30 AM Balázs Gibizer balazs.gibizer@est.tech wrote:
On Mon, Sep 30, 2019 at 5:35 PM, Balázs Gibizer balazs.gibizer@est.tech wrote:
On Mon, Sep 30, 2019 at 4:45 PM, Ben Nemec openstack@nemebean.com wrote:
Hi,
I've just proposed https://review.opendev.org/#/c/685724/ which reverts a change that recently went in to make the fake driver in oslo.messaging use jsonutils for message serialization instead of json.dumps.
As explained in the commit message on the revert, this is problematic because the rabbit driver uses kombu's default serialization method, which is json.dumps. By changing the fake driver to use jsonutils we've made it more lenient than the most used real driver which opens us up to merging broken changes in consumers of oslo.messaging.
We did have some discussion of whether we should try to override the kombu default and tell it to use jsonutils too, as a number of other drivers do. The concern with this was that the jsonutils handler for things like datetime objects is not tz-aware, which means if you send a datetime object over RPC and don't explicitly handle it you could lose important information.
I'm open to being persuaded otherwise, but at the moment I'm leaning toward less magic happening at the RPC layer and requiring projects to explicitly handle types that aren't serializable by the standard library json module. If you have a different preference, please share it here.
Hi,
I might me totally wrong here and please help me understand how the RabbitDriver works. What I did when I created the original patch that I looked at each drivers how they handle sending messages. The oslo_messaging._drivers.base.BaseDriver defines the interface with a send() message. The oslo_messaging._drivers.amqpdriver.AMQPDriverBase implements the BaseDriver interface's send() method to call _send(). Then _send() calls rpc_commom.serialize_msg which then calls jsonutils.dumps.
The oslo_messaging._drivers.impl_rabbit.RabbitDriver driver inherits from AMQPDriverBase and does not override send() or _send() so I think the AMQPDriverBase ._send() is called that therefore jsonutils is used during sending a message with RabbitDriver.
I did some tracing in devstack to prove my point. See the result in https://review.opendev.org/#/c/685724/1//COMMIT_MSG@11
Cheers, gibi
Cheers, gibi
[1]
https://github.com/openstack/oslo.messaging/blob/7734ac1376a1a9285c8245a91cf...
Thanks.
-Ben
TLDR: I've abandoned the revert.
After looking at Gibi's investigation further I agree that rabbit was actually using the jsonutils version of dumps, so making the fake driver use it is consistent. Apologies for the confusion.
-Ben
On 10/1/19 3:35 PM, Ken Giusti wrote:
Sorry I'm late to the party....
At the risk of stating the obvious I wouldn't put much faith in the fact that the Kafka and Amqp1 drivers use jsonutils. The use of jsonutils in these drivers is simply a cut-n-paste from the way old qpidd driver. Why jsonutils was used there... I dunno.
IMHO the RabbitMQ driver is the authoritative source for correct driver implementation - the Fake driver (and the others) should use the same serialization as the rabbitmq driver if possible.
-K
On Tue, Oct 1, 2019 at 4:30 AM Balázs Gibizer balazs.gibizer@est.tech wrote:
On Mon, Sep 30, 2019 at 5:35 PM, Balázs Gibizer <balazs.gibizer@est.tech> wrote: > > > On Mon, Sep 30, 2019 at 4:45 PM, Ben Nemec <openstack@nemebean.com <mailto:openstack@nemebean.com>> > wrote: >> Hi, >> >> I've just proposed https://review.opendev.org/#/c/685724/ which >> reverts a change that recently went in to make the fake driver in >> oslo.messaging use jsonutils for message serialization instead of >> json.dumps. >> >> As explained in the commit message on the revert, this is >> problematic >> because the rabbit driver uses kombu's default serialization method, >> which is json.dumps. By changing the fake driver to use jsonutils >> we've made it more lenient than the most used real driver which >> opens >> us up to merging broken changes in consumers of oslo.messaging. >> >> We did have some discussion of whether we should try to override the >> kombu default and tell it to use jsonutils too, as a number of other >> drivers do. The concern with this was that the jsonutils handler for >> things like datetime objects is not tz-aware, which means if you >> send >> a datetime object over RPC and don't explicitly handle it you could >> lose important information. >> >> I'm open to being persuaded otherwise, but at the moment I'm leaning >> toward less magic happening at the RPC layer and requiring projects >> to explicitly handle types that aren't serializable by the standard >> library json module. If you have a different preference, please >> share >> it here. > > Hi, > > I might me totally wrong here and please help me understand how the > RabbitDriver works. What I did when I created the original patch that > I > looked at each drivers how they handle sending messages. The > oslo_messaging._drivers.base.BaseDriver defines the interface with a > send() message. The oslo_messaging._drivers.amqpdriver.AMQPDriverBase > implements the BaseDriver interface's send() method to call _send(). > Then _send() calls rpc_commom.serialize_msg which then calls > jsonutils.dumps. > > The oslo_messaging._drivers.impl_rabbit.RabbitDriver driver inherits > from AMQPDriverBase and does not override send() or _send() so I think > the AMQPDriverBase ._send() is called that therefore jsonutils is used > during sending a message with RabbitDriver. I did some tracing in devstack to prove my point. See the result in https://review.opendev.org/#/c/685724/1//COMMIT_MSG@11 Cheers, gibi > > Cheers, > gibi > > > [1] > https://github.com/openstack/oslo.messaging/blob/7734ac1376a1a9285c8245a91cf43599358bfa9d/oslo_messaging/_drivers/amqpdriver.py#L599 > >> >> Thanks. >> >> -Ben >> > >-- Ken Giusti (kgiusti@gmail.com mailto:kgiusti@gmail.com)
On 9/30/19 4:45 PM, Ben Nemec wrote:
The concern with this was that the jsonutils handler for things like datetime objects is not tz-aware, which means if you send a datetime object over RPC and don't explicitly handle it you could lose important information.
echo Etc/UTC >/etc/timezone
Problem solved... :)
Thomas
participants (4)
-
Balázs Gibizer
-
Ben Nemec
-
Ken Giusti
-
Thomas Goirand