pkg/platform: cleanup, and deprecate OSType#45298
Conversation
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Make sure the package's documentation is available for any platform, not just "unix". Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
It was not immediately clear why we were not using runtime.GOARCH for these (with a conversion to other formats, such as x86_64). These docs are based on comments that were posted when implementing this package; - moby#13921 (comment) - moby#13921 (comment) Some links were now redirecting to a new location, so updated them to not depend on the redirect. While at it, also updated a call to logrus to use structured formatting (WithError()). Signed-off-by: Sebastiaan van Stijn <[email protected]>
These consts are only used internally, and never returned to the user. Un-export to make it clear these are not for external consumption. While looking at the code, I also noticed that we may be using the wrong Windows API to collect this information (and found an implementation elsewhere that does use the correct API). I did not yet update the code, in cases there are specific reasons. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This const looks to only be there for "convenience", or _possibly_ was created with future normalization or special handling in mind. In either case, currently it is just a direct copy (alias) for runtime.GOOS, and defining our own type for this gives the impression that it's more than that. It's only used in a single place, and there's no external consumers, so let's deprecate this const, and use runtime.GOOS instead. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
added
status/2-code-review
impact/changelog
impact/deprecation
kind/refactor
| logrus.Errorf("Could not read system architecture info: %v", err) | ||
| logrus.WithError(err).Error("Could not read system architecture info") |
There was a problem hiding this comment.
FWIW, I wonder if we should consider replacing var Architecture with a func Architecture() (with a sync.Once). Currently this is part of an init(), which means that we'll be loading "kernel32.dll" on Windows (and a syscall), and we may be logging some error just by importing this package 🤔
| // NumProcs returns the number of processors on the system | ||
| func NumProcs() uint32 { | ||
| var sysinfo systeminfo | ||
| _, _, _ = syscall.SyscallN(procGetSystemInfo.Addr(), uintptr(unsafe.Pointer(&sysinfo))) | ||
| return sysinfo.dwNumberOfProcessors | ||
| } |
There was a problem hiding this comment.
Related to the above; I could use some thoughts on this part. Do we know how costly it is to get this information? ❓ I see that this function is called (on Windows) as part of daemon.GetContainerStats() -> daemon.stats()
moby/daemon/stats/collector.go
Lines 116 to 117 in dd3b71d
Lines 148 to 149 in dd3b71d
Lines 533 to 536 in dd3b71d
That means that we're calling this function for every container, and at least once every second.
I think what we're collecting here is the hardware information (i.e. number of physical processors), and if that's true, we could assume that to not change at runtime, but I wasn't 100% sure if this could have "hot-pluggable" CPUs 🤔.
If that's not the case, and if it's "costly", we could use a sync.Once for this?
var (
numProcsOnce sync.Once
numProcs uint32
)
// NumProcs returns the number of processors on the system
func NumProcs() uint32 {
numProcsOnce.Do(func() {
var si systeminfo
_, _, _ = syscall.SyscallN(procGetSystemInfo.Addr(), uintptr(unsafe.Pointer(&si)))
numProcs = si.dwNumberOfProcessors
})
return numProcs
}
There was a problem hiding this comment.
I don't have any idea whether this is costly or not, but if you wanted to write some way to measure it, I'd be happy to run a test on my Windows system 😅
3 participants
pkg/platform: rename files for consistency
pkg/platform: move package doc to platform-agnostic file
Make sure the package's documentation is available for any platform,
not just "unix".
pkg/platform: replace use of deprecated syscall.Syscall
pkg/platform: use const for OSType, improve GoDoc
It was not immediately clear why we were not using runtime.GOARCH for
these (with a conversion to other formats, such as x86_64). These docs
are based on comments that were posted when implementing this package;
pkg/platform: un-export consts that are only for internal use
These consts are only used internally, and never returned to the user.
Un-export to make it clear these are not for external consumption.
While looking at the code, I also noticed that we may be using the wrong
Windows API to collect this information (and found an implementation elsewhere
that does use the correct API). I did not yet update the code, in cases there
are specific reasons.
pkg/platform: deprecate OSType in favor or runtime.GOOS
This const looks to only be there for "convenience", or possibly was created
with future normalization or special handling in mind.
In either case, currently it is just a direct copy (alias) for runtime.GOOS,
and defining our own type for this gives the impression that it's more than
that. It's only used in a single place, and there's no external consumers, so
let's deprecate this const, and use runtime.GOOS instead.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)