Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proxy gateways #1171

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

Alon-L
Copy link

@Alon-L Alon-L commented Jan 11, 2025

This adds the feature requested in issue #1170 .

Add the proxy attribute to the xspec in order to create proxy gateways.
Proxy gateways do not run tests, and aren't considered as additional workers.
Proxy gateways must be named (must have an id attribute), and need to be passed using the via keyword to additional gateways.

Example:

pytest -sv --dist=load --tx "socket=IP:PORT//id=my_proxy//proxy" --tx "5*popen//via=my_proxy" 

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a testvase for this feature

Given that the separation of the role is a worse idea than i initially thought,

How about introducing a --px ... option to pass the xspec of a proxy

The naming is something where a little bikeshedding may be useful to avoid grief down the line

Thanks for getting this started

Proxy gateways do not run workers, and are meant to be passed with the
`via` attribute to additional gateways.
They are useful for running multiple workers on remote machines.

Example usage:
```
pytest -sv --dist=load --px "id=my_proxy//socket=IP:PORT" --tx "5*popen//via=my_proxy"
```

Proxy gateways do not run workers, anda re meant to be passed
Add docs for using proxy gateways to run multiple workers on remote
machines
@Alon-L Alon-L force-pushed the feature/proxy-gateways branch from 315f040 to 333e2d3 Compare January 12, 2025 18:38
@Alon-L Alon-L marked this pull request as draft January 12, 2025 18:45
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now

I think it's fine to bring in after i validate my concern

Thanks for preparing this and discussing the details

@@ -57,8 +57,17 @@ def __init__(
if self.testrunuid is None:
self.testrunuid = uuid.uuid4().hex
self.group = execnet.Group(execmodel="main_thread_only")
for proxy_spec in self._getpxspecs():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to crosscheck in execnet on how proxy startup is managed

I vaguely recall that main thread only may cause trouble when more than one worker is behind a proxy

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally using 5 workers behind a local socket proxy, and it worked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized my mistake

Proxy doesn't get thr default value of main thread only as its using a own spec

Thanks for validation

@Alon-L
Copy link
Author

Alon-L commented Jan 12, 2025

@RonnyPfannschmidt how can I write a test that uses a remote server? Is there some predefined server available during the tests so I can test the proxy gateway's functionality?

EDIT: I see that the test test_ssh_setup_nodes expects a --gx ssh=... argument, but it seems to never be given on your CICD. How do you make sure these tests are run?

@RonnyPfannschmidt
Copy link
Member

RonnyPfannschmidt commented Jan 12, 2025

These are commonly passed locally

As it's arbitrary code execution we haven't created a ci strategy

Testing against a popen gateway may suffice

Alon-L and others added 2 commits January 12, 2025 21:30
The test `test_proxy_gateway_setup_nodes` makes sure that 2 execnet
gateways are created, but only one node is allocated.

The test `test_proxy_gateway` tries to run a test on a proxy.
@Alon-L Alon-L marked this pull request as ready for review January 12, 2025 19:32
@Alon-L
Copy link
Author

Alon-L commented Jan 13, 2025

Is there an ETA for the merge and release of this feature? I'm not really familiar with the project's release schedule.

@RonnyPfannschmidt
Copy link
Member

There's no eta but i intend to merge/ release this week

I was knocked out by a cold today

@Alon-L
Copy link
Author

Alon-L commented Jan 14, 2025

Alright. Get well soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants