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

config-vm: Recycle the hook-process schema #962

Closed

Conversation

wking
Copy link
Contributor

@wking wking commented Apr 2, 2018

We've had two ways to specify a process to launch (for the container process and for hooks) for a while now. Last week, we landed #949 with a third process-launching structure. This pull request replaced that third structure with a recycled hook-process schema for launcing the hypervisor. I prefer using the more-fundamental container process schema, but @crosbymichael felt it had too many properties that wouldn't apply to the hypervisor-launching case. Versus master, this pull request adds:

  • The ability to set argv[0] independently of path (like execv and similar).
  • The ability to set env and timeout for the hypervisor process.
  • The ability to lean on existing hook-launching code for launching hypervisor processes, with the difference being just whether you pass the state in on stdin or not.

And removes:

If we get this or a similar unification landed before we cut a release that includes #949, then I think this change is all positive with no downside. If we punt on this until after 1.1.0, then this is probably just going to end up as one of those unfortunate historical quirks that are not worth the migration pain of fixing.

Ping @sameo and @crosbymichael, since they're the only two who have expressed opinions on this sort of thing so far.

So we'll be able to link to it without linking the rest of the 'hooks'
structure.

Signed-off-by: W. Trevor King <wking@tremily.us>
We already have two ways to specify a process to launch (for the
container process and for hooks).  This commit recycles the
hook-process schema for launching the hypervisor.  I prefer using the
more-fundamental container process schema, but Michael felt it had too
many properties that wouldn't apply to the hypervisor-launching case
[1].

[1]: opencontainers#949 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

house keeping: i'm inclined to close this for now. While I am all for re-using schema, this slight change imposes a change and hitching VM intricacies to the other hooks (which sounds fine, but may not align to ongoing reality)

@vbatts vbatts closed this Dec 17, 2019
@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

(decided to close, since the neither of the two reviewers have chimed in on this in 1.5yrs...)

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

Successfully merging this pull request may close these issues.

2 participants