Add support for Unix sockets #1118 #1129

graingert · 2015-08-13T12:17:48Z

This also adds support for TLS!

Simply use https://exmaple.com/ for TLS or http+unix://%2Fvar%2Frun%2Fluigid%2Fluigid.sock for Unix sockets

There doesn't seem to be any actual tests to see if the real fetch method works, as all the tests that cover RemoteScheduler patches "._fetch"

Tarrasch · 2015-08-13T13:04:48Z

Hmm, can you elaborate more on what this is and who would use it and why? Is this only related to the luigi server?

graingert · 2015-08-13T13:06:07Z

@Tarrasch the discussion is in #1118

erikbern · 2015-08-13T14:51:10Z

lgtm

Tarrasch · 2015-08-13T20:14:10Z

luigi/rpc.py

+    def fetch(self, full_url, body, timeout):
+        try:
+            body = urlencode(body).encode('utf-8')
+            return urlopen(full_url, body, self._connect_timeout).read().decode('utf-8')


Eh this is scary. There's no such thing as a self._connect_timeout in this context right? Is this code path really tested?

I know I've had to disable some tests on Travis. Can you make sure a normal tox -e py27-nonhdfs passes before we merge this?

Ah woops, looks like something depends on requests in your test dependencies. Will fix

graingert · 2015-08-14T13:02:40Z

@Tarrasch thanks for the review, I've made those changes.

Tarrasch · 2015-08-14T13:07:11Z

tox.ini

@@ -1,5 +1,5 @@
 [tox]
-envlist = py{27,33,34}-{cdh,hdp,nonhdfs,gcloud,postgres}, pypy-scheduler, docs, pep8
+envlist = py{27,33,34}-{cdh,hdp,nonhdfs,gcloud,postgres}{-unixsockets,}, pypy-scheduler, docs, pep8


Again, why multiplicative?

Tarrasch · 2015-08-14T13:14:36Z

Starting to look better, but I want to make sure that this part of the code hold high quality as it's so central. :)

graingert · 2015-08-14T13:24:36Z

@Tarrasch thanks!

Tarrasch · 2015-08-14T13:33:02Z

Looks good to me now :)

Tarrasch · 2015-08-14T14:33:36Z

luigi/rpc.py

-            port=self._port,
-            prefix=self._url_prefix,
-            url=url)
+        full_url = self._url + url


Erm, there's no way this line can be "right". Are the variable names extremely confusing? Or you actually string-concatenating two urls?

full_url = self._url + url_suffix?

Yea, that's much clearer.

This also adds support for TLS! Simply use https://exmaple.com/ for TLS or http+unix://%2Fvar%2Frun%2Fluigid%2Fluigid.sock/ for Unix sockets

Tarrasch · 2015-08-14T21:29:50Z

Ok, can somebody else give a somewhat careful review and then we can merge? Maybe you can add some documentation @graingert?

graingert · 2015-08-15T22:47:43Z

@Tarrasch docs added

erikbern · 2015-08-16T00:27:21Z

LGTM

Add support for Unix sockets #1118

Tarrasch · 2015-08-16T17:55:37Z

Cool, let's see how this does in the wild now :)

graingert · 2015-08-17T12:59:03Z

@Tarrasch do you know when the next release might be? Is there an official release schedule?

Tarrasch · 2015-08-17T15:20:24Z

No, it'll take a while (month?) from now I think. Unless somebody really asks for it. I'll have to release a 2.0.0 since things have changed.

paxan · 2015-08-19T02:16:43Z

luigi/rpc.py


+        self._url = url


If trailing / is present on url, fetcher gets lots of 404s for path //app/...
So it's better to do:

self._url = url.rstrip('/')

Would be nice if you could whip up a PR with testcase+fix. :)

This was possible before with the url_prefix kwargument, so I did this intentionally. But yes the other behaviour makes way more sense

@graingert I think I'm seeing this problem. Looking at the URLLibFetcher():

class URLLibFetcher(object): def fetch(self, full_url, body, timeout): try: body = urlencode(body).encode('utf-8') return urlopen(full_url, body, timeout).read().decode('utf-8') except (URLError, socket.timeout) as e: raise FetcherException(e)

For my task the full_url is http://luigi-master:8082//api/add_task, which has a double slash after the port. Changing that to a single slash seems to fix the problem.

@Tarrasch here you go:
#1147

geowurster · 2015-08-19T16:41:55Z

@graingert @Tarrasch @erikbern I think this PR may have broken something involving the worker-master communication, although it could be an issue with my cluster configuration. For reference, I'm using Google's bdutil 1.3.1 (latest) to configure and deploy a hadoop cluster on Google Compute Engine. After installing the latest commit on master my workers are not able to connect to the scheduler:

ERROR:luigi-interface:Failed connecting to remote scheduler 'http://luigi-master:8082/'
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/luigi/rpc.py", line 124, in _fetch
    response = self._fetcher.fetch(full_url, body, self._connect_timeout)
  File "/usr/local/lib/python2.7/dist-packages/luigi/rpc.py", line 71, in fetch
    raise FetcherException(e)
FetcherException
WARNING: Failed pinging scheduler
WARNING:luigi-interface:Failed pinging scheduler

I tried several different commits and I believe fb9a0a2 is the latest functional commit and 0a37979 introduced the change.

The blame shows that the object raising this exception was added in a commit in this PR. The annotated screenshot might help to resolve the timeline.

Is anyone else on GCE experiencing this behavior?

graingert · 2015-08-19T16:45:08Z

@geowurster what's the "original_exc" from the worker?

geowurster · 2015-08-19T17:13:40Z

@graingert HTTP Error 404: Not Found. The log stream contains the IP and port, which I can navigate to in a browser without issue and see the task visualizer.

graingert · 2015-08-19T17:14:33Z

that's odd

Tarrasch · 2015-08-19T19:25:44Z

@graingert Can you commit to fixing this? I think it's pretty urgent if people can't rely on HEAD anymore! Is #1147 by @paxan related?

This commit contains: * Removal of pretty much redundant tests in rpc_test * Moved tests from bin_test.py to server_test.py, as they were rendered useless after the refactoring in spotify#1129, probably forgotten since they were put in their own file in the first place. * Added test case 'test_with_cmdline' * Don't use any "random port", always use port 8083. Randomness like that leads to intermittent and irreproducible test failures. * Add a sanity check in INETURLLibServerTest that the HAS_REQUESTS mocking actually is meaningful. * Other small cleanups

graingert force-pushed the support-unix-sockets branch 2 times, most recently from 829be55 to 654387d Compare August 13, 2015 16:48

Tarrasch reviewed Aug 13, 2015
View reviewed changes

graingert force-pushed the support-unix-sockets branch 2 times, most recently from 36cf189 to 152ef30 Compare August 14, 2015 11:24

Tarrasch reviewed Aug 14, 2015
View reviewed changes

graingert force-pushed the support-unix-sockets branch from 152ef30 to f776f1c Compare August 14, 2015 13:24

graingert force-pushed the support-unix-sockets branch from f776f1c to 3b38b36 Compare August 14, 2015 13:38

Tarrasch reviewed Aug 14, 2015
View reviewed changes

Add support for Unix sockets Fixes spotify#1118

ecc591b

This also adds support for TLS! Simply use https://exmaple.com/ for TLS or http+unix://%2Fvar%2Frun%2Fluigid%2Fluigid.sock/ for Unix sockets

graingert force-pushed the support-unix-sockets branch from 3b38b36 to ecc591b Compare August 14, 2015 14:43

Document Unix sockets and TLS support

fadf282

Tarrasch added a commit that referenced this pull request Aug 16, 2015

Merge pull request #1129 from graingert/support-unix-sockets

0a37979

Add support for Unix sockets #1118

Tarrasch merged commit 0a37979 into spotify:master Aug 16, 2015

graingert deleted the support-unix-sockets branch August 16, 2015 21:10

paxan reviewed Aug 19, 2015
View reviewed changes

Tarrasch mentioned this pull request Sep 9, 2015

Log underlying RPC exception. #1200

Closed

Tarrasch mentioned this pull request Sep 13, 2015

tests: Refactor (and actually test) server tests #1214

Merged

Tarrasch mentioned this pull request Nov 17, 2015

tornado < 4 consistently makes requests to luigid fail with 500 Internal server error #1394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Unix sockets #1118 #1129

Add support for Unix sockets #1118 #1129

graingert commented Aug 13, 2015

Tarrasch commented Aug 13, 2015

graingert commented Aug 13, 2015

erikbern commented Aug 13, 2015

Tarrasch Aug 13, 2015

graingert Aug 13, 2015

graingert commented Aug 14, 2015

Tarrasch Aug 14, 2015

Tarrasch commented Aug 14, 2015

graingert commented Aug 14, 2015

Tarrasch commented Aug 14, 2015

Tarrasch Aug 14, 2015

graingert Aug 14, 2015

Tarrasch Aug 14, 2015

Tarrasch commented Aug 14, 2015

graingert commented Aug 15, 2015

erikbern commented Aug 16, 2015

Tarrasch commented Aug 16, 2015

graingert commented Aug 17, 2015

Tarrasch commented Aug 17, 2015

paxan Aug 19, 2015

Tarrasch Aug 19, 2015

graingert Aug 19, 2015

geowurster Aug 19, 2015

paxan Aug 19, 2015

geowurster commented Aug 19, 2015

graingert commented Aug 19, 2015

geowurster commented Aug 19, 2015

graingert commented Aug 19, 2015

Tarrasch commented Aug 19, 2015

Add support for Unix sockets #1118 #1129

Add support for Unix sockets #1118 #1129

Conversation

graingert commented Aug 13, 2015

Tarrasch commented Aug 13, 2015

graingert commented Aug 13, 2015

erikbern commented Aug 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert commented Aug 14, 2015

Choose a reason for hiding this comment

Tarrasch commented Aug 14, 2015

graingert commented Aug 14, 2015

Tarrasch commented Aug 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tarrasch commented Aug 14, 2015

graingert commented Aug 15, 2015

erikbern commented Aug 16, 2015

Tarrasch commented Aug 16, 2015

graingert commented Aug 17, 2015

Tarrasch commented Aug 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geowurster commented Aug 19, 2015

graingert commented Aug 19, 2015

geowurster commented Aug 19, 2015

graingert commented Aug 19, 2015

Tarrasch commented Aug 19, 2015