pkg/sysinfo: move MemInfo and ReadMemInfo to a separate package#44949
pkg/sysinfo: move MemInfo and ReadMemInfo to a separate package#44949thaJeztah merged 1 commit intomoby:masterfrom
Conversation
thaJeztah
added
status/2-code-review
impact/changelog
kind/refactor
|
| // | ||
| // Deprecated: use [sysinfo.Memory]. | ||
| type MemInfo = sysinfo.Memory | ||
| type MemInfo = struct{} |
There was a problem hiding this comment.
Can we just remove this function, and add "Memory was previously available as [system.MemInfo]" to the comment lines of sysinfo.Memory?
There was a problem hiding this comment.
Yeah, I was really on the fence to either remove it altogether, but then consumers would have no guidance at all where it went.
To be fair; the list of consumers is very limited; https://grep.app/search?q=.ReadMemInfo%28%29&filter[lang][0]=Go
(ignoring the vendor/xx and PodMan code (which uses its own fork of this), I think it's only nerdctl and amazon-ecs-agent;
- https://github.com/containerd/nerdctl/blob/5da1481d3342d75bf356469afeac2fb534363047/pkg/infoutil/infoutil_linux.go#L123
- https://github.com/aws/amazon-ecs-agent/blob/13d1e854decd15a8839ea76cc1cd1b73b9f9d03b/agent/api/ecsclient/client.go#L310
There was a problem hiding this comment.
but then consumers would have no guidance at all where it went.
I expect them to run git grep MemInfo and find the guidance comment (Memory was previously available as [system.MemInfo]).
There was a problem hiding this comment.
I prefer option C: solve the dependency-graph problem without breaking any users. Move the MemInfo and ReadMemInfo implementations into a new pkg/meminfo package and update both pkg/system and pkg/sysinfo to have deprecated aliases.
There was a problem hiding this comment.
I'm a bit on the fence doing so. These functions were dropped in pkg/system as it was already a bit of a grab-bag of functions, but were only actually meant for internal use (most notably, together with sysinfo, which provided the other info about the system.
If we would, I would not update the aliases in pkg/sysinfo, as that location has never been released (only on master).
thaJeztah
force-pushed
the
remove_system_meminfo_alias
branch
from
f880c4f to
5dfda68
Compare
thaJeztah
changed the title
thaJeztah
force-pushed
the
remove_system_meminfo_alias
branch
from
5dfda68 to
22686cb
Compare
| } | ||
|
|
||
| // ReadMemInfo retrieves memory statistics of the host system and returns a | ||
| // Read retrieves memory statistics of the host system and returns a |
There was a problem hiding this comment.
| // Read retrieves memory statistics of the host system and returns a | |
| // readMemInfo retrieves memory statistics of the host system and returns a |
There was a problem hiding this comment.
Ah, dang! Looks like my IDE was trying to be smart there. Fixed 👍
Commit 6a516ac moved the MemInfo type and ReadMemInfo() function into the pkg/sysinfo package. In an attempt to assist consumers of these to migrate to the new location, an alias was added. Unfortunately, the side effect of this alias is that pkg/system now depends on pkg/sysinfo, which means that consumers of this (such as docker/cli) now get all (indirect) dependencies of that package as dependency, which includes many dependencies that should only be needed for the daemon / runtime; - github.com/cilium/ebpf - github.com/containerd/cgroups - github.com/coreos/go-systemd/v22 - github.com/godbus/dbus/v5 - github.com/moby/sys/mountinfo - github.com/opencontainers/runtime-spec This patch moves the MemInfo related code to its own package. As the previous move was not yet part of a release, we're not adding new aliases in pkg/sysinfo. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
remove_system_meminfo_alias
branch
from
22686cb to
2d49080
Compare
3 participants
Commit 6a516ac (#44663) moved the MemInfo type and ReadMemInfo() function into the pkg/sysinfo package. In an attempt to assist consumers of these to migrate to the new location, an alias was added.
Unfortunately, the side effect of this alias is that pkg/system now depends on pkg/sysinfo, which means that consumers of this (such as docker/cli) now get all (indirect) dependencies of that package as dependency, which includes many dependencies that should only be needed for the daemon / runtime;
This patch moves the MemInfo related code to its own package. As the previous move
was not yet part of a release, we're not adding new aliases in pkg/sysinfo.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)