RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 1666710 - unicode is not working correctly on python-openvswitch when C json parser is used in a python2 runtime
Summary: unicode is not working correctly on python-openvswitch when C json parser is ...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: openvswitch2.10
Version: 7.6
Hardware: Unspecified
OS: Unspecified
unspecified
high
Target Milestone: rc
: ---
Assignee: Terry Wilson
QA Contact: ovs-qe
URL:
Whiteboard:
Depends On:
Blocks: 1636867 1665213
TreeView+ depends on / blocked
 
Reported: 2019-01-16 12:35 UTC by Miguel Duarte Barroso
Modified: 2023-09-14 04:45 UTC (History)
11 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1671498 (view as bug list)
Environment:
Last Closed: 2019-10-17 21:17:02 UTC
Target Upstream Version:
Embargoed:
mduarted: needinfo+


Attachments (Terms of Use)

Description Miguel Duarte Barroso 2019-01-16 12:35:50 UTC
Description of problem:

When the C json parser in python-openvswitch is used, and the json payload features unicode, an error is thrown.
This issue was found on python2 runtimes - can't tell if it reproduces in python3.

Version-Release number of selected component (if applicable):
python2-openvswitch-2.10.1-1.el7.x86_64

How reproducible:
Always


Steps to Reproduce:

Please check the 'steps to reproduce' section of the dependent bugs on ovirt & rdo:
  - https://bugzilla.redhat.com/show_bug.cgi?id=1636867
  - https://bugzilla.redhat.com/show_bug.cgi?id=1665213


Actual results:
Please check the 'Actual results' section of the dependent bugs on ovirt & rdo:
  - https://bugzilla.redhat.com/show_bug.cgi?id=1636867
  - https://bugzilla.redhat.com/show_bug.cgi?id=1665213


Expected results:
logical network created and result presented to the user.


Additional info:
if we delete the C json parser library - thus forcing python-openvswitch to use the python parser - everything works as expected.

Comment 2 Numan Siddique 2019-01-16 12:46:08 UTC
@Miguel - Looks like this commit [1] should resolve this issue. Can you please try this commit locally and see if it fixes your issue ? 

[1] - https://github.com/openvswitch/ovs/commit/75ff71168a462d4b0e352185f40c1746bd29aa55

Comment 3 Miguel Duarte Barroso 2019-01-16 12:54:55 UTC
Proof using the python parser - which is mentioned in the 'additional info' section - works is found in [0]. 
This means a python-openvswitch build without the C json parser would work as a valid workaround.

[0] - https://gerrit.ovirt.org/#/c/94768/

Comment 4 Miguel Duarte Barroso 2019-01-16 13:01:50 UTC
@numan: I can confirm the proposed patch - located in [0] - *works*.

Thanks for the prompt reply Numan, and thanks for the work Terry.

[0] - https://github.com/openvswitch/ovs/commit/75ff71168a462d4b0e352185f40c1746bd29aa55

Comment 5 Dan Kenigsberg 2019-01-16 20:10:48 UTC
Terry, thanks for this patch.
I do not see in it a unit test, making sure that another random revert breaks this functionality again.
Could you add it upstream, if it is missing?

Could you backport it to a near z-stream?

Comment 6 qding 2019-01-17 01:31:01 UTC
Hello Miguel Duarte Barroso,

We don't have test case for the feature, can you please help verify the issue when it's fixed?

Thanks
Qijun

Comment 7 Miguel Duarte Barroso 2019-01-17 09:06:47 UTC
Replying to Qijun in comment #6:

I can help you verify the issue of the dependent bug 1636867.

Comment 8 Miguel Duarte Barroso 2019-01-17 09:09:10 UTC
I accidentally cleared the needinfo flag; re-setting it w/ the following questions from Dan on comment #5:

I do not see in it a unit test, making sure that another random revert breaks this functionality again.
Could you add it upstream, if it is missing?

Could you backport it to a near z-stream?

