RPC fake driver should accept datetime items for data

Bug #1529084 reported by Sylvain Bauza
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
oslo.messaging
Fix Released
Undecided
Balazs Gibizer

Bug Description

Since the fake driver only accepts JSON items while jsonutils does accept like datatime, it means that for example, Nova functional tests could be angry because of [1] for example, while it works fine for both Tempest and devstack.

IMHO, we should still accept datetime values as good like [2] or use jsonutils instead.

[1] https://github.com/openstack/oslo.messaging/blob/4dd644ac201ee0fe247d648a2f735998416bf2c7/oslo_messaging/_drivers/impl_fake.py#L176-L185

[2] https://gist.github.com/mriedem/6aa2085b1ecbfd78057f

Revision history for this message
Mehdi Abaakouk (sileht) wrote :

As the comment said any advanced conversion must be done via a serializer, if that works for other driver this is a bug.

Changed in oslo.messaging:
status: New → Invalid
Revision history for this message
Matt Riedemann (mriedem) wrote :

Why doesn't the driver use the serializer used to construct the rpc client? If that were the case, the nova tests would still use the nova RequestContextSerializer(JsonPayloadSerializer()) which would properly serialize datetimes.

Revision history for this message
Balazs Gibizer (balazs-gibizer) wrote :

@Matt: The driver calls the serializer according to my debugging. The problem appears to be that the nova rpc client uses the NovaObjectSerializer that can serialize versioned objects with datetime fields properly but simply ignore (and pass through) a datetime object in a dict. And such dict is passed to rpc in nova.compute.rpcapi.ComputeAPI.prep_resize [1] as the ImageMeta object is first converted to primitives.

Btw in RPC we don't use the JsonPayloadSerializer (it is only used for notifications) but we use the NovaObjectSerializer instead.

[1] https://github.com/openstack/nova/blob/af40e3d1a67c8542683368fd6927ac9c0363a3b8/nova/compute/rpcapi.py#L762

Revision history for this message
Balazs Gibizer (balazs-gibizer) wrote :

I dig deeper.

In a devstack we call the NovaObjectSerializer that does not do anything with a datetime entity. Then the message, still containing datetime objects goes to the messaging driver that calls jsonutils.dumps on it [1]. The jsonutil.dumps call handles datetime properly.

The FakeDriver used in the nova functional test is a lot more strict. It uses the json.dumps to check if the message passed to the driver is serializable to json. And it blows on datetime objects.

Moreover every in tree drivers that implements the RPC send calls jsonutils.dumps except the FakeDriver.

Going to post a fix for this.

[1] https://opendev.org/openstack/oslo.messaging/src/branch/master/oslo_messaging/_drivers/amqpdriver.py#L599
[2] https://github.com/openstack/oslo.messaging/blob/4dd644ac201ee0fe247d648a2f735998416bf2c7/oslo_messaging/_drivers/impl_fake.py#L176-L185

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to oslo.messaging (master)

Fix proposed to branch: master
Review: https://review.opendev.org/683600

Changed in oslo.messaging:
assignee: nobody → Balazs Gibizer (balazs-gibizer)
status: Invalid → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to oslo.messaging (master)

Reviewed: https://review.opendev.org/683600
Committed: https://git.openstack.org/cgit/openstack/oslo.messaging/commit/?id=d8980bfed8621eadce36c6a56944ca23245187a6
Submitter: Zuul
Branch: master

commit d8980bfed8621eadce36c6a56944ca23245187a6
Author: Balazs Gibizer <email address hidden>
Date: Fri Sep 20 19:33:27 2019 +0200

    Align message serialization between drivers

    Every in tree driver that implements RPC send uses jsonutils.dumps to
    serialize the message, except FakeDriver. FakeDriver uses json.dumps.
    However json.dumps is a lot more strict than jsonutils. This caused
    nova to introduce test specific changes in the rpc handling [1].

    This patch makes sure that each driver uses the same json serialization.

    I've tried to dig in to the history of the strictness of the
    FakeDriver. That driver with the json.dumps() call was added back in
    2013 with e2b74cc9e6605156dfd6e36cdfd1b5136161d526. (I cannot link to
    that commit in any online way but it is in my local git clone.)
    Checking out that commit I don't see any other drivers present in the
    repo but the code does mention drivers like RabbitDriver and ZmqDriver
    in oslo.messaging/openstack/common/messaging/drivers.py but only there.

    Today the oslo_messaging._drivers.common.serialize_msg() call is used
    to do the final serialization of the message. It uses jsonutils.dumps
    since Icd54ee8e3f5c976dfd50b4b62c7f51288649e112 which is a revert of
    I0e0f6b715ffc4a9ad82be52e55696d032b6d0976 that changed from
    jsonutils.dumps to jsonutils.dump_as_bytes by mistake. And before this
    back and forth it was jsonutils.dumps since the code was imported from
    oslo-incubator by I38507382b1ce68c7f8f697522f9a1bf00e76532d. Here
    I lost the trail. Honestly I don't know the reason why the fake driver
    was made stricter than the real drivers. Still I think today the
    strictness is unnecessary as every driver uses jsonutils and even
    counterproductive as in [1].

    [1] https://github.com/openstack/nova/blob/09bf71407f4c0d1ddbef89c489ec87c3bca0b7b2/nova/compute/rpcapi.py#L820

    Change-Id: I186305b7897a2a4ce033c11ab9e6bc028854381b
    Closes-Bug: #1529084

Changed in oslo.messaging:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/oslo.messaging 10.3.0

This issue was fixed in the openstack/oslo.messaging 10.3.0 release.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.