-
Notifications
You must be signed in to change notification settings - Fork 17
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
FreeBSD sandbox module, using jail(8) #156
Conversation
(rule | ||
(deps sandbox.runc.ml) | ||
(target sandbox.ml) | ||
(enabled_if (<> %{system} macosx)) | ||
(enabled_if (and (<> %{system} macosx) (<> %{system} freebsd))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given runc
is Linux can this rule become (enabled_if (<> %{system} linux))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be (enabled_if (= %{system} linux))
, but yes, I suppose this could work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it turns out making this change breaks a few builds (such as debian 11.4 on arm32 and powerpc64, see build report), so I'll revert that particular change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is to do with the variable expansion in dune
if you add a default sandbox.dummy.ml
with
type config = unit
let cmdliner = failwith "Sandbox not available"
and setup a copy rule for Win32
that should work. I'm not sure what is happening with the POWER build, the reported value for system
is clearly not doing what I expect.
ee8bc06
to
c0ea181
Compare
(rule | ||
(deps sandbox.runc.ml) | ||
(target sandbox.ml) | ||
(enabled_if (<> %{system} macosx)) | ||
(enabled_if (and (<> %{system} macosx) (<> %{system} freebsd))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is to do with the variable expansion in dune
if you add a default sandbox.dummy.ml
with
type config = unit
let cmdliner = failwith "Sandbox not available"
and setup a copy rule for Win32
that should work. I'm not sure what is happening with the POWER build, the reported value for system
is clearly not doing what I expect.
match config.network with | ||
| [ "host" ] -> | ||
"ip4=inherit" :: "ip6=inherit" :: "host=inherit" :: options | ||
| _ -> options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we restrict the jail networking to only access itself here?
The runc implementation overwrites hosts
with 127.0.0.1 localhost builder
https://github.com/ocurrent/obuilder/blob/master/lib/sandbox.runc.ml#L283
lib/sandbox.jail.ml
Outdated
Due to the above mutex, only one jail may be started by a given | ||
obuilder process, so appending the obuilder pid is enough to | ||
guarantee uniqueness. *) | ||
jail_name = "name=obuilder_" ^ (Int.to_string (Unix.getpid ())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to handle multiple jails concurrently from the ocluster-worker
this will need a different approach to generating the jail name and removing the mutex.
lib/sandbox.jail.ml
Outdated
|
||
let cmdliner : config Term.t = | ||
let make dummy = { dummy } in | ||
Term.(const make $ dummy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make type config = unit
then this can just become
let cmdliner : config Term.t =
Term.(const ())
@@ -106,6 +106,9 @@ module Zfs = struct | |||
| `And_snapshots -> ["-r"] | |||
| `And_snapshots_and_clones -> ["-R"] | |||
in | |||
(* At least under FreeBSD, pass the -f option to make sure any dangling | |||
devfs mount within the filesystem gets automatically unmounted. *) | |||
let opts = "-f" :: opts in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs validation on linux and macOS ZFS implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
man zfs-destroy
on macOS says:
-f Forcibly unmount file systems. This option has no effect on non-file systems or unmounted file systems.
6335928
to
f768c0c
Compare
cc37609
to
2e710de
Compare
Rebasquashed, and documentation draft added. |
CHANGES: - Add a Docker backend for Windows and Linux jobs. (@MisterDA ocurrent/obuilder#127 ocurrent/obuilder#75, reviewed by @talex5 and @tmcgilchrist) - Add FreeBSD sandbox backend using jail(8) (@dustanddreams ocurrent/obuilder#156 ocurrent/obuilder#174, reviewed by @tmcgilchrist, @MisterDA, and @mtelvers) - Add Macos ZFS sandbox (@mtelvers ocurrent/obuilder#164, reviewed by @tmcgilchrist) - Support XFS store (@mtelvers ocurrent/obuilder#170, reviewed by @tmcgilchrist) - Search for bash rather than assume it lies in /bin (@dustanddreams ocurrent/obuilder#159, reviewed by @tmcgilchrist) - Prune builds one at a time up to the limit (@mtelvers ocurrent/obuilder#157) - Specify upper bound on number of items in the store (@mtelvers ocurrent/obuilder#158, reviewed by @MisterDA) - Fix case where BTRFS is not fully allocated (@mtelvers ocurrent/obuilder#162) - Avoid pruning parent cache objects (@mtelvers ocurrent/obuilder#176, reviewed by @tmcgilchrist)
CHANGES: - Add a Docker backend for Windows and Linux jobs. (@MisterDA ocurrent/obuilder#127 ocurrent/obuilder#75, reviewed by @talex5 and @tmcgilchrist) - Add FreeBSD sandbox backend using jail(8) (@dustanddreams ocurrent/obuilder#156 ocurrent/obuilder#174, reviewed by @tmcgilchrist, @MisterDA, and @mtelvers) - Add Macos ZFS sandbox (@mtelvers ocurrent/obuilder#164, reviewed by @tmcgilchrist) - Support XFS store (@mtelvers ocurrent/obuilder#170, reviewed by @tmcgilchrist) - Search for bash rather than assume it lies in /bin (@dustanddreams ocurrent/obuilder#159, reviewed by @tmcgilchrist) - Prune builds one at a time up to the limit (@mtelvers ocurrent/obuilder#157) - Specify upper bound on number of items in the store (@mtelvers ocurrent/obuilder#158, reviewed by @MisterDA) - Fix case where BTRFS is not fully allocated (@mtelvers ocurrent/obuilder#162) - Avoid pruning parent cache objects (@mtelvers ocurrent/obuilder#176, reviewed by @tmcgilchrist)
Jail-based sandbox implementation.
Related to #109