Comment 9 qding 2019-01-17 09:27:38 UTC
(In reply to Miguel Duarte Barroso from comment #7)
> 
> I can help you verify the issue of the dependent bug 1636867.

Once the fix is available I will let you know.

Thanks.

Comment 10 Numan Siddique 2019-01-17 13:16:24 UTC
I have backported the fix to 2.10  and pushed it to fdn - -2.10.0-43.el7fdn

Comment 12 Terry Wilson 2019-02-20 17:10:57 UTC
(In reply to Dan Kenigsberg from comment #5)
> Terry, thanks for this patch.
> I do not see in it a unit test, making sure that another random revert
> breaks this functionality again.
> Could you add it upstream, if it is missing?
> 
> Could you backport it to a near z-stream?

The problem is that the unit tests exist, they just aren't run against the python json C extension. I had a patch that enabled the testing, but it was reverted due to some build issues. It's pretty complex in that it changes how the python lib is built and there are changes all throughout the testing code. I will see if I can address the issues, but the patch originally took me a week (at PTG summit with unlimited work time) to finish. Theoretically most of the C json tests should still apply to the C extension, it was just that py2/py3 differences in unicode handling became an issue. I will spend some time trying to fix the original patch, but if it takes too much time, someone else may have to take over.

Comment 13 Terry Wilson 2019-02-20 17:12:52 UTC
I should note, that with the patch with the build changes that would allow running tests against the C extension, all tests passed once the merged change was made. So the patch was tested, it's just not still being tested until we can make the testing patch more robust. :/

Comment 14 Terry Wilson 2019-02-20 17:22:28 UTC
I should also mention, that if you install the RPMs and *then* run the unit tests, the tests *will* be run against the C json extension--since it is used if it exists. It's just that building from source doesn't create/load the c json extension. If it is already in site-packages, it's used when you run the tests. Maybe that is good enough for now?

Comment 15 Flavio Leitner 2019-02-20 19:05:25 UTC
(In reply to Terry Wilson from comment #14)
> I should also mention, that if you install the RPMs and *then* run the unit
> tests, the tests *will* be run against the C json extension--since it is
> used if it exists. It's just that building from source doesn't create/load
> the c json extension. If it is already in site-packages, it's used when you
> run the tests. Maybe that is good enough for now?

We only run the unit tests during RPM build time, and then we don't ship them,
so we would need verification running from sources.

Comment 16 Terry Wilson 2019-02-21 13:40:14 UTC
(In reply to Flavio Leitner from comment #15)
> (In reply to Terry Wilson from comment #14)
> > I should also mention, that if you install the RPMs and *then* run the unit
> > tests, the tests *will* be run against the C json extension--since it is
> > used if it exists. It's just that building from source doesn't create/load
> > the c json extension. If it is already in site-packages, it's used when you
> > run the tests. Maybe that is good enough for now?
> 
> We only run the unit tests during RPM build time, and then we don't ship
> them,
> so we would need verification running from sources.

It looks like we will have a problem in the spec even if we figure out how to fix things in the upstream build process. If I'm reading things correctly, we build both shared and static versions during the RPM build and run the tests from build_static, which is incompatible with building the C extension.

Comment 17 Miguel Duarte Barroso 2019-02-21 15:48:43 UTC
The big point here is that this only happens for py2. 

Personally, I don't think spending lots of time writing a unit for this scenario is worth it, especially with the ongoing effort of moving into py3.

Having said that, @dan knows the roadmap a lot better than I do, thus I think it's his call on pushing for this.

Comment 18 Dan Kenigsberg 2019-02-21 16:03:52 UTC
py2 is to stay with us for many years (osp-13 and rhv-4.3 are long term beasts).

I didn't understand the complexity of when the test run vs RPM build.

All I know is that this Unicode bug haunts us for the third time, and that Life is too short to write non-continuously-tested code.

Comment 19 Flavio Leitner 2019-02-21 17:40:47 UTC
(In reply to Terry Wilson from comment #16)
Timothy, could you please check comment#16 and tell if we have a work around?

Comment 20 Terry Wilson 2019-10-17 21:17:02 UTC
This is fixed in 2.10 which is unsupported, and upstream 2.11+. The testing issue is an issue we can look at separately. Going forward, testing should be a lot simpler since python2 support has been removed.

Comment 21 Red Hat Bugzilla 2023-09-14 04:45:12 UTC
The needinfo request[s] on this closed bug have been removed as they have been unresolved for 1000 days


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