-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Use ps command that prints full command lines without errors #1809
Conversation
@nugend, thanks for your PR! By analyzing the annotation information on this pull request, we identified @daveFNbuck, @erikbern and @steenzout to be potential reviewers |
LGTM |
@@ -42,7 +42,7 @@ def getpcmd(pid): | |||
_, val = lines | |||
return val | |||
else: | |||
cmd = 'ps -xo pid,args' | |||
cmd = 'ps x -wwo pid,args' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a couple of lines of comments above this line as you find necessary? Example coming to my mind:
- What OS versions this have been tested for
- Links to various online man-pages for various OSes
- Summarize what behaviour these set of parameters give and how it fits with luigi.
I mean, this is fully optional, I only want you to include what you think will make it easier for the next person dealing with this code line.
LGTM. But consider adding some docs. :) |
Sorry, I don't really have much time for a few weeks and I understand that this is a very hazardous sort of patch. Maybe you could solicit some other contributors to test out the command on various OS's? |
I tested it on my Ubuntu servers and OSX laptop. It matches the man pages for those too. |
Okay. I merge this to fix the bug. But I would appreciate if we could follow up where you document your research efforts as comments in the code. :) |
Unfortunately this fix doesn't work well on busybox (e.g. Alpine Linux) because it uses the 'w' option that busybox's ps doesn't support. (similar to #1530 ) |
@keitap, any solution that works sounds fine for me ^^ |
I think this is an important functionality. But it would be nice if we could avoid a dependency. :) |
Ok, well, should |
Oh, yea maybe procfs is the right thing here. I for one don't know. :) |
@daveFNbuck @nugend @keitap @erikbern, I have some alarming experience regarding the locking. I don't remember how slot it was before this patch. But I've upgraded to the latest version of luigi. And I experience that startups (just the lock checking!) takes 3 minutes. This needs a fix soon! We should think if we can use procfs or some other algorithm for this. Alternatively find out why this is slow. I could experiment with this a bit further. But I would like to hear from you if you've seen any regressions. |
Hmm... this seem to be because my |
Oh this seems to be for many many files.
This is clearly unrelated to this patch I see. Sorry for alarming. Though I'll work on getting something up that cleans the |
Description
Based on errors with the existing command, this should behave correctly
Motivation and Context
Fixes #1808
Have you tested this? If so, how?
Tested on colleagues machine. Tested command on our machine. Would need external validation about behavior with ps on more operating systems/versions.