settingsLogin | Registersettings

[openstack-dev] [neutron] monkey patching strategy

0 votes

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all,

there were some moves recently to make monkey patching strategy sane
in neutron.

This was triggered by some bugs found when interacting with external
oslo libraries 1, and a cross project spec to make eventlet usage
sane throughout the project 2.

Specifically, instead of monkey patching stdlib in each of services
and agents (and forgetting to do so for some of them 3), we should
monkey patch it as part of a common import (ideally, it would be any
neutron.* import).

Initially, we've tried to patch it inside neutron/init.py 4, but
it didn't place nice with some advanced services importing from
neutron while not expecting stdlib to be patched, and so was reverted.

So an alternative that I currently look into is "the Nova way".
Specifically, moving all main() functions for all agents and services
into neutron/cmd/... and monkey patching stdlib thru
neutron/cmd/init.py.

I've sent a series of patches to do just that 5. It was rightfully
blocked by Mark to seek for broader agreement.

I encourage community to say your word on the direction.

Cheers,
/Ihar
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU3P4QAAoJEC5aWaUY1u57A/cH/AuKbkewZy5Z0Hus2m4bClGp
4DJ37ygcY9HwGmJTLpvUyfRcDWnaO9S+6sj28Ebv49MN1w9qJ4MuQmaYA1xsFERb
aR6uKgnkiIT0FS8CVjbClEC7gN5elHCe2dcB8cakrk7uUsTJ2LP5A6rdNQqly/uN
2hkdfa1WBcAYMX6raFWD8DJ49R1MhbPr09YXXU9ApoflMY6ZywvNBzwIZEw5gqPO
Vpjb9DwevaFZ9kqzjHTrXk47CqOSYS7ZXQjS1bOGUOJFOBqNRLzl2qPX7wkBiA2N
12U4Qe3/3MvWwBig0O+mY2RwN2OtnxhK8X5tP6kbrybyOKLGUe4ZgIlvfQHI33Q=
=8pX5
-----END PGP SIGNATURE-----


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
asked Feb 12, 2015 in openstack-dev by Ihar_Hrachyshka (35,300 points)   3 9 16
retagged Mar 6, 2015 by admin

6 Responses

0 votes

Why did the services fail with the stdlib patched? Are they incompatible
with eventlet?

On Thu, Feb 12, 2015 at 11:25 AM, Ihar Hrachyshka ihrachys@redhat.com
wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all,

there were some moves recently to make monkey patching strategy sane
in neutron.

This was triggered by some bugs found when interacting with external
oslo libraries [1], and a cross project spec to make eventlet usage
sane throughout the project [2].

Specifically, instead of monkey patching stdlib in each of services
and agents (and forgetting to do so for some of them [3]), we should
monkey patch it as part of a common import (ideally, it would be any
neutron.* import).

Initially, we've tried to patch it inside neutron/init.py [4], but
it didn't place nice with some advanced services importing from
neutron while not expecting stdlib to be patched, and so was reverted.

So an alternative that I currently look into is "the Nova way".
Specifically, moving all main() functions for all agents and services
into neutron/cmd/... and monkey patching stdlib thru
neutron/cmd/init.py.

I've sent a series of patches to do just that [5]. It was rightfully
blocked by Mark to seek for broader agreement.

I encourage community to say your word on the direction.

[1]: https://bugs.launchpad.net/oslo.concurrency/+bug/1418541
[2]: https://review.openstack.org/154642
[3]:

http://git.openstack.org/cgit/openstack/neutron/tree/neutron/plugins/mlnx/agent/eswitch_neutron_agent.py
[4]: https://review.openstack.org/153699
[5]:

https://review.openstack.org/#/q/status:open+project:openstack/neutron+branch:master+topic:bug/1418541,n,z

Cheers,
/Ihar
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU3P4QAAoJEC5aWaUY1u57A/cH/AuKbkewZy5Z0Hus2m4bClGp
4DJ37ygcY9HwGmJTLpvUyfRcDWnaO9S+6sj28Ebv49MN1w9qJ4MuQmaYA1xsFERb
aR6uKgnkiIT0FS8CVjbClEC7gN5elHCe2dcB8cakrk7uUsTJ2LP5A6rdNQqly/uN
2hkdfa1WBcAYMX6raFWD8DJ49R1MhbPr09YXXU9ApoflMY6ZywvNBzwIZEw5gqPO
Vpjb9DwevaFZ9kqzjHTrXk47CqOSYS7ZXQjS1bOGUOJFOBqNRLzl2qPX7wkBiA2N
12U4Qe3/3MvWwBig0O+mY2RwN2OtnxhK8X5tP6kbrybyOKLGUe4ZgIlvfQHI33Q=
=8pX5
-----END PGP SIGNATURE-----


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

--
Kevin Benton


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Feb 13, 2015 by Kevin_Benton (24,800 points)   3 5 5
0 votes

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/13/2015 02:33 AM, Kevin Benton wrote:
Why did the services fail with the stdlib patched? Are they
incompatible with eventlet?

It's not like service entry points are not ready for neutron.* to be
monkey patched, but tools around it (flake8 that imports
neutron.hacking.checks, setuptools that import hooks from
neutron.hooks etc). It's also my belief that base library should not
be monkey patched not to put additional assumptions onto consumers.

(Though I believe that all the code in the tree should be monkey
patched, including those agents that currently run without the library
patched - for consistency and to reflect the same test environment for
unit tests that will be patched from neutron/tests/init.py).

/Ihar
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU3hpVAAoJEC5aWaUY1u57nBUIANIjk5j31l2E9+HNvUhiMulP
mIa7zB3PNO+XsFHq9DWWUHzKoY5GVYF9DdLN56wbiLCCD7aoR3XZ/euGRm9NnJzf
Akb6+qsq/1+qge4zG0C33aBc/lted+1RdVU7aDk8pUpUbWAEW833EqwolXdtg0RF
aljsg5U3759MPpZpRV8o/GzHScmTvWenn6hLzXQ2frGFHoR4OpPkEctME1LAmNxs
GqfKeK5ST0wVsCimTFMM/BV7zQTQSeiMNYQesnqTh4V3mjaS3UiHJfCQF9KtiJJG
AaMEW3wyBAO3ew373oXUMTd4k0HhO34RuK4eznYdq7BFQO6KDjFSC1HtvFJaPCk=
=KckY
-----END PGP SIGNATURE-----


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Feb 13, 2015 by Ihar_Hrachyshka (35,300 points)   3 9 16
0 votes

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

response was huge so far :) so to add more traction, I have a question
for everyone. Let's assume we want to move entry points for all
services and agents into neutron/cmd/... If so,

    • Do we want all existing tools stored in the path to be monkey
      patched too? I would say 'yes', to make sure we run our unit tests in
      the same environment as in real life;
    • Which parts of services we want to see there? Should they include
      any real main() or register_options() code, or should they be just a
      wrappers to call actual main() located somewhere in other parts of the
      tree? I lean toward leaving just a one liner main() under
      neutron/cmd/... that calls to 'real' main() located in a different
      place in the tree.

Comments?

/Ihar

On 02/13/2015 04:37 PM, Ihar Hrachyshka wrote:
On 02/13/2015 02:33 AM, Kevin Benton wrote:

Why did the services fail with the stdlib patched? Are they
incompatible with eventlet?

It's not like service entry points are not ready for neutron.* to
be monkey patched, but tools around it (flake8 that imports
neutron.hacking.checks, setuptools that import hooks from
neutron.hooks etc). It's also my belief that base library should
not be monkey patched not to put additional assumptions onto
consumers.

(Though I believe that all the code in the tree should be monkey
patched, including those agents that currently run without the
library patched - for consistency and to reflect the same test
environment for unit tests that will be patched from
neutron/tests/init.py).

/Ihar


OpenStack Development Mailing List (not for usage questions)

Unsubscribe:
OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU41iWAAoJEC5aWaUY1u57zBYIAIuobIYMZ1NJmm+7sV+NW6LS
ZS4PNKlwcYRrdfArGliUq7GLVi/ZRNPNgilF9RIJXQAiOXEc6PmKqpKw1JnwkQ7v
l3/NeciYmkMhSNRv1vIrOBHegAYx9Js6o2lOBCF7BFKIpu88OsC95oobcLGtcrYU
BxoBUM7DYvHssDhRp3NujNbyMrRkg4roer7+4qGE3a449tv4xViTcoUWg5MoNalY
vD1ld/Gg8LfKPt7v7FbF2YnHkMG+UJSk47rRd0yv9KGABS69TkNuvJXeJ14sgw0O
YqIY3oMO0nza+T8tdQGTrYv9N4rWOMFsJMyrOLIvoUyq526QQZ/K7Hrijj1IQjE=
=ZtVP
-----END PGP SIGNATURE-----


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Feb 17, 2015 by Ihar_Hrachyshka (35,300 points)   3 9 16
0 votes

My opinions inline.

On 17 February 2015 at 16:04, Ihar Hrachyshka ihrachys@redhat.com wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

response was huge so far :) so to add more traction, I have a question
for everyone. Let's assume we want to move entry points for all
services and agents into neutron/cmd/... If so,

I don't have anything again this assumption. Also it seems other projects
are already doing it this way so there is no "divergence" issue here.


    • Do we want all existing tools stored in the path to be monkey
      patched too? I would say 'yes', to make sure we run our unit tests in
      the same environment as in real life;

I say yes but mildly here. If you're referring to the tools used for
running flake8 or unit tests in theory it should not really matter whether
they're patched or not. However, I'm aware of unit tests which spawn
eventlet threadpools, so it's definitely better to ensure all these tools
are patched.


    • Which parts of services we want to see there? Should they include
      any real main() or register_options() code, or should they be just a
      wrappers to call actual main() located somewhere in other parts of the
      tree? I lean toward leaving just a one liner main() under
      neutron/cmd/... that calls to 'real' main() located in a different
      place in the tree.

My vote is for the one-liner.

Comments?

/Ihar

On 02/13/2015 04:37 PM, Ihar Hrachyshka wrote:

On 02/13/2015 02:33 AM, Kevin Benton wrote:

Why did the services fail with the stdlib patched? Are they
incompatible with eventlet?

It's not like service entry points are not ready for neutron.* to
be monkey patched, but tools around it (flake8 that imports
neutron.hacking.checks, setuptools that import hooks from
neutron.hooks etc). It's also my belief that base library should
not be monkey patched not to put additional assumptions onto
consumers.

(Though I believe that all the code in the tree should be monkey
patched, including those agents that currently run without the
library patched - for consistency and to reflect the same test
environment for unit tests that will be patched from
neutron/tests/init.py).

/Ihar


>
>
OpenStack Development Mailing List (not for usage questions)

Unsubscribe:
OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU41iWAAoJEC5aWaUY1u57zBYIAIuobIYMZ1NJmm+7sV+NW6LS
ZS4PNKlwcYRrdfArGliUq7GLVi/ZRNPNgilF9RIJXQAiOXEc6PmKqpKw1JnwkQ7v
l3/NeciYmkMhSNRv1vIrOBHegAYx9Js6o2lOBCF7BFKIpu88OsC95oobcLGtcrYU
BxoBUM7DYvHssDhRp3NujNbyMrRkg4roer7+4qGE3a449tv4xViTcoUWg5MoNalY
vD1ld/Gg8LfKPt7v7FbF2YnHkMG+UJSk47rRd0yv9KGABS69TkNuvJXeJ14sgw0O
YqIY3oMO0nza+T8tdQGTrYv9N4rWOMFsJMyrOLIvoUyq526QQZ/K7Hrijj1IQjE=
=ZtVP
-----END PGP SIGNATURE-----


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Feb 17, 2015 by Salvatore_Orlando (12,280 points)   2 5 8
0 votes

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/17/2015 04:19 PM, Salvatore Orlando wrote:
My opinions inline.

On 17 February 2015 at 16:04, Ihar Hrachyshka <ihrachys@redhat.com
ihrachys@redhat.com> wrote:

Hi,

response was huge so far :) so to add more traction, I have a
question for everyone. Let's assume we want to move entry points
for all services and agents into neutron/cmd/... If so,

I don't have anything again this assumption. Also it seems other
projects are already doing it this way so there is no
"divergence" issue here.

  • Do we want all existing tools stored in the path to be monkey
    patched too? I would say 'yes', to make sure we run our unit tests
    in the same environment as in real life;

I say yes but mildly here. If you're referring to the tools used
for running flake8 or unit tests in theory it should not really
matter whether they're patched or not. However, I'm aware of unit
tests which spawn eventlet threadpools, so it's definitely better
to ensure all these tools are patched.

No, I mean ovscleanup, sanitycheck, usage_audit that are located in
the neutron/cmd path but not patched.

  • Which parts of services we want to see there? Should they
    include any real main() or register_options() code, or should they
    be just a wrappers to call actual main() located somewhere in other
    parts of the tree? I lean toward leaving just a one liner main()
    under neutron/cmd/... that calls to 'real' main() located in a
    different place in the tree.

My vote is for the one-liner.

Comments?

/Ihar

On 02/13/2015 04:37 PM, Ihar Hrachyshka wrote:

On 02/13/2015 02:33 AM, Kevin Benton wrote:

Why did the services fail with the stdlib patched? Are they
incompatible with eventlet?

It's not like service entry points are not ready for neutron.*
to be monkey patched, but tools around it (flake8 that imports
neutron.hacking.checks, setuptools that import hooks from
neutron.hooks etc). It's also my belief that base library should
not be monkey patched not to put additional assumptions onto
consumers.

(Though I believe that all the code in the tree should be monkey
patched, including those agents that currently run without the
library patched - for consistency and to reflect the same test
environment for unit tests that will be patched from
neutron/tests/init.py).

/Ihar


OpenStack Development Mailing List (not for usage questions)

Unsubscribe:
OpenStack-dev-request@lists.openstack.org?subject:unsubscribe

http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)

Unsubscribe:
OpenStack-dev-request@lists.openstack.org?subject:unsubscribe

http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


OpenStack Development Mailing List (not for usage questions)

Unsubscribe:
OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU429PAAoJEC5aWaUY1u57IkcH/1HCRHYzeXfWE3I3ZamAJufI
/1/CPcrzW3w3pJebfSLXDNbdv9ziQXwZogcM7JcDMeeYfWA42OexhFQJqerkoJnr
oL3eqUOgh19pgVUgAah1n7yQEHxyzbnVR0TcdVOvMlxno8I3hUXy78WvBWQPYIpr
NRDSbT+SiQv/OP6/zTkKLkk2SA88lJlKQpGg5Q0iRQTnqiNtF3REBdUTM/32aJyh
h9ZxmR8wyrXJv6oEUfGj210vJHUvmPHk3SsH2udQjCG0MdbIQTIZJwgzMVxD4aIs
3uQ9ONqn8Fd2LKzfawHVwT0azd6kCkQjEalZx9Tn/58NPuQ4WERhHcCzBT8pNyI=
=DGy3
-----END PGP SIGNATURE-----


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Feb 17, 2015 by Ihar_Hrachyshka (35,300 points)   3 9 16
0 votes

Hi,

The general discussion is going in https://review.openstack.org/#/c/148318/.
My opinions on the following specific topics inline.

2015-02-18 0:04 GMT+09:00 Ihar Hrachyshka ihrachys@redhat.com:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

response was huge so far :) so to add more traction, I have a question
for everyone. Let's assume we want to move entry points for all
services and agents into neutron/cmd/... If so,


    • Do we want all existing tools stored in the path to be monkey
      patched too?

I start to think neutron/cmd/eventlet/* can be an option.
The single place to apply monkey-patch is nice, but I am not sure
it is good that all commands in neutron/cmd are monkey-patched.

At now we only have small tools in cmd and they are not affected even
if monkey-patched.
If we have non-eventlet version of commands/agents, where should we place them?
I believe that a good naming convention helps new/most developers
understand the code base.

I would say 'yes', to make sure we run our unit tests in
the same environment as in real life;

Regarding our unit tests, I am not sure what way is good.
At now most codes are run with monkey-patched version of stdlib,
so apply monkey-pathc in neutron/tests/init.py sounds good.
However, what can we do if some non-eventlet modules are available?
These modules should not be monkey-patched.


    • Which parts of services we want to see there? Should they include
      any real main() or register_options() code, or should they be just a
      wrappers to call actual main() located somewhere in other parts of the
      tree? I lean toward leaving just a one liner main() under
      neutron/cmd/... that calls to 'real' main() located in a different
      place in the tree.

My vote is for one-liner main(). More precisely, codes which are directly
related to eventlet monkey-patch should be placed.
It seems config options are not related to event monkey-patch directly.
If we have non-eventlet version of commands/agents, real 'main' can stay
in the same place and we can just remove the corresponding part from
neutron/cmd/.

Thanks,
Akihiro

Comments?

/Ihar

On 02/13/2015 04:37 PM, Ihar Hrachyshka wrote:

On 02/13/2015 02:33 AM, Kevin Benton wrote:

Why did the services fail with the stdlib patched? Are they
incompatible with eventlet?

It's not like service entry points are not ready for neutron.* to
be monkey patched, but tools around it (flake8 that imports
neutron.hacking.checks, setuptools that import hooks from
neutron.hooks etc). It's also my belief that base library should
not be monkey patched not to put additional assumptions onto
consumers.

(Though I believe that all the code in the tree should be monkey
patched, including those agents that currently run without the
library patched - for consistency and to reflect the same test
environment for unit tests that will be patched from
neutron/tests/init.py).

/Ihar


OpenStack Development Mailing List (not for usage questions)

Unsubscribe:
OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU41iWAAoJEC5aWaUY1u57zBYIAIuobIYMZ1NJmm+7sV+NW6LS
ZS4PNKlwcYRrdfArGliUq7GLVi/ZRNPNgilF9RIJXQAiOXEc6PmKqpKw1JnwkQ7v
l3/NeciYmkMhSNRv1vIrOBHegAYx9Js6o2lOBCF7BFKIpu88OsC95oobcLGtcrYU
BxoBUM7DYvHssDhRp3NujNbyMrRkg4roer7+4qGE3a449tv4xViTcoUWg5MoNalY
vD1ld/Gg8LfKPt7v7FbF2YnHkMG+UJSk47rRd0yv9KGABS69TkNuvJXeJ14sgw0O
YqIY3oMO0nza+T8tdQGTrYv9N4rWOMFsJMyrOLIvoUyq526QQZ/K7Hrijj1IQjE=
=ZtVP
-----END PGP SIGNATURE-----


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

--
Akihiro Motoki amotoki@gmail.com


OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-request@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
responded Feb 18, 2015 by Akihiro_Motoki (8,520 points)   2 3 4
...