should remove the use of str() with LOG.* and exceptions in Cinder

Bug #1274245 reported by Jay Bryant
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Fix Released
High
Jay Bryant

Bug Description

It is my understanding that the use of str() is now frowned upon and that six.text_type should be used instead as we move forward to Python3 . There are many instances where str() is being used in Cinder that it appears need to be addressed.

Jay Bryant (jsbryant)
Changed in cinder:
assignee: nobody → Jay Bryant (jsbryant)
Revision history for this message
Jay Bryant (jsbryant) wrote :

Some background. I came across this concern as I am trying to re-enable lazy translation for Cinder. I was referred to this bug as a reason why we probably shouldn't be using str() for anything logging or exceptions: https://bugs.launchpad.net/oslo/+bug/1225099 .

Open to discussion as to thoughts on whether this is an appropriate change.

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

Ok, so I think I better understand the situation here:

1) They had issues with doing translation in havana because they were getting a mixture of basestrings from str() and unicode strings. If you attempt to use str() on a unicode string and there is something that doesn't translate to ASCII this causes an exception and the message/string in question just disappears. This issue was what caused the lazy translation to be removed at the last minute from havana.

2) There is a workaround in gettextutils that 'should' catch these issues, but if you are doing an str() on unicode further up the stack you could still encounter the same issue.

3) Why the move to six.text_type? Because in py2 you have unicode but in py3 they are changing unicode to string. So, it is best right now to just switch to using six.text_type to avoid the logging issue and so that there are no issues when moving to py3 with the change from unicode to string.

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

I have found a real-world case of this being a problem in taskflow and have opened a bug/submitted a patch: https://bugs.launchpad.net/taskflow/+bug/1275895

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

I am hitting other cases in Cinder where the use of str() on messages is causing issues. Tempest fails on a couple of spots. So, I think I need to fix this and make the i18n lazy translation patch dependent upon this fix.

Jay Bryant (jsbryant)
Changed in cinder:
importance: Undecided → High
Jay Bryant (jsbryant)
summary: - should remove the use of str() in Cinder
+ should remove the use of str() with LOG.* and exceptions in Cinder
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

Fix proposed to branch: master
Review: https://review.openstack.org/76664

Changed in cinder:
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.openstack.org/76664
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=cbe1d5f5e22e5f792128643e4cdd6afb2ff2b5bf
Submitter: Jenkins
Branch: master

commit cbe1d5f5e22e5f792128643e4cdd6afb2ff2b5bf
Author: Jay S. Bryant <email address hidden>
Date: Wed Feb 26 14:40:38 2014 -0600

    Remove str() from LOG.* and exceptions

    This commit removes the use of str() from LOG.* messages
    and exceptions.

    The reason we need to remove str() is because str() will fail if
    it gets a Unicode string that has something that doesn't translate
    to ASCII in it. If such a situation is encountered you will lose
    the message string in question. In most cases, the use of str() is
    unnecessary for LOG.* and exception messages. Using %s is smart
    enough to figure out what to do with what it is passed. It first
    tries to str() it, if this fails it falls back to using unicode. Either
    way, the result will then be something that gettextutils can
    handle and translate.

    Change-Id: I6eb81043edd9fa5e035d81ee81e8439340546d24
    Closes-bug: 1274245
    Related-bp: i18n-messages

Changed in cinder:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in cinder:
milestone: none → icehouse-3
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in cinder:
milestone: icehouse-3 → 2014.1
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.