[PR] Support monitoring short-lived processes in the access log module [skywalking-rover]

["mrproliu (via GitHub)" <gi...@apache.org>]

mrproliu opened a new pull request, #214:
URL: https://github.com/apache/skywalking-rover/pull/214

   (no comment)


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Support monitoring short-lived processes in the access log module [skywalking-rover]

["mrproliu (via GitHub)" <gi...@apache.org>]

mrproliu merged PR #214:
URL: https://github.com/apache/skywalking-rover/pull/214


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Support monitoring short-lived processes in the access log module [skywalking-rover]

["Copilot (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org