Add stats support for job containers #979
Conversation
|
Should we just rename the |
dcantah
force-pushed
the
hostprocess-stats
branch
from
91ff222 to
6066d2b
Compare
My opinion is we should keep job containers used as the name here. The fact that this low-level implementation is surfaced via a different name is kind of something we shouldn't care about. Just like how enabling many capabilities is called "privileged" at a high level for Linux containers. |
That sounds fine to me (and makes sense), just might be a bit odd for "job container" to pop up in error strings for k8s |
dcantah
changed the title
| if systemProcInfo.NextEntryOffset == 0 { | ||
| break | ||
| } | ||
| systemProcInfo = (*winapi.SYSTEM_PROCESS_INFORMATION)(unsafe.Pointer(uintptr(unsafe.Pointer(systemProcInfo)) + uintptr(systemProcInfo.NextEntryOffset))) |
There was a problem hiding this comment.
It's unclear to me if this is a safe operation. According to the unsafe package description:
Unlike in C, it is not valid to advance a pointer just beyond the end of its original allocation:
// INVALID: end points outside allocated space.
var s thing
end = unsafe.Pointer(uintptr(unsafe.Pointer(&s)) + unsafe.Sizeof(s))
// INVALID: end points outside allocated space.
b := make([]byte, n)
end = unsafe.Pointer(uintptr(unsafe.Pointer(&b[0])) + uintptr(n))
So I would think that since you previously cast systemProcInfo to a SYSTEM_PROCESS_INFORMATION pointer, then using pointer arithmetic with that beyond it's size would be invalid too even though we know that there's memory there that we've allocated and can access. IDK this may not be super relevant in this case.
There was a problem hiding this comment.
I had a couple iterations of this that failed checkptr, but this passes so I'm erring on the side of this is fine but I'm open to keep digging here
There was a problem hiding this comment.
My take is this is okay because the "original allocation" in this case is the make([]byte, size) on line 566, and we never advance past that allocation.
There was a problem hiding this comment.
On the other hand, it would probably be a good idea to validate each NextEntryOffset and make sure it's still within the bounds of size.
There was a problem hiding this comment.
@kevpar Done, sorry for delay
dcantah
force-pushed
the
hostprocess-stats
branch
3 times, most recently
from
92cff66 to
f260fa0
Compare
* Add PropertiesV2 and Properties calls for host process containers. The only supported queries for them are PropertiesV2: Statistics Properties: ProcessList * Add NtQuerySystemInformation and SYSTEM_PROCESS_INFORMATION binds. This work will be utilized in the containerd shim just as the PropertiesV2 and Properties calls are today for process and hv isolated containers. Signed-off-by: Daniel Canter <[email protected]>
dcantah
force-pushed
the
hostprocess-stats
branch
from
f260fa0 to
012856b
Compare
This PR updates our internal ADO repo to the github commit [d9474d2](microsoft@d9474d2). Related work items: microsoft#964, microsoft#965, microsoft#966, microsoft#967, microsoft#968, microsoft#969, microsoft#970, microsoft#971, microsoft#972, microsoft#974, microsoft#975, microsoft#976, microsoft#977, microsoft#978, microsoft#979, microsoft#980, microsoft#981, microsoft#982, microsoft#983, microsoft#984, microsoft#987, microsoft#990, microsoft#991, microsoft#992, microsoft#993, microsoft#995, microsoft#996, microsoft#997, microsoft#1000
Add stats support for job containers
PropertiesV2: Statistics
Properties: ProcessList
This work will be utilized in the containerd shim just as the PropertiesV2 and Properties calls
are today for process and hv isolated containers.
Signed-off-by: Daniel Canter [email protected]