Bug 1666710

@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

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/

@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

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?

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

Replying to Qijun in comment #6:

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

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?

(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.

I have backported the fix to 2.10  and pushed it to fdn - -2.10.0-43.el7fdn

(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.

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. :/

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?

(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.

(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.

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.

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.

(In reply to Terry Wilson from comment #16)
Timothy, could you please check comment#16 and tell if we have a work around?

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.

The needinfo request[s] on this closed bug have been removed as they have been unresolved for 1000 days