Eventlet dnspython monkey-patching problems

Bug #1178732 reported by Brant Knudson on 2013-05-10
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Undecided
Adam Young

Bug Description

We were in a hurry yesterday to fix Keystone gate on CentOS now that the testing infra is moving to CentOS from Oneiric for python 2.6. Essentially any test that starts up Keystone server fails.
#TODO: Look into what's actually failing on CentOS. Use the debugger.

Nova's got a fix where they set the EVENTLET_NO_GREENDNS env var, and also check that eventlet wasn't imported first and raise if it wasn't, see https://review.openstack.org/#/c/26325/4/nova/cmd/__init__.py and https://bugs.launchpad.net/nova/+bug/1164822 .

I put a similar patch into Keystone but my attempt wound up breaking swift. In this case, I put the change in keystone/__init__.py so that it should happen anytime someone imports anything in keystone.*. Seemed like a good idea at the time, and it got the tests to work, but it was too pervasive since now everything that imports keystone.* now also imports eventlet, which I don't think we want because of eventlet's automatic monkey-patching.

#TODO: figure out why swift is importing keystone?

As a quick workaround to fix smokestack and swift, changes were made to
1) set EVENTLET_NO_GREENDNS in the test environment: https://review.openstack.org/#/c/28802/
2) Revert my original fix that changed keystone/__init__.py to import eventlet.

This should buy us some time to work on a better fix where we set the environment variable in the right places. The change to the test environment should be reverted once we've got it.

There's a couple of places where we need to make sure the env var is set:
1) when start keystone server from command-line, in keystone-all
2) when running tests
** In other cases, do NOT want to import eventlet! For example, if running Keystone in Apache **

First, it's easy to modify keystone-all to set the env var.
The tests are a little trickier. I shouldn't have to put this in every test file. I'll look into adding an __init__.py in tests and doing it there if possible.

Do not break swift this time. We should test with swift somehow
#TODO: figure out how to test with swift. See if there's a testcase to be added to Keystone.

Once Keystone works, we can revert https://review.openstack.org/#/c/28802/
#TODO: If this is in stable/grizzly, revert there, too.

Also, re-enable Keystone's IPv6 tests.

Brant Knudson (blk-u) wrote :

I think a better way to fix this is to
1) Move the Server function out to its own module so that it can be imported independently. Then those parts that create a server (keystone-all and tests) will import it.

This will be safer because then other importers of the wsgi model don't get the nasty surprise of eventlet monkeypatching.

So this involves moving Server (https://github.com/openstack/keystone/blob/master/keystone/common/wsgi.py#L91) out to a new module, I'll probably call it wsgi_server.py. Then changing keystone-all to use it and tests.

2) Do the eventlet setup in the new wsgi_server module

Brant Knudson (blk-u) wrote :

This looks like it's fixed by https://review.openstack.org/#/c/29050/

Brant Knudson (blk-u) wrote :

I should say it's partially fixed, https://review.openstack.org/#/c/29050/ moves the server to a separate part to isolate it.

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

Changed in keystone:
assignee: nobody → Brant Knudson (blk-u)
status: New → In Progress
Changed in keystone:
assignee: Brant Knudson (blk-u) → Adam Young (ayoung)

Reviewed: https://review.openstack.org/29710
Committed: http://github.com/openstack/keystone/commit/f9865a57725e316606f00d42fcadc32ea7d61d31
Submitter: Jenkins
Branch: master

commit f9865a57725e316606f00d42fcadc32ea7d61d31
Author: Brant Knudson <email address hidden>
Date: Sun May 19 09:36:16 2013 -0500

    Consolidate eventlet code

    This change consolidates eventlet code to a single module.
    Importing eventlet drags along alot of other behavior changes
    (such as replacing gettaddr with a version that doesn't support
    IPv6), so it's safer if all the eventlet code is in one place so
    it doesn't get imported when it's not needed, such as when
    running under httpd.

    blueprint extract-eventlet

    Part of fix for bug 1178732

    Change-Id: Ia557f8df7f856d7707a9d8c19800ddc36f1572ed

Changed in keystone:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2013-05-29
Changed in keystone:
milestone: none → havana-1
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2013-10-17
Changed in keystone:
milestone: havana-1 → 2013.2
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers