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

Add support for NetBSD #18562

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Add support for NetBSD #18562

merged 2 commits into from
Oct 27, 2023

Conversation

darkgeek
Copy link
Contributor

Hi,

I want to add some code to make Nomad build and run on NetBSD.

In this PR, allocdir/fs_netbsd.go and host/netbsd.go are used for NetBSD only, while the df part in host/unix.go is reworked, which might affect other Linux/Unix: instead of leveraging on the frozen syscall package, where Statfs is not available on NetBSD, it might be better to switch to gopsutil, which supports more platforms (currently it doesn't support NetBSD though, I have also sent a PR shirou/gopsutil#1530 to it).

Can you check this PR to see if it's decent enough to be merged? Thanks.

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 23, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @darkgeek! Thanks for opening this PR. A few comments other than the remarks I've left inline:

  • Does this need for gopsutil to cut a tag with Add some support for NetBSD shirou/gopsutil#1530, and then for Nomad to pick up that dependency bump in the go.mod?
  • Is this actually enough to run workloads? What task drivers are you using to verify this?

client/allocdir/fs_netbsd.go Outdated Show resolved Hide resolved
Comment on lines 4 to 5
//go:build netbsd
// +build netbsd
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we're missing this same function for other non-Linux/Darwin Unixes (which makes me wonder if any of the BSDs can build?). Should this be a unix build tag instead of a netbsd build tag?

Copy link
Contributor Author

@darkgeek darkgeek Oct 25, 2023

Choose a reason for hiding this comment

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

OK, I deleted darwin.go, linux.go, netbsd.go, and moved the mountedPaths() function to unix.go with unix build tag in this commit: 1c61ce3.

Then I tried on Linux and NetBSD, it built and ran well like before. Though my tests are quite limited.

@darkgeek
Copy link
Contributor Author

darkgeek commented Oct 25, 2023

Hi @darkgeek! Thanks for opening this PR. A few comments other than the remarks I've left inline:

Yes, my PR has been merged, and gopsutil already has NetBSD support since the v3.23.9 tag, so you might consider bumping to this version.

  • Is this actually enough to run workloads? What task drivers are you using to verify this?

It's enough to (and only can) run raw_exec workloads perfectly on my NetBSD aarch64 box:

mmexport1698246787648
mmexport1698246801904
mmexport1698246803354
mmexport1698246805749

@tgross
Copy link
Member

tgross commented Oct 26, 2023

Yes, my PR has been merged, and gopsutil already has NetBSD support since the v3.23.9 tag, so you might consider bumping to this version.

Ok, but in which case we should do that in this same PR.

@darkgeek
Copy link
Contributor Author

darkgeek commented Oct 26, 2023

Yes, my PR has been merged, and gopsutil already has NetBSD support since the v3.23.9 tag, so you might consider bumping to this version.

Ok, but in which case we should do that in this same PR.

OK, I did that in the commit b1cd0a9, and have tested on my NetBSD box.

@tgross
Copy link
Member

tgross commented Oct 26, 2023

@darkgeek you've got some merge conflicts here you'll need to resolve.

Currently, the `raw_exec` is the only usable driver.
@darkgeek darkgeek force-pushed the netbsd-arm64-port-v2 branch from 878e280 to b1cd0a9 Compare October 27, 2023 08:49
@darkgeek
Copy link
Contributor Author

@darkgeek you've got some merge conflicts here you'll need to resolve.

Resolved by doing rebase on main. And I also squash my previous commits to a single one.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

I've tested this out on Linux and I've had one of my colleagues who's on macOS give this a spin just to make sure nothing got broken there. I've added a changelog entry as well. Once this is green in CI we'll merge it and it'll go out in the upcoming Nomad 1.7.0 release.

Thanks @darkgeek!

@tgross
Copy link
Member

tgross commented Oct 27, 2023

The test-ui/finalize failure is because of some weird CI issue we have around not exposing the token for Percy to external contributors. That needs to get fixed but isn't a blocker for you here (especially as this isn't a UI PR 😀 )

@tgross tgross merged commit b76e042 into hashicorp:main Oct 27, 2023
20 of 21 checks passed
@tgross tgross added this to the 1.7.0 milestone Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants