Copilot commented on code in PR #214:
URL: https://github.com/apache/skywalking-rover/pull/214#discussion_r3593145029
##########
pkg/process/finders/kubernetes/finder.go:
##########
@@ -59,10 +60,42 @@ var log = logger.GetLogger("process", "finder",
"kubernetes")
var (
kubepodsRegex =
regexp.MustCompile(`cri-containerd-(?P<Group>\w+)\.scope`)
openShiftPodsRegex = regexp.MustCompile(`crio-(?P<Group>\w+)\.scope`)
+ dockerPodsRegex = regexp.MustCompile(`docker-(?P<Group>\w+)\.scope`)
ipExistTimeout = time.Minute * 10
ipSearchParallel = 10
)
+// containerIDFromCgroupDir turns the base name of a cgroup directory into the
id of the container
+// it holds, or "" when it holds none. It mirrors the naming GetProcessCGroup
already parses out of
+// /proc/<pid>/cgroup, so that a container resolved by walking the tree and
one resolved by reading
+// a process's cgroup line come out with the same id.
+func containerIDFromCgroupDir(dirName string) string {
+ for _, re := range []*regexp.Regexp{kubepodsRegex, openShiftPodsRegex,
dockerPodsRegex} {
+ if m := re.FindStringSubmatch(dirName); len(m) > 1 {
+ return m[1]
+ }
+ }
+ // the cgroupfs driver, unlike the systemd one, names the directory
after the container id
+ if isContainerID(dirName) {
+ return dirName
+ }
+ return ""
+}
Review Comment:
`containerIDFromCgroupDir` allocates a new slice of regex pointers on every
call (`[]*regexp.Regexp{...}`). This function is used as the cgroup directory
normalizer during a full tree walk, so the allocation happens once per
directory and adds avoidable GC/CPU overhead. Consider doing the regex checks
without per-call slice allocation (or moving the slice to a package-level var).
##########
pkg/tools/cgroup/resolver.go:
##########
@@ -0,0 +1,199 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Package cgroup resolves the cgroup ids that eBPF programs report(through
+// bpf_get_current_cgroup_id) to the container they belong to, and back.
+package cgroup
+
+import (
+ "fmt"
+ "io/fs"
+ "os"
+ "path/filepath"
+ "strings"
+ "sync"
+ "syscall"
+
+ "github.com/apache/skywalking-rover/pkg/logger"
+ "github.com/apache/skywalking-rover/pkg/tools/host"
+)
+
+var log = logger.GetLogger("tools", "cgroup")
+
+// DefaultMountPoint is where the unified(v2) cgroup hierarchy lives, as seen
from inside the agent
+// container.
+//
+// It has to be the *host's* /sys: the agent's own only exposes its own cgroup
subtree, and the
+// containers whose ids we need are not in it. Where the host's /sys is
mounted is a property of the
+// deployment - the helm chart bind-mounts it at /sys, an environment where
the node is itself a
+// container may put it elsewhere - so the prefix comes from
ROVER_HOST_SYS_MAPPING rather than
+// being assumed, exactly as the /proc prefix does.
+func DefaultMountPoint() string {
+ return host.GetHostSysInHost("fs/cgroup")
+}
+
+const (
+ // controllersFile only exists on the unified(v2) hierarchy, which
makes its presence the
+ // probe for "this host runs cgroup v2". On a v1-only host
bpf_get_current_cgroup_id() reports
+ // the id from the unified hierarchy, where the containers are not, so
every lookup would miss
+ // and the caller must disable the whole cgroup based path instead.
+ controllersFile = "cgroup.controllers"
+)
+
+// There is deliberately no bound on how deep the walk goes, because there is
no depth to bound it
+// to. Where a container's cgroup sits is not fixed by anything:
+//
+// - the cgroup driver decides both the naming and the number of
levels(systemd:
+//
kubepods.slice/kubepods-<qos>.slice/kubepods-<qos>-pod<uid>.slice/<id>.scope,
cgroupfs:
+// kubepods/<qos>/pod<uid>/<id>)
+// - the QoS class adds a level for burstable/besteffort that guaranteed
pods do not have
+// - wherever the kubelet itself runs inside a container(kind,
docker-in-docker) the whole
+// hierarchy hangs under that container's cgroup, e.g.
/system.slice/docker-<node>.scope/
+// kubelet.slice/kubelet-kubepods.slice/..., which is three levels further
down
+// - the kubelet can be pointed at an arbitrary cgroup root
+//
+// If the layout were knowable the path could simply be built and stat'ed, and
none of this would
+// exist. It is not, which is why the tree is walked and matched by name - so
what makes a directory
+// a container is its name, never how deep it happens to be. An earlier
version guessed a depth that
+// fit a bare node; on a nested one it matched nothing at all and did so
silently.
+//
+// The walk itself is cheap: the tree holds at most a few thousand
directories, only their names are
+// read, and a directory is stat'ed only once its name says it is a container.
+
+// NameNormalizer turns the base name of a cgroup directory into the id of the
container it holds,
+// or "" when the directory does not belong to a container. It is injected
rather than implemented
+// here because the naming is container runtime specific and the process
finder already owns those
+// rules.
+type NameNormalizer func(dirName string) string
+
+// Resolver maps cgroup ids to container ids and back, by walking the host's
cgroup v2 tree.
+//
+// The kernel identifies a cgroup v2 by the inode of its directory, and that
inode is precisely
+// what bpf_get_current_cgroup_id() returns, so the mapping is built by
stat'ing the tree.
+//
+// Reading the tree from the mounted cgroupfs - rather than from
/proc/<pid>/cgroup - is the whole
+// point: the paths in /proc/<pid>/cgroup are rendered relative to the cgroup
namespace of the
+// *reading* process, so an agent living in its own namespace reads
unresolvable paths such as
+// "0::/../<container-id>", whereas a bind-mounted host cgroupfs exposes the
full tree with real
+// inodes no matter which namespace the reader sits in.
+type Resolver struct {
+ mountPoint string
+ normalize NameNormalizer
+
+ mu sync.RWMutex
+ idByContainer map[string]uint64
+ containerByID map[uint64]string
+}
+
+// NewResolver builds a Resolver over mountPoint(use DefaultMountPoint unless
testing). It does not
+// walk anything yet; call Refresh for that.
+func NewResolver(mountPoint string, normalize NameNormalizer) *Resolver {
+ return &Resolver{
+ mountPoint: mountPoint,
+ normalize: normalize,
+ idByContainer: make(map[string]uint64),
+ containerByID: make(map[uint64]string),
+ }
+}
+
+// Available reports whether mountPoint holds a usable unified(v2) hierarchy.
A caller must treat a
+// false here as "the cgroup id of a process cannot be resolved on this host"
and fall back to
+// whatever it did before, never as a reason to fail.
+func Available(mountPoint string) bool {
+ st, err := os.Stat(filepath.Join(mountPoint, controllersFile))
+ return err == nil && !st.IsDir()
+}
+
+// Refresh rebuilds the mapping from the current state of the tree. It is
meant to be driven by the
+// periodic process discovery: a cgroup that appears between two refreshes is
simply resolved on the
+// next one, and until then the caller behaves as it did before this package
existed.
+func (r *Resolver) Refresh() error {
+ idByContainer := make(map[string]uint64)
+ containerByID := make(map[uint64]string)
+
+ root := filepath.Clean(r.mountPoint)
+ scanned := 0
+ err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err
error) error {
+ if err != nil {
+ // cgroups come and go while we walk; a vanished
directory is normal churn on a busy
+ // node, so skip it rather than abandoning the whole
refresh.
+ return nil //nolint:nilerr // deliberate: keep walking
past transient errors
+ }
Review Comment:
In cgroup tree walking, the WalkDir callback currently ignores *all*
filesystem errors. That’s safe for transient `ENOENT` while cgroups churn, but
it also silently hides non-transient errors (e.g. permission/IO errors), which
can leave the resolver with an incomplete mapping without any signal. Consider
only ignoring "not exists" errors and returning other errors so Refresh fails
and the caller can reliably fall back.
##########
pkg/process/finders/kubernetes/finder.go:
##########
@@ -423,6 +498,81 @@ func (f *ProcessFinder) ShouldMonitor(pid int32) bool {
return true
}
+// ShouldMonitorExecuting judges a process the kernel has just started.
+//
+// It prefers the ordinary /proc based path, which is richer and is exactly
what the periodic scan
+// does. Only when /proc has nothing left to say - the process already exited,
which is the whole
+// reason it is worth catching this early - does it fall back to what the
kernel handed us: the
+// cgroup id identifies the container, and the task name stands in for the
command line.
+func (f *ProcessFinder) ShouldMonitorExecuting(exec
*api.ProcessExecuteContext) bool {
+ if f.ShouldMonitor(exec.Pid) {
+ return true
+ }
+ if exec.CgroupID == 0 || f.cgroupResolver == nil {
+ // no kernel side identity to fall back on(cgroup v1, or the
tree could not be walked)
+ return false
+ }
+ containerID, exist :=
f.cgroupResolver.ContainerByCgroupID(exec.CgroupID)
+ if !exist {
+ return false
+ }
+ pc, exist := f.registry.BuildPodContainers()[containerID]
+ if !exist || pc == nil {
+ return false
+ }
Review Comment:
This fallback path rebuilds the full pod/container map via
`BuildPodContainers()` to look up a single container ID. Since
`ShouldMonitorExecuting` already calls `f.ShouldMonitor(exec.Pid)` (which also
calls `BuildPodContainers()`), this can rebuild the map multiple times per
kernel event. Consider caching the containers map (or adding a direct lookup
API), and at minimum avoid rebuilding it again here by storing it in a local
variable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
