Skip to content

[ Runtime ] Allow for excluding pid from state #459

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[ Runtime ] Allow for excluding pid from state #459

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions runtime.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ There is no requirement that it be unique across hosts.
The ID is provided in the state because hooks will be executed with the state as the payload.
This allows the hooks to perform cleanup and teardown logic after the runtime destroys its own state.
* **`pid`**: (int) is the ID of the main process within the container, as seen by the host.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could change all state fields phrased like:
pid: (int, OPTIONAL) ...
like we did in all other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could change all state fields phrased like: pid: (int, OPTIONAL)...

I have that in flight with #507. I'll pull that apart to isolate it from the creating change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have that in flight with #507. I'll pull that apart to isolate it from the creating change.

Pulled off into #664 (ended up being just “cherry-pick the tip commit from #507”).

The pid MAY be excluded or set to a value indicating empty or null if it is not seen by the host.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather phrase this as:

  • pid (int, optional) is the ID ….
    This property is REQUIRED on the following platforms: Linux, …

although that's tied up in specifying platforms for VM-based containers.

Copy link
Member

Choose a reason for hiding this comment

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

This phrasing looks wrong to me as well (especially "seen by the host"). However, I would also want that we specify what an "empty or null" value means for a PID. Should we make it a pointer? Or just make it 0 to indicate "empty"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pointers are our usual pattern for optional fields. I don't think a zero PID is clearly enough “no-op in the spec” to qualify for the non-pointer-ok exception listed there.

* **`bundlePath`**: (string) is the absolute path to the container's bundle directory.
This is provided so that consumers can find the container's configuration and root filesystem on the host.

Expand Down