I think I'd prefer to share code where possible. Even if the readibility suffers, it will reduce the liklihood we miss patches to one or the other

I think I'd prefer to share code where possible. Even if the readibility suffers, it will reduce the liklihood we miss patches to one or the other

The approach I took in Podman for some similar changes was to move e.g. container_create_linux.go to container_create_common.go and then iteratively factor out things which didn't make sense for FreeBSD back to container_create_linux.go until I got to a place where both platforms build. I can do something similar here and see how that turns out.

I re-implemented this to keep as much of the code for container creation shared between Linux and FreeBSD. I kept the history fairly detailed to make it easier to see the sequence of small refactors clear.

Codecov Report

Attention: Patch coverage is 39.20530% with 459 lines in your changes missing coverage. Please review.

Project coverage is 47.50%. Comparing base (403bef8) to head (8287e41).
Report is 18 commits behind head on main.

@@            Coverage Diff             @@
##             main    #7727      +/-   ##
==========================================
+ Coverage   47.49%   47.50%   +0.01%     
==========================================
  Files         154      156       +2     
  Lines       23125    23132       +7     
==========================================
+ Hits        10983    10989       +6     
- Misses      11072    11073       +1     
  Partials     1070     1070              

A friendly reminder that this PR had no activity for 30 days.

Note: this iteration disables timezone support for FreeBSD - this can be removed if we update the vendor of github.com/containers/common to v0.58 but perhaps that should happen in a separate PR?

@dfr, you might have to rebase this against main as there have been changes that will affect your work here.

@dfr, you might have to rebase this against main as there have been changes that will affect your work here.

I just rebased and updated the PR, thanks!

there do seem to be some legitimate breakages here

@dfr, could you rebase on top of the main branch to pick the container/common changes?

@dfr Thanks for your patience.
Could you fix this test?
https://github.com/cri-o/cri-o/actions/runs/13288331349/job/37102473662?pr=7727

@dfr Thanks for your patience. Could you fix this test? https://github.com/cri-o/cri-o/actions/runs/13288331349/job/37102473662?pr=7727

Done.

It's not clear to me what should be working with this PR.
How about adding an integration test for freebsd?

There's no default runner, but there seem some 3rd party options.
actions/runner#385

I think it's ok to disable some tests, but we can get better understanding what is remaining to fully support freebsd.

It's not clear to me what should be working with this PR. How about adding an integration test for freebsd?

There's no default runner, but there seem some 3rd party options. actions/runner#385

I think it's ok to disable some tests, but we can get better understanding what is remaining to fully support freebsd.

I think that at this stage, I would be happy with a simple 'does it build' test. We did this in Podman and it has been quite helpful - this was easy in Podman since they use Cirrus CI which supports FreeBSD. I'm not sure what it would take to add a 3rd party FreeBSD runner.

hmm, did you do any manual test on this change? or is this PR just a preliminary one for the future changes?
I suppose CreateContainer should succeed probably.

I think that at this stage, I would be happy with a simple 'does it build' test.
I'm not sure what it would take to add a 3rd party FreeBSD runner.

I believe we should have the e2e test for the freebsd functionality as soon as possible, but you made the point about 3rd party runner. we may as well not implement it for now.

ideally, https://actuated.com/blog/kvm-in-github-actions should be the target. i.e. on linux, we deploy a minimal freebsd VM using kvm and do podman build -t freebsd-helloworld -f Dockerfile.freebsd-test. that will resonate with larger community out in the wild.

hmm, did you do any manual test on this change? or is this PR just a preliminary one for the future changes? I suppose CreateContainer should succeed probably.

I have a small set of tests which use crictl to create pods and containers in a few configurations that cover creating simple containers, containers with volumes, etc. and I run them manually. I also have a two node FreeBSD Kubernetes cluster that uses cri-o with this PR and a few much smaller patches that I can use for ad-hoc testing.

I think that at this stage, I would be happy with a simple 'does it build' test.
I'm not sure what it would take to add a 3rd party FreeBSD runner.

I believe we should have the e2e test for the freebsd functionality as soon as possible, but you made the point about 3rd party runner. we may as well not implement it for now.

If we can collectively figure out a way to run tests in a FreeBSD VM, I would be very happy. Possibly https://github.com/marketplace/actions/cross-platform-action would work for this?

I just remembered that I actually added a FreeBSD cross build which runs on a Linux worker and helps to keep the tree buildable. I will see if this binary is functional, which it should be unless something in the build depends on CGO.

@dfr
I created the issue for FreeBSD CI. #9005
Aside from that, as you tested it already manually, I feel ok to merge it.
Once you verified the binary, please let us know.

/lgtm
/hold

Feel free to remove the hold when it's ready. Thanks for being so patient and thorough here. I'm very excited about this!

/lgtm /hold

Feel free to remove the hold when it's ready. Thanks for being so patient and thorough here. I'm very excited about this!

/unhold

After some more testing, including with my experimental FreeBSD Kubernetes cluster, I'm happy with this change. I will follow up with a few much smaller pull requests to fill out the features needed for minimal Kubernetes support.

/retest-required

seems like unrelated timeouts and not related to these changes?

seems like unrelated timeouts and not related to these changes?

I think so - it usually takes a while to get this set of tests to pass. Unfortunately, they are important end-to-end tests ensuring that we can still bring up Kubernetes clusters etc. so we need them. I do wish they were less flaky though.

/retest