api review: update field descriptions by Billy99 · Pull Request #419 · bpfman/bpfman-operator

Billy99 · 2025-03-21T17:13:07Z

As part of the API review, a comment was made to improve the description of all fields. This commit makes a pass at the ClusterBpfApplication, BpfApplication, ClusterBpfApplicationState and BpfApplicationState CRD fields.

The description fields are most easily seen using the kubectl explain command. For example:

$ kubectl explain clusterbpfapplication
GROUP:      bpfman.io
KIND:       ClusterBpfApplication
VERSION:    v1alpha1

DESCRIPTION:
    ClusterBpfApplication is the schema for the cluster scoped BPF Applications
    API. Using this API allows applications to load one or more eBPF programs on
    a Kubernetes cluster using bpfman to load the programs.
    
    
    The clusterBpfApplication.status field can be used to determine if any
    errors occurred in the loading of the eBPF programs. Because one eBPF
    program can be loaded on multiple Kubernetes nodes, and then attached
    multiple times on each Kubernetes node, clusterBpfApplication.status is just
    a summary, all loaded or something failed. bpfman creates a
    ClusterBpfApplicationState CRD instance for each Kubernetes Node for each
    ClusterBpfApplication instance. The ClusterBpfApplicationState CRD provides
    load status for each eBPF program and each of it's attachment points
    (links).
    
FIELDS:
  apiVersion	<string>
    APIVersion defines the versioned schema of this representation of an object.
    Servers should convert recognized schemas to the latest internal value, and
    may reject unrecognized values. More info:
    https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources

  kind	<string>
    Kind is a string value representing the REST resource this object
    represents. Servers may infer this from the endpoint the client submits
    requests to. Cannot be updated. In CamelCase. More info:
    https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds

  metadata	<ObjectMeta>
    Standard object's metadata. More info:
    https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata

  spec	<Object>
    spec defines the desired state of the ClusterBpfApplication. The
    ClusterBpfApplication describes the set of one or more cluster scoped eBPF
    programs that should be loaded for a given application and attributes for
    how they should be loaded. eBPF programs that are grouped together under the
    same ClusterBpfApplication instance can share maps and global data between
    the eBPF programs loaded on the same Kubernetes Node.

  status	<Object>
    status reflects the status of a BPF Application and indicates if all the
    eBPF programs for a given instance loaded successfully or not.

The following gist collects all the kubectl explain output:
https://gist.github.com/Billy99/b871e60f04944d4b03c9c0106d2c8a43

Billy99 · 2025-03-21T17:19:11Z

NOTE: I remove most of the comments that were before a struct in the API types files. Here is a sample output:

$ kubectl explain clusterbpfapplication.spec.programs.fentry.links
GROUP:      bpfman.io
KIND:       ClusterBpfApplication
VERSION:    v1alpha1

FIELD: links <[]Object>


DESCRIPTION:
    Whether the program should be attached to the function.
    ClFentryAttachInfo indicates that the Fentry program should be attached to
    the function identified in ClFentryLoadInfo. The only valid value for Attach
    is true.
    
FIELDS:
  mode	<string> -required-
  enum: Attach, Dettach
    <no description>

The generated output is a concatenation of the links parameter comment (first struct) and comment for the struct links is using (second struct). Might be because of the inline tag.

// ClFentryProgramInfo defines the Fentry program details
type ClFentryProgramInfo struct {
	ClFentryLoadInfo `json:",inline"`
	// Whether the program should be attached to the function.
	// +optional
	// +kubebuilder:validation:MaxItems=1
	// +kubebuilder:default:={}
	Links []ClFentryAttachInfo `json:"links"`
}

// ClFentryAttachInfo indicates that the Fentry program should be attached to
// the function identified in ClFentryLoadInfo. The only valid value for Attach
// is true.
type ClFentryAttachInfo struct {
	// +kubebuilder:validation:Enum=Attach;Dettach;
	Mode AttachTypeAttach `json:"mode"`
}

To keep it clear and consistent, removed the comment for the struct and left the comments for the parameter.

mergify · 2025-03-21T20:12:10Z

@Billy99, this pull request is now in conflict and requires a rebase.

codecov · 2025-03-21T20:28:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (87cd4b1) to head (4e2b3e7).
Report is 4 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #419   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

anfredette · 2025-03-26T22:20:34Z

apis/v1alpha1/cluster_bpf_application_types.go

+	// has allocated an sk_buff. TCX is newer implementation of TC with enhanced
+	// performance and better support for running multiple programs on a given
+	// network device. This makes TC useful for packet classification actions.


I suggest you delete the blurb about TCX here. It's stated below in the TCX section.

mergify · 2025-03-26T22:21:08Z

@Billy99, this pull request is now in conflict and requires a rebase.

apis/v1alpha1/cluster_bpf_application_types.go

apis/v1alpha1/cluster_fentry_program_types.go

apis/v1alpha1/cluster_fexit_program_types.go

anfredette · 2025-03-27T17:22:03Z

apis/v1alpha1/cluster_kprobe_program_types.go

+	// links is optional and is the list of hook points to which the KProbe program
+	// should be attached. The eBPF program is loaded in kernel memory when the BPF
+	// Application CRD is created and the selected Kubernetes nodes are active. The
+	// eBPF program will not be triggered until the program has also been attached
+	// to a hook point described in this list. Items may be added or removed from
+	// the list at any point, causing the eBPF program to be attached or detached.
+	//


Can this be a general statement made in the bpf application CRDs that applies to all program types rather than repeat it for each type? I suggested some additional explanation for Fentry and Fexit that explain how they are a little different, but the general statement still applies to them.

As a general comment, I think that all of the details for each program type should be described on the attributes of the associated *AttachInfo structure and not anywhere else.

anfredette

I'm done reviewing everything. I just have a few suggestions.

apis/v1alpha1/cluster_tc_program_types.go

apis/v1alpha1/cluster_uprobe_program_types.go

apis/v1alpha1/shared_types.go

anfredette · 2025-03-28T13:25:29Z

apis/v1alpha1/shared_types.go

+	// For a ClusterBpfApplicationState instance or a BpfApplicationState instance,
+	// conditions contains the summary state for all eBPF programs defined in the
+	// instance for a given Kubernetes nodes.
 	// +patchMergeKey=type


There is now a BpfApplicationStateConditionType for the state objects.

anfredette

I'm done reviewing. I just made a few suggestions. If you choose to use them, some apply accross multiple program types.

everettraven

Did a quick pass and have some comments

apis/v1alpha1/cluster_bpf_application_types.go

apis/v1alpha1/cluster_fentry_program_types.go

everettraven · 2025-04-01T18:57:37Z

apis/v1alpha1/cluster_kprobe_program_types.go

+	// offset is optional and the value is added to the address of the hook point
+	// function.


What does this mean to a user? I'm still not sure why a user would care to set this

eBPF programs can be used to debug kernel code without having to recompile and reload the kernel. So maybe some want to attach a probe somewhere later in a given kernel function other than at the start of the function, maybe after some 'if" check to determine if that is why a function is returning early or something, the offset lets them attach at a given offset in the code. The user has to know what they are doing here. We are just a wrap to what the kernel is exposing, so we are also exposing it.

apis/v1alpha1/cluster_tc_program_types.go

everettraven · 2025-04-01T19:03:21Z

apis/v1alpha1/cluster_tc_program_types.go

-	// from 0 to 1000 where lower values have higher precedence.
+	// priority is required and determines the execution order of the TC program
+	// relative to other TC programs attached to the same hook point. It must be a
+	// value between 0 and 1000, where lower values indicate higher precedence.


Am I allowed to set the same priority for multiple programs?

Yes, updated text to that effect.

everettraven · 2025-04-01T19:03:35Z

apis/v1alpha1/cluster_tc_program_types.go

+	//   UnSpec, OK, ReClassify, Shot, Pipe, Stolen, Queued, Repeat, ReDirect,
+	//   Trap and DispatcherReturn


What do these mean?

everettraven · 2025-04-01T19:05:40Z

apis/v1alpha1/cluster_xdp_program_types.go

+	// proceedOn is optional and allows the user to call other XDP programs in a
+	// chain, or not call the next program in a chain based on the exit code of
+	// an XDP program. Allowed values are:
+	//   Aborted, Drop, Pass, TX, ReDirect and DispatcherReturn


What do these mean?

rework shell check task

As part of the API review, a comment was made to improve the description of all fields. This commit makes a pass at the ClusterBpfApplication, BpfApplication, ClusterBpfApplicationState and BpfApplicationState CRD fields. Signed-off-by: Billy McFall <22157057+Billy99@users.noreply.github.com>

anfredette · 2025-04-28T17:47:22Z

The force push above was just a rebase.

anfredette

Overall, this looks great. Thanks for doing this @Billy99.

I made a few final tweaks myself and pushed another commit.

Signed-off-by: Andre Fredette <afredette@redhat.com>

anfredette

/LGTM

Billy99 force-pushed the billy99-api-review-01 branch from ce3889a to 8ba6c9b Compare March 21, 2025 19:32

mergify bot added the needs-rebase label Mar 21, 2025

Billy99 force-pushed the billy99-api-review-01 branch from 8ba6c9b to 8d64911 Compare March 21, 2025 20:22

mergify bot removed the needs-rebase label Mar 21, 2025

anfredette reviewed Mar 26, 2025

View reviewed changes

mergify bot added the needs-rebase label Mar 26, 2025

anfredette reviewed Mar 26, 2025

View reviewed changes

apis/v1alpha1/cluster_bpf_application_types.go Outdated Show resolved Hide resolved