FreeBSD Support #126
FreeBSD Support #126
Conversation
Initial implementation of HostInfo for FreeBSD platforms.
On some platforms (e.g. FreeBSD) user home directories are symlinks and os.Getcwd will return the symlink path. On FreeBSD CWD for processes that's not the running process will not give the symlink but the resolved path causing the test to fail at this point. Fix is to use the os.SameFile call to resolve both Os.Getcwd and CWD.
Implement Process(), Self() and Processes() to the bare minimum. That is passing all basic tests.
- Fix issue on FreeBSD 12. - Minor cosmetic issues.
- Remove cgo build directives from things not needing it. - Add linker flags into .go files.
|
💚 CLA has been signed |
|
@dbolcsfoldi Can you sign the agreement to get your commits in? |
❕ Build Aborted
Expand to view the summary
Build stats
Steps errors
Expand to view the steps failures
|
|
Hi @redanthrax, A small fix for the Environment stuff. To be sure this also works in a jail I ran the test from there. It works ok, but my shell had an odd environment variabele the test tripped over: fixed by diff --git a/providers/freebsd/process_freebsd.go b/providers/freebsd/process_freebsd.go
index 117d56c..aa47a53 100644
--- a/providers/freebsd/process_freebsd.go
+++ b/providers/freebsd/process_freebsd.go
@@ -125,7 +125,7 @@ func makeMap(from []string) map[string]string {
out := make(map[string]string, len(from))
for _, env := range from {
- parts := strings.Split(env, "=")
+ parts := strings.SplitN(env, "=", 2)
if len(parts) > 1 {
out[parts[0]] = parts[1]
}Best Regards, |
|
Hope this gets (CLA-signed and) committed soon, as this is a show-stopper for beats 8 support on FreeBSD. |
|
Did this move forward, please? |
providers/freebsd/memory_freebsd.go
Outdated
|
|
||
| func SwapMaxPages() (uint32, error) { | ||
| var maxPages uint32 | ||
| if err := sysctlByName(hwPhysmemMIB, &maxPages); err != nil { |
There was a problem hiding this comment.
For the record, it's vm.swap_maxpages OID.
|
In the year of our lord 2024 I have given up that this will ever be considered. |
|
Yeah, I have also given up and resorted to setting up a personal fork with this merged 🤷 |
|
Hello all! I signed the CLA - 2024 will be the year! |
|
I opened #204 to add CI testing for FreeBSD. |
Add GH action step test execute unit tests on FreeBSD 14.0. The tests run inside of a qemu VM running on top of a linux worker. Until a FreeBSD provider is present this step will test nothing. It will become active as soon as files exist in 'providers/freebsd/'. Relates: #126 --------- Co-authored-by: Victor Martinez <[email protected]>
[git-generate] gofumpt -w --extra $(find . -name '*.go')
providers/freebsd/host_freebsd.go:56:9: cannot use newHost() (value of type *host) as types.Host value in return statement: *host does not implement types.Host (missing method FQDN)
The |
There was a problem hiding this comment.
I've had a light look at the C and it appears sane. It would be good to have @haesbaert take a look at this.
| if len(r.errs) > 0 { | ||
| return errors.Join(r.errs...) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
| if len(r.errs) > 0 { | |
| return errors.Join(r.errs...) | |
| } | |
| return nil | |
| return errors.Join(r.errs...) |
There was a problem hiding this comment.
Apologies, but I don't understand the reasoning behind this change. The other providers are using identical methods.
There was a problem hiding this comment.
The reason is that it reduces the line count. errors.Join returns nil if there is no non-nil error in the parameter list. I suspect that the other cases look the way they do because the were written before errors.Join existed.
| return | ||
| } | ||
| m.Metrics = make(map[string]uint64, 6) | ||
| m.Metrics["active_bytes"] = uint64(activePages) * pageSize |
There was a problem hiding this comment.
ISTM that all these conversions should happen in the helpers that are providing the values. If the helpers were methods on *reader then we would not need to consider errors or conversions here (see comment on L132).
There was a problem hiding this comment.
I moved the integer casts into their respective helper methods. Will this be sufficient, or do you prefer I move the helper methods onto *reader as well?
EDIT: I can also simply pass *r into the helpers as an argument (see below).
| if r.addErr(err) { | ||
| return | ||
| } | ||
| pageSize := uint64(ps) |
There was a problem hiding this comment.
Can we not be optimistic here? At the moment, any single non-implemented collection means all subsequent collections are not performed. Is this guaranteed to be the behaviour we want? The alternative is to use the approach that is used for collections used in the linux newHost func where all are optimistically collected and then the aggregated errors are returned along with all the data that was obtained.
There was a problem hiding this comment.
I can either move the helpers onto *reader (pointer receiver) or we can also do this if permitted:
func totalPhysicalMem(r *reader) uint64 {
const mib = "hw.physmem"
v, err := unix.SysctlUint64(mib)
if r.addErr(err) {
return 0
}
return v
}EDIT: redanthrax@bbb75c4
There was a problem hiding this comment.
That looks OK. It reads a bit like C, but I guess we are working close to that anyway here.
|
|
||
| var _ registry.HostProvider = freebsdSystem{} | ||
|
|
||
| func TestHost(t *testing.T) { |
There was a problem hiding this comment.
Can we test for the presence of some keys?
There was a problem hiding this comment.
Can you please elaborate? e.g. check that we're actually on a FreeBSD system?
There was a problem hiding this comment.
At the moment, the only thing that is being tested without human intervention is that the calls return without error. The results could all be empty or non-sane, but the only way we can know this is if a human reads the test logs. We could probably do better at least by checking that some of the expected values (keys) are present.
There was a problem hiding this comment.
Understood. I added additional validations to the tests.
Extracted code from the updated PR:
// TestArchitecture -- I can also validate with `uname -m` if you prefer
assert.Regexp(t, `(amd64|i386|powerpc|arm(64)?|riscv|mips|sparc64|pc98)`, arch)
// TestKernelVersion
cmd := exec.Command("/bin/freebsd-version", "-r")
// TestMachineID -- I know Regex is slow here, but trying to avoid extra packages
assert.Regexp(t, "^[a-zA-Z0-9]{8}-[a-zA-Z0-9]{4}-[a-zA-Z0-9]{4}-[a-zA-Z0-9]{4}-[a-zA-Z0-9]{12}$", machineID)EDIT: revised architectures list (mistakened a few from MACHINE_ARCH values)
providers/freebsd/sysctl_freebsd.go
Outdated
| const mib = "kern.cp_time" | ||
| buf, err := unix.SysctlRaw("kern.cp_time") |
There was a problem hiding this comment.
| const mib = "kern.cp_time" | |
| buf, err := unix.SysctlRaw("kern.cp_time") | |
| const mib = "kern.cp_time" | |
| buf, err := unix.SysctlRaw(mib) |
| t.Fatal(err) | ||
| } | ||
|
|
||
| // Apply a sanity check. This assumes the host has rebooted in the last year. |
There was a problem hiding this comment.
Can we shell out to uptime and get a value to use?
There was a problem hiding this comment.
I created a PR on the forked repo that addresses your request (awaiting approval). It's a bit overkill for a test, but there's no other structured way to get the uptime apart from sysctl or adding more CGO.
There was a problem hiding this comment.
That looks reasonable after clean-up; not overkill at all.
|
Can someone tackle the review comments? |
Requested upstream changes
|
I Guess we need @sarog to sign the cla |
I did sign the CLA. Is the approval automated or must someone at Elastic review it? |
|
CLA looks good now. |
|
Would be nice to see its merged. I am now adding it as a local patch to build beats8/FreeBSD and everything works so far. W/o patch beats is unusable |
- fix beats by integrating patch from the elastic/go-sysinfo#126 - upgrade to the latest version ChangeLog: https://www.elastic.co/guide/en/beats/libbeat/current/release-notes.html Approved by: otis (elastic) PR: 272701
There was a problem hiding this comment.
Thanks a lot for your work (and patience :)). I think it's pretty good, but it needs to be a bit more careful with the null derefs in the the procstat_free* functions.
I'm not sure how much we care about 32bit, see kern.cp_times comment.
I do think the "I'll pass nil down instead of handling at the time I get it" makes it a bit confusing to read, but I only spotted one real bug.
Feel free to ignore some of the subjective suggestions.
| var errstr *C.char | ||
| kd := C.kvm_open(nil, nil, nil, unix.O_RDONLY, errstr) | ||
| if errstr != nil { |
There was a problem hiding this comment.
This is effectively passing NULL as errstr:
The kvm_open() function is the Sun kvm compatible open call. Here, the
errstr argument indicates how errors should be handled. If it is NULL,
no errors are reported and the application cannot know the specific na-
ture of the failed kvm call. If it is not NULL, errors are printed to
stderr with errstr prepended to the message, as in perror(3). Nor-
mally, the name of the program is used here. The string is assumed to
persist at least until the corresponding kvm_close() call.
I'd say either pass "go-sysinfo", so that it will prepend it and print out to stderr, but I guess this is not the desired behavior, or use kvm_openfiles() passing a storage of _POSIX2_LINE_MAX, like ps(1) does:
char errbuf[_POSIX2_LINE_MAX];
kd = kvm_openfiles(nlistf, memf, NULL, O_RDONLY, errbuf);
if (kd == NULL)
xo_errx(1, "%s", errbuf);
| if n, err := C.kvm_getswapinfo(kd, (*C.struct_kvm_swap)(unsafe.Pointer(&swap)), 1, 0); n != 0 { | ||
| return nil, fmt.Errorf("failed to get kvm_getswapinfo: %w", err) |
There was a problem hiding this comment.
kvm_getswapinfo() doesn't set errno, so you might be printing an old value.
, assuming you do the kvm_openfiles dance above: you'd have to go through kvm_geterr(3) . If not, I'd just zap err.
| //go:build freebsd && cgo | ||
|
|
There was a problem hiding this comment.
I don't see any references to Cgo here, do you really need it (and the file being named _cgo). I'm not an expert in Cgo so forgive me if I'm missing something.
There was a problem hiding this comment.
kvmGetSwapInfo is defined in a Cgo-dependent way, and is referred to here, so this file is tainted. The build constraint here protects that. Given that, it may be clearer to just have that function defined in this file.
| const sizeOfUint64 = int(unsafe.Sizeof(uint64(0))) | ||
|
|
||
| // cpuStateTimes uses sysctl kern.cp_time to get the amount of time spent in | ||
| // different CPU states. | ||
| func cpuStateTimes() (*types.CPUTimes, error) { | ||
| tickDuration, err := tickDuration() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| const mib = "kern.cp_time" | ||
| buf, err := unix.SysctlRaw(mib) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get %s: %w", mib, err) | ||
| } | ||
|
|
||
| var clockTicks [unix.CPUSTATES]uint64 | ||
| if len(buf) < len(clockTicks)*sizeOfUint64 { |
There was a problem hiding this comment.
I don't think this will work for 32bit systems, the problem is cp_times uses longs, not uint32_t or uint64_t, so on 32bit systems that will actually be 32 instead of 64, so the size of the structure changes.
See this call from top(1):
size = sizeof(long) * maxcpu * CPUSTATES;
if (sysctlbyname("kern.cp_times", times, &size, NULL, 0) == -1)
err(1, "sysctlbyname kern.cp_times");
I realize it's a bit sad that there isn't a unix.SysctlUlong, so in order to do this we could have to conditionally check.
| unsigned int countArrayItems(char **items) { | ||
| unsigned int i = 0; | ||
| for (i = 0; items[i] != NULL; ++i); | ||
| return i; | ||
| } |
There was a problem hiding this comment.
A bit of bikeshedding, can we not initialize i twice.
|
|
||
| unsigned int countArrayItems(char **items) { | ||
| unsigned int i = 0; | ||
| for (i = 0; items[i] != NULL; ++i); |
There was a problem hiding this comment.
minor nit, can we put the ; in its own new line, my brain always skips it.
| } | ||
| defer C.procstat_close(procstat) | ||
|
|
||
| env, err := C.procstat_getenvv(procstat, &p.kinfo, 0) |
There was a problem hiding this comment.
While you properly handle env == nil in copyArray(), and C.procstat_freeenv() can be called even if the allocation failed: I think is a bit too much imho, people will touch this code in the future and miss this artifact. Feel free to ignore this suggestion.
| } | ||
| defer C.procstat_close(procstat) | ||
|
|
||
| args, err := C.procstat_getargv(procstat, &p.kinfo, 0) |
| return i; | ||
| } | ||
|
|
||
| char * itemAtIndex(char **items, unsigned int index) { |
There was a problem hiding this comment.
Just a note itemAtIndex and countArrayItems could be implemented in go with unsafe.Add(), but I think this is fine.
There was a problem hiding this comment.
Prefer the unsafe over Cgo here. One is potentially (probably) a call and the other gets written as a single instruction.
| const maxlen = uint(1024) | ||
| out := make([]C.char, maxlen) |
There was a problem hiding this comment.
could use C.MAXPATHNAMELEN or C.PATH_MAX.
To address your 32-bit concern, I believe go-sysinfo does not plan to support x86 FreeBSD, given how it's been demoted to a Tier 2 platform with plans to drop support in 15.x. |
|
I can confirm that 32bit platforms seems to be broken now: |
|
I will try to find some time on the weekend to make patch 32 bit compatible, it seems to be not a hard task to do. |
|
another problem - cross build failing because we are defining |
The entire package is dependent on Cgo, so all files except doc.go should be guarded by the cgo build constraint. |
|
Did we make any recent progress here? |
69 days to go until 2024 is over. 😭 |
Updated support for FreeBSD using dbolcsfoldi's work from #40.
Sample host data
{ "host.cpu": { "idle": 84740000000, "irq": 3400000000, "system": 15410000000, "user": 43490000000 }, "host.info": { "architecture": "amd64", "boot_time": "2024-04-11T07:17:11.943687Z", "id": "535510ba-782f-4398-a566-76cef038cad1", "ip": [ "fe80::5054:ff:fe49:8a64/64", "192.168.122.66/24", "::1/128", "fe80::1/64", "127.0.0.1/8" ], "kernel_version": "14.0-RELEASE", "mac": [ "52:54:00:49:8a:64" ], "name": "freebsd", "native_architecture": "", "os": { "build": "RELEASE", "family": "freebsd", "major": 14, "minor": 0, "name": "FreeBSD", "patch": 0, "platform": "freebsd", "type": "freebsd", "version": "14.0-RELEASE" }, "timezone": "UTC", "timezone_offset_sec": 0 }, "host.memory": { "available_bytes": 6008164864, "free_bytes": 5357379584, "raw": { "active_bytes": 29511680, "buffer_bytes": 608727552, "cache_bytes": 0, "free_bytes": 5357379584, "inactive_bytes": 42057728, "wired_bytes": 774049792 }, "total_bytes": 6402035712, "used_bytes": 803561472, "virtual_free_bytes": 1073610752, "virtual_total_bytes": 1073610752, "virtual_used_bytes": 0 } }