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

create: add support for --bundle and --pid-file #27

Closed
wants to merge 1 commit into from

Conversation

dfr
Copy link
Contributor

@dfr dfr commented Apr 23, 2022

Issue number:
#26

Description of changes:
This adds the commonly used --bundle and --pid-file options to create, while preserving the existing support for supplying the bundle path as a positional argument. With this, I was able to get my experimental freebsd port of buildah to use runj as its OCI runtime.

Testing done:
Adhoc testing with my hacked buildah, also testing that the containerd shim still works with this change

Terms of contribution:
By submitting this pull request, I agree that this contribution is licensed under the terms found in the LICENSE file.

@dfr
Copy link
Contributor Author

dfr commented Apr 24, 2022

I think the failing check is a problem with CI, not with this change:

make
go build -o bin/runj ./cmd/runj
go: missing Git command. See https://golang.org/s/gogetcmd
error obtaining VCS status: exec: "git": executable file not found in $PATH
	Use -buildvcs=false to disable VCS stamping.

@samuelkarp samuelkarp added the enhancement New feature or request label Apr 26, 2022
@emaste
Copy link
Contributor

emaste commented May 5, 2022

I opened #28 to fix CI

@emaste
Copy link
Contributor

emaste commented May 5, 2022

When I try this branch in CI with #28 applied I get

=== RUN   TestRenderConfigBasic
    conf_test.go:19: 
        	Error Trace:	conf_test.go:19
        	Error:      	Not equal: 
        	            	expected: "basic {\n  path = \"/tmp/test/basic/root\";\n  persist;\n}\n"
        	            	actual  : "basic {\n  path = \"/tmp/test/basic/root\";\n  ip4 = inherit;\n  ip6 = inherit;\n  allow.raw_sockets;\n  persist;\n}\n"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -2,2 +2,5 @@
        	            	   path = "/tmp/test/basic/root";
        	            	+  ip4 = inherit;
        	            	+  ip6 = inherit;
        	            	+  allow.raw_sockets;
        	            	   persist;
        	Test:       	TestRenderConfigBasic
--- FAIL: TestRenderConfigBasic (0.00s)
FAIL
FAIL	go.sbk.wtf/runj/jail	0.007s
?   	go.sbk.wtf/runj/oci	[no test files]
?   	go.sbk.wtf/runj/runtimespec	[no test files]
?   	go.sbk.wtf/runj/state	[no test files]
FAIL
*** Error code 1

@dfr
Copy link
Contributor Author

dfr commented May 5, 2022 via email

@dfr
Copy link
Contributor Author

dfr commented May 13, 2022

It turns out that --bundle and --pid-file are required by the OCI Runtime CLI specification: https://github.com/opencontainers/runtime-tools/blob/master/docs/command-line-interface.md

@samuelkarp
Copy link
Owner

Hey @dfr, that repository is a collection of tools for working with the specification, not the specification itself. You can find the spec here: https://github.com/opencontainers/runtime-spec.

@dfr
Copy link
Contributor Author

dfr commented May 19, 2022

The document I linked seems to be part of the 'Runtime compliance testing' tools and https://github.com/opencontainers/runtime-tools/blob/master/docs/runtime-compliance-testing.md includes the words:

In order to be tested for compliance, runtimes MUST support at least one of the following APIs:

Version 1.0.1 of the OCI Runtime Command Line Interface.

It seems counter intuitive (to me anyway) that each runtime can make its own choices for how to define the interface and this document in runtime-tools seems to suggest that the compliance testing process requires a core subset of the CLI interface.

@samuelkarp
Copy link
Owner

I agree that it's confusing, but the runtime-tools repository is not part of the spec. The OCI does not currently run a compliance program for the runtime spec (and has only recently started a conformance program for the distribution spec).

@dfr
Copy link
Contributor Author

dfr commented Jun 4, 2022

I rebased this to pick up @emaste's CI fix

&bundle,
"bundle",
"",
`path to the root of the bundle directory, defaults to the current directory`)
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have a specific need for runj to support CWD here as the default or are you more interested in just adding support for --bundle? CWD does not currently work with your PR (the jail utility requires an absolute pathname and there's no current code to compute that).

I'm going to remove support for CWD for now while I'm working on this code and we can add it back in a separate PR if you need it.

@samuelkarp
Copy link
Owner

I've merged this (with some minor modifications) as 889c370. Thank you for the contribution!

@samuelkarp samuelkarp closed this Jun 7, 2022
@dfr
Copy link
Contributor Author

dfr commented Jun 7, 2022 via email

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

Successfully merging this pull request may close these issues.

3 participants