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

Enforce Linux pids as pid_t (or i32). #3764

Merged
merged 3 commits into from Oct 13, 2017
Merged

Conversation

fnichol
Copy link
Collaborator

@fnichol fnichol commented Oct 12, 2017

While this doesn't completely solve the Launcher reaping/log issues, it tries to correct the type mismatch with how we think about process ids in Linux.

The main work was to thread though the notion of a Pid alias type which was already present in the Windows implementation which helped to keep the calling functions basically generic enough.

One small wrinkle which may not be the best is that I made the protocol level representation an int64. Why? Because in Linux we are dealing with an int32 and this fits in without overflow just fine. On Windows, the pid is a uint32 which also fits into the int64. There are only 2 issues in my mind here: 1) there's a bit of casting going into and out of the wire, but since we only ever communicate with ourselves on the same host/kernel, we always know what we expect to find (this is a bit like generics in Java) and 2) I modified an existing protocol type, but since this only happens in memory on the host, both sides upgrade in lock step.

Thoughts? Is this useful? I was using a Launch and Supervisor with this change for a week in my Builder development container when working on the builder-worker and I can confirm that I was seeing negative pid numbers with this change, and the abs(pid) was the real, correct pid of the service.

@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@fnichol
Copy link
Collaborator Author

fnichol commented Oct 12, 2017

Nice, it looks like this work pre-dated the Docker exporting work in builder-worker. @mwrock do you want me to get the CI fail going, or do you want to take it and rebase? Either way is fine by me.

@mwrock
Copy link
Contributor

mwrock commented Oct 12, 2017

I can carry it through @fnichol thanks!

fnichol and others added 2 commits October 12, 2017 15:17
Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
@mwrock mwrock force-pushed the fnichol/linux-pids-are-signed branch from 13ef345 to 5fb74e5 Compare October 12, 2017 22:33
@mwrock
Copy link
Contributor

mwrock commented Oct 12, 2017

My commit adds a solution to the launcher reaping issues. The protocol change seems innocent but I'd be curious what others think.

@christophermaier
Copy link
Contributor

tenor-264853771

Signed-off-by: mwrock <matt@mattwrock.com>
@reset
Copy link
Collaborator

reset commented Oct 13, 2017

@mwrock I think the protocol change is ok. Since it's still a 32 bit integer you'll just get integer wrapping at the worst and you're handling it on the Launcher's end. This looks good to me but we should do some testing in acceptance before we push it out :)

@reset reset merged commit a875b56 into master Oct 13, 2017
@reset reset deleted the fnichol/linux-pids-are-signed branch October 13, 2017 00:45
@eeyun eeyun added I-linux and removed P-linux labels Mar 6, 2018
@christophermaier christophermaier added Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component and removed A-supervisor labels Jul 24, 2020
@christophermaier christophermaier added Focus:Launcher Related to the Habitat Launcher (core/hab-launcher) component Platform: Linux Deals with Linux-specific behavior Platform: Windows Deals with Windows-specific behavior Type: Chore Issues for general code and infrastructure maintenance and removed A-launcher labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus:Launcher Related to the Habitat Launcher (core/hab-launcher) component Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Platform: Linux Deals with Linux-specific behavior Platform: Windows Deals with Windows-specific behavior Type: Chore Issues for general code and infrastructure maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants