cinder is testing with non-namespaced oslo_i18n in Juno

Bug #1420335 reported by Thomas Goirand
22
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Cinder
Fix Released
Medium
Unassigned
Juno
Fix Released
Medium
John Griffith

Bug Description

Below is the traceback.

As much as I know, oslo_i18n is *not* available in Juno, and shouldn't be in use. I wonder how this slipped into the Juno branch and was released with the last 2014.2.2 point release.

======================================================================
FAIL: unittest.loader.ModuleImportFailure.cinder.tests.test_wsgi
----------------------------------------------------------------------
Traceback (most recent call last):
_StringException: Traceback (most recent call last):
ImportError: Failed to import test module: cinder.tests.test_wsgi
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/testtools/run.py", line 472, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib/python2.7/unittest/loader.py", line 232, in _get_module_from_name
    __import__(name)
  File "cinder/tests/test_wsgi.py", line 28, in <module>
    from oslo_i18n import fixture as i18n_fixture
ImportError: No module named oslo_i18n

Revision history for this message
John Griffith (john-griffith) wrote :

Stems from this bad commit here: https://github.com/openstack/cinder/commit/3965a5f72984fabfa131ef5359a2959a80787a22#diff-e734ea39ff572cae552bf498a40bb33b

My original proposal was updated by others that were unhappy with the change I made and thought it better to add the fixture. Or course that fixture added oslo.i18n which is now causing all sorts of havoc. I've asked Jay and his coworker to get this fixed up asap and expect to see something this morning, if not I'll look at just reverting back to my original proposal.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (stable/juno)

Fix proposed to branch: stable/juno
Review: https://review.openstack.org/154569

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on cinder (stable/juno)

Change abandoned by Bharat Kumar Kobagana (<email address hidden>) on branch: stable/juno
Review: https://review.openstack.org/154569
Reason: As this patch is not required now.

Revision history for this message
Jay Bryant (jsbryant) wrote :

John,

I believe the right solution is to cap the oslo-i18n level in Juno to avoid these issues. I am testing that theory right now. The reason we have not yet pushed a fix, as I understand it, is Doug Hellman is also working on pushing up fixes for oslo library levels. Jim is coordinating with Doug to make sure that we don't make things worse. So, please give us a bit more time today to sort this out.

Thanks.

Revision history for this message
Jay Bryant (jsbryant) wrote :

Actually, Thomas, what level of oslo.i18n do you have installed when you are seeing this?

Revision history for this message
John Griffith (john-griffith) wrote :

@Jay,
Well I think the point here is as I keep trying to emphasize, we should NOT pull in the use of the oslo.i18n into a stable branch at all. If it had been there to begin with that would be one thing, but to add it (even if it's in the req's file which I'm not sure how that worked out). The point is the import of the fixture in test_wsgi shouldn't be there and we should just be using the gettextutils.

This whole i18n effort has become a bit of a mess I think, and I'm most responsible because I gave the patches +2/A :(

Revision history for this message
James Carey (jecarey) wrote :

This appears to be a duplicate of https://bugs.launchpad.net/cinder/+bug/1419964

It looks like the changes to cinder for the oslo i18n namespace changes got pulled back into Juno, so that would say that we need to bump the version for it in Juno.

There is currently a patch to explicitly pin the dependencies in Juno: https://review.openstack.org/#/c/147451/ and it is proposing pinning oslo.i18n to 1.3.1 which includes the namespace change and the fixture.

Revision history for this message
James Carey (jecarey) wrote :

Discussion in #openstack-oslo:

<jecarey> dhellmann, ok ... cinder has backported changes to juno that need the namespace change and the initial fixtures.
...
<dhellmann> jecarey: they shouldn't have done that, we'll be capping the libs to versions that don't include those changes.

Revision history for this message
James Carey (jecarey) wrote :

Added comment to the juno review above that oslo.i18n should be pinned to 1.0.0 and not 1.3.1

Once oslo.i18n is correctly pinned, I think we need to revert this commit: https://review.openstack.org/#/c/145642/

Revision history for this message
John Griffith (john-griffith) wrote :

sighh... again the issue is not pinning, the issue is that the library was introduced by the fixture add and that should have never happened to begin with. I give up.

Revision history for this message
Thomas Goirand (thomas-goirand) wrote :

FYI, Debian Experimental (which holds Juno) has oslo.i18n version 1.0.0, as it should (but I guess everyone in this thread guessed that). And no, the way to fix it isn't to upgrade to the latest oslo.i18n, as it isn't part of Juno. If we decide to do so (which I don't really mind, since oslo.i18n doesn't have much dependencies), then we *must* change:

1/ the global-requirements.txt of Juno (to force a newer oslo.i18n which has the non-namespaced module)
2/ the requirements.txt of Cinder in Juno (to force a newer oslo.i18n which has the non-namespaced module)

But I don't believe this is the path being taken here.

I agree with John (or whoever will be saying that) we shouldn't have allow something that uses the non-namespaced oslo.i18n (the fixture, as it seems). That's the obvious, really...

Cheers,

Thomas Goirand (zigo)

Revision history for this message
Jay Bryant (jsbryant) wrote :

John,

The oslo.i18n level is part of the issue. This is what I shared in IRC. The fix is two part.

The whole reason we had to attempt to put https://github.com/openstack/cinder/commit/3965a5f72984fabfa131ef5359a2959a80787a22#diff-e734ea39ff572cae552bf498a40bb33b in was because the oslo.i18n library levels automatically updated. This caused the test cases to break because of _lazy no loner being accessible in the place it had previously been.

If we pin oslo.i18n to 1.0.0 we move back to the point before the namespace change that broke the test cases. Once the requirements changes lands, we can then revert 3965a5f729 and we will be back to where we originally were for Juno.

Revision history for this message
James Carey (jecarey) wrote :

Here is what I think the situation is:
(1) Cinder moved to using the oslo.i18n library instead of gettextutils from oslo-incubator with https://review.openstack.org/#/c/112613/ which was merged into master on August 9, 2014 which means Juno uses oslo.i18n. This was the first release of the oslo.i18n library 1.0.0.
(2) As part of that patch I changed some of the test cases to mess with the internals of the oslo.i18n library in order to have the test perform the same test.
(3) In the next release of oslo.i18n (1.2.0) it moved its internals to oslo_i18n which made the test cases which used the internals of the library no longer work.
(4) These test cases were updated in Kilo with https://review.openstack.org/#/c/145359/ to no longer be naughty, but to instead use a new test fixture provided with oslo.i18n release 1.3.0
(5) stable/juno did not cap the oslo.i18n library, so instead of using release 1.0.0, it picked up the latest version 1.3.1 This broke Cinder in stable/juno.
(6) The patch in #4 above was backported to stable/juno with https://review.openstack.org/#/c/145642/ in order to fix Cinder in stable/juno

What is happening now:
(A) stable/juno should not use the new oslo.i18n versions. It is going to be patched to pin it to pin oslo.i18n to 1.0.0. This is prior to the namespace change #3 above.
(B) This will break the patch added to stable/juno in #6 above because it relies both on the namespace (1.2.0) and the fixture (1.3.0), so it must be reverted. This will return cinder to the code that worked with oslo.i18n 1.0.0.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (stable/juno)

Fix proposed to branch: stable/juno
Review: https://review.openstack.org/154754

Changed in cinder:
assignee: nobody → John Griffith (john-griffith)
Changed in cinder:
status: New → In Progress
importance: Undecided → Medium
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (stable/juno)

Reviewed: https://review.openstack.org/154754
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=7f861a8d2ab0d8943ab1e86741543ec9109c1c3e
Submitter: Jenkins
Branch: stable/juno

commit 7f861a8d2ab0d8943ab1e86741543ec9109c1c3e
Author: John Griffith <email address hidden>
Date: Tue Feb 10 21:53:32 2015 -0700

    Remove the import of oslo_i18n lib in Juno

    To address bug 1408099 I submitted a patch that
    removed the calls to the private method in the i18n
    code (change I2ae3d9b98c107cebaf386adbdcdb3cfafee070be).

    Unfortunately as can be seen in the patch review,
    my work was modified by another contributor and an update
    of the oslo_18n lib which included a test fixture to togle
    lazy translations was added.

    This was probably fine for Master, HOWEVER that patch was
    then back ported to Juno, which was completely inappropriate
    as Juno was not using the oslo_i18n lib but using
    openstack/common/gettextutils (change 894f20d).

    This patch modifies the change that was submitted and removes the
    import of the oslo_i18n lib which should have never been introduced
    in stable in the first place.

    Before somebody proposes it, leaving the lib dependency and capping
    it is NOT the right answer here. The lib should absolutely not be
    introduced/back-ported in to stable. Especially in this case where
    it simply is used for a unit test that quite frankly is a crap test
    that shouldn't be there to begin with. There is no reason I can see
    at all why we are testing translations in Cinder unit tests, that
    should be left to the unit test for the library itself.

    Change-Id: I5776d9f54d7a85d74311d9b8dbbe28fb81c62027
    Closes-Bug: #1420335

tags: added: in-stable-juno
Revision history for this message
Sean McGinnis (sean-mcginnis) wrote :

Automatically unassigning due to inactivity.

Changed in cinder:
assignee: John Griffith (john-griffith) → nobody
status: In Progress → Triaged
Changed in cinder:
status: Triaged → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

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