Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

Close file descriptors when forking subprocess to reload haproxy #29

Closed
wants to merge 1 commit into from

Conversation

cgbaker
Copy link

@cgbaker cgbaker commented Dec 3, 2015

References #28

@brndnmtthws
Copy link
Contributor

Makes sense, thanks!

@brndnmtthws
Copy link
Contributor

Hmm, upon closer inspection, I see that close_fds=True is the default in Python 3: https://docs.python.org/3/library/subprocess.html#popen-constructor

However, close_fds=False is the default in Python 2: https://docs.python.org/2/library/subprocess.html#popen-constructor

Since marathon-lb.py is now using Python 3, I don't think we need this change. Thoughts?

@cgbaker
Copy link
Author

cgbaker commented Dec 4, 2015

Excellent point; I didn't notice that that the interpreter hint had changed to python3. Does the script still work with python2? If so, an explicit invocation, a la
python2 servicerouter.py
would still trigger the effect addressed in #28.

If it's not necessary for marathon-lb, then it seems we can close it.

I had thought (mistakenly) that the servicerouter utility was being moved out of the main marathon repo (it's not in master anymore), but I see that it's still part of the distribution in the 0.12 and 0.13 releases, where it's still coding against python2. Should I refile this over in the marathon project and get these changes integrated there? I'm not sure what the long-term plan is for this utility.

@brndnmtthws
Copy link
Contributor

This is indeed the successor to servicerouter.py. It was only recently removed from the repo, however, so it probably won't be absent until the next release.

@cgbaker
Copy link
Author

cgbaker commented Dec 4, 2015

So, as the current version of servicerouter doesn't work with python 2 (because of the urllib.parse requirement) and the proposed change isn't necessary for python3, I agree that this can be closed. I'll go ahead and close it.

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

Successfully merging this pull request may close these issues.

2 participants