runtime-spec
runtime-spec copied to clipboard
opencontainers
Add FreeBSD as a platform
Fixed the type of FreeBSDDevice.Mode and fixed a typo in the json mapping for FreeBSDJail.SysVShm.
I took another pass over this today and made a couple of minor changes. I think the only current open question is whether the Mapping from jail(8) config file section is useful or whether it should be removed.
Does anyone else have comments or suggestions for the FreeBSD runtime extension? What should our next steps be to make this acceptable for the runtime spec?
@dfr we attended the developer meeting but it was Juneteenth and the attendance was relatively low. They suggested that you ping Tianon and Sam to ask for them to review it. If that doesn't work, we can try to attend the dev call again.
I've no specific comments on the FreeBSD part, but I can help to move this forward.
I've left an inline comment
Thanks! I will check out your comment on the PR but my internet connection is flaky at the moment which is slowing me down a bit (hopefully that will be fixed over the weekend).
This looks pretty decent to me, although admittedly I'm not super familiar with FreeBSD / jails internals.
If we're not in a hurry to merge, I'd love to wait until @samuelkarp has a chance to review too, but if we've got something urgent that needs this, I think it's probably in a reasonable state (and hopefully he doesn't disagree).
It looks like Sam is going to be away for a while focusing on family matters and he is unlikely to be able to give feedback in the near future. He was involved in the working group which led to this pull request and was a key reviewer during that process if that helps. As far as I know, he has not read through the exact text from this pull request.
Timing wise, I am not in a great hurry. It would be great if it makes it into the 1.3.0 release if at all possible.
I just pushed a small update which adds support for jail's allow.mlock capability. This has been requested by potential users and (I think) is required by postgresql.
I started trying to migrate runj to this and immediately ran into challenges with vnet interfaces. Are we expecting to require CNI in order to do networking? That seems at odds with the work that was just merged for Linux in #1271.
The interface added in #1271 looks useful. Currently it seems to be Linux-specific and I'm not sure how it handles the firewall rules which are typically added for NAT and port forwarding.
Currently it seems to be Linux-specific
Yes, but we have equivalent FreeBSD-specific capabilities provided by the jail interface in the kernel and we are defining the FreeBSD-specific config section here.
I'm not sure how it handles the firewall rules which are typically added for NAT and port forwarding.
It doesn't. This is also true for the jail interface in FreeBSD; that's left up to things like pf.
I think I'm currently :-1: on this PR as-is. There are two known OCI jail implementations (well, they're not currently OCI given the spec doesn't have FreeBSD support yet, but they are planned to be): runj and ocijail. The config defined here does not satisfy the requirements of runj as it exists today.
I think it'd be a reasonable requirement from the spec maintainers to say that there should at least be a POC PR that demonstrates the proposed changes working in both runj and ocijail before merging. I'm tracking that for runj in https://github.com/samuelkarp/runj/issues/60 and welcome contributions to that (I also have some WIP code on my laptop, but my time here is extremely limited right now).
Currently it seems to be Linux-specific
Yes, but we have equivalent FreeBSD-specific capabilities provided by the jail interface in the kernel and we are defining the FreeBSD-specific config section here.
I'm not sure how it handles the firewall rules which are typically added for NAT and port forwarding.
It doesn't. This is also true for the jail interface in FreeBSD; that's left up to things like pf.
I still think that CNI etc. adds value for some use-cases but it certainly doesn't hurt to add some support for simple use-cases where we just need to add aliases to a loopback interface and allow the jail to use them. This is a very common pattern for jail users and has been requested by others. I will add support for specifying addresses and/or interfaces in this pull request.
I think I'm currently 👎 on this PR as-is. There are two known OCI jail implementations (well, they're not currently OCI given the spec doesn't have FreeBSD support yet, but they are planned to be): runj and ocijail. The config defined here does not satisfy the requirements of runj as it exists today.
I think it'd be a reasonable requirement from the spec maintainers to say that there should at least be a POC PR that demonstrates the proposed changes working in both runj and ocijail before merging. I'm tracking that for runj in samuelkarp/runj#60 and welcome contributions to that (I also have some WIP code on my laptop, but my time here is extremely limited right now).
Sam, that is a very good point. I do have a branch in ocijail which tracks the progress of this PR but its currently private. I'll bring that up to date with the latest iteration of the PR and make it public on github.
We're happy to test and provide feedback on both ocijail and runj when it gets support
I think I'm currently 👎 on this PR as-is. There are two known OCI jail implementations (well, they're not currently OCI given the spec doesn't have FreeBSD support yet, but they are planned to be): runj and ocijail. The config defined here does not satisfy the requirements of runj as it exists today. I think it'd be a reasonable requirement from the spec maintainers to say that there should at least be a POC PR that demonstrates the proposed changes working in both runj and ocijail before merging. I'm tracking that for runj in samuelkarp/runj#60 and welcome contributions to that (I also have some WIP code on my laptop, but my time here is extremely limited right now).
Sam, that is a very good point. I do have a branch in ocijail which tracks the progress of this PR but its currently private. I'll bring that up to date with the latest iteration of the PR and make it public on github.
I pushed my OCI development branch to github here.
I think I'm currently 👎 on this PR as-is.
Seems this PR should be deferred to the v1.4.0 milestone?
- https://github.com/opencontainers/runtime-spec/issues/1295
I updated the pull request to address to address @samuelkarp's concerns about container IP addresses and added an implementation to my ocijail development branch (here). I still need to implement the proposed vnetInterfaces field.
@AkihiroSuda I certainly don't want to hold up the 1.3 release if the timing for that is running out but I do feel that we are close to agreement on this approach to OCI runtime support on FreeBSD.
@dfr some comments about timing in https://github.com/opencontainers/runtime-spec/issues/1295 which I don't quite understand, but it may be my lack of experience :/
@dfr this needs a rebase
I just updated the pull request, rebasing and addressing @samuelkarp's review comments.
LGTM overall, left a couple of nits (and opened #1298 to fix a pre-existing issue).
Since this PR moves FileMode from defs-linux.json to defs.json, I will pre-emptively copy the changes in defs-linux.json from #1298 in anticipation of that change being merged.
LGTM overall, left a couple of nits (and opened #1298 to fix a pre-existing issue).
Since this PR moves FileMode from
defs-linux.jsontodefs.json, I will pre-emptively copy the changes indefs-linux.jsonfrom #1298 in anticipation of that change being merged.
You can just rebase your patches on top of #1298 (this will make those patches part of this PR) and mark the PR as draft until that other PR (#1298) is merged. Once it's merged, rebase again.
Or just wait for it to be merged (should happen in a couple of days I hope) and rebase.
Needs rebase
I will take care of that today. I also have small changes to add jail(8)'s interface pseudo-parameter and to add a few fields to the interface schema which were missing.
cyphar modified the milestones
@cyphar Aren't we ready to merge this in the spec v1.3.0 ? (which will correspond to runc v1.4.0)
Ah okay, I was going off the discussion we had a few weeks ago, I didn't notice that we'd resolved the outstanding issues. I'm okay to merge this then.
... merging with open/active discussion and only two approvals on a fairly large addition seems kind of premature? 🤷
I was hoping to get more information about whether the runj approach was acceptable to @dfr but as I said, I don't have a strong opinion on it. I'm a bit more confused why there was a perceived rush to merge this for 1.3 (we want 1.3 for runc 1.4 -- though I also don't think this should be a hard requirement -- but the FreeBSD stuff is not really relevant for that either).
At the end of the day, I'm fine with the merged version.
Sorry, I am a little late to this part of the discussion and failed to reply before this was merged.
I have been thinking a lot on the suggested structure which splits out the ip4 and ip6 parameters into sub-objects. I don't think this adds much in readability - in particular, the way it moves the namespace parameters (e.g. vnet=new, ip4=inherit) could be confusing given that the other namespace parameters still live in the jail object.
The underlying jail parameter set has no sub-structure - its just a list of key/value pairs where some of the keys names happen to have "." characters in them, e.g. ip4.addr. This is why I had a single jail object in the runtime-spec proposal which is (mostly) a simple 1-1 mapping, renaming some keys to fit with the style of other parts of runtime-spec.
Personally, I am happy with the merged version as it represents the consensus approach for our working group and has benefitted from this review process. If there are any concerns regarding the open discussion points, perhaps this can be addressed in followup pull requests if there is time before 1.3.
Sorry for the rush, feel free to open follow-up issues (or PRs) for remaining concerns 🙇