Add CPU and Disk stats support to Illumos #378
Conversation
disk/disk_unix.go
Outdated
|
|
||
| func Usage(path string) (*UsageStat, error) { | ||
| stat := syscall.Statfs_t{} | ||
| err := syscall.Statfs(path, &stat) | ||
| stat := unix.Statfs_t{} |
There was a problem hiding this comment.
I'm sorry, but could you tell me why use unix instead of syscall?
If use unix, at least darwin platform this error occured.
# github.com/shirou/gopsutil/disk
./disk_unix.go:17: cannot use stat (type unix.Statfs_t) as type syscall.Statfs_t in argument to getFsType
getFsType is this function
func getFsType(stat syscall.Statfs_t) string {
return common.IntToString(stat.Fstypename[:])
}
There was a problem hiding this comment.
Drat, let me look into that. I'm mid-way through cherrypicking some patches onto this branch that will add memory support. I don't know why I didn't catch this during testing, however.
IMNSHO the syscall package should not be used anywhere where an x/sys/unix package variant is available because the syscall package is frozen, whereas the x/sys/unix package will track the current API.
In this particular case, there wasn't support for syscall.Statfs on the target platform, but was under the unix. package. It has been lamented a few times by the Go authors that the syscall package should be removed. IMO syscall would have been "fine" (on some level) if it weren't frozen. But since unix is there and not frozen, syscall should be outright deprecated and eschewed from all code bases so that programs can converge on a single maintained package (read: unix > syscall).
For instance, Illumos grew support for unix. Statvfs which didn't exist previously and couldn't have existed under the syscall package.
golang/sys@1e99a4f
There was a problem hiding this comment.
Sorry for my laziness since #30, we should move to x/sys/unix. I agree. However, I think this migration is not a part of this PR. So could you remove from this PR? Or, if x/sys/unix is required to work on illumos, I will make a new PR to migrate to x/sys/unix (but it takes a little time).
|
Could you revert some commits about rename And, I support using |
|
@shirou All of the As for
The tool is available via the Downstream consumers of
However that exercise is left to the consumer since Does that work/make sense? |
|
Well, I am not familiar with Solaris, but |
|
@shirou At present the worst that will happen is some of the values will be As issues arise, I'm happy to continue to try and address. |
|
|
|
@shirou right, but there isn't a |
|
@shirou Until golang/go#20603 is accepted or finished, could you please review and merge this PR? Once the PR is merged I will back out the |
|
Understood. I leave this PR open. If there is progress, could you write comment here? Thanks. |
Memoize the paths to psrinfo(1) and isainfo(1).
This requires an updated `golang.org/x/sys/unix` to pick up `unix.Statvfs()` for Illumos.
This implementation uses kstat(1) for the time being.
This will go away in the future but for now centralize the functionality so it can be hunted down and removed when the time is right.
Add `VirtualMemoryZone()` and return `VirtualMemory()` in terms of the values found in `VirtualMemoryZone()` when the measurement is run inside of a zone.
|
What is the status of this? I've recently started diving into Nomad and SmartOS and require this functionality. |
|
#20603 is still not merged, so no one is working I think. |
|
I'll open up a new PR for this when the new illumos tag is added to go 1.13 at the end of July. |
|
Go 1.13 has landed with support for the illumos GOOS build tag. Waiting for the corresponding PR @Smithx10 😉 Though I would prefer to not add a new external dependency (easyjson) just for illumos. |
Add support for various Illumos stats (via
kstat(1)atm).CC @jen20