From 0ded0d73110dd021a18ee4964da139afee163e07 Mon Sep 17 00:00:00 2001 From: Maryam Tahhan Date: Fri, 1 Dec 2023 21:37:45 +0000 Subject: [PATCH] bpf-map-pinning: fixes a few issues * Update the documentation to reflect configuration changes needed. * Update the program to pin the xsk map to just use xdp-loader. The previous c implementation had issues and the version of libxdp in the container images is too old for libxdp pinning support. Reverting to bpf (for pinning support) would break the libxdp based program unloading. * Don't return an error from the bpf clean up function when there's no program on the interface. Signed-off-by: Maryam Tahhan --- README.md | 2 + cmd/deviceplugin/main.go | 1 - constants/constants.go | 3 ++ deployments/daemonset.yml | 7 ++++ images/amd64.dockerfile | 3 +- internal/bpf/bpfWrapper.c | 59 +++------------------------- internal/bpf/bpfWrapper.go | 23 +++++++++-- internal/bpf/bpfWrapper.h | 1 - internal/bpf/mapManager.go | 37 +++++++++-------- internal/cni/cni.go | 16 ++++---- internal/deviceplugin/poolManager.go | 6 +-- 11 files changed, 68 insertions(+), 90 deletions(-) diff --git a/README.md b/README.md index a6a19113..af15b58f 100644 --- a/README.md +++ b/README.md @@ -411,6 +411,8 @@ UdsServerDisable is a Boolean configuration. If set to true, devices in this poo BpfMapPinningEnable is a Boolean configuration. If set to true, will use BPF map pinning instead of a UDS to share an XSK map with a pod. By default, this is set to false. Should set UdsServerDisable to true when using this configuration. +> **_NOTE:_** When using this setting it's important to adjust the securityContext for the DP Daemonset to `privileged: true` to allow for bidirectional propagation of the volume where the BPF maps are pinned. this will no longer be needed when the DP is integrated with bpfman. + #### UdsTimeout UdsTimeout is an integer configuration. This value sets the amount of time, in seconds, that the UDS server will wait while there is no activity on the UDS. When this timeout limit is reached, the UDS server terminates and the UDS is deleted from the filesystem. This can be a useful setting, for example, in scenarios where large batches of pods are created together. Large batches of pods tend to take some time to spin up, so it might be beneficial to have the UDS server sit waiting a little longer for the pod to start. The maximum allowed value is 300 seconds (5 min). The minimum and default value is 30 seconds. diff --git a/cmd/deviceplugin/main.go b/cmd/deviceplugin/main.go index 167d8ef6..a79ac45d 100644 --- a/cmd/deviceplugin/main.go +++ b/cmd/deviceplugin/main.go @@ -143,7 +143,6 @@ func main() { logging.Errorf("Termination error: %v", err) } } - } func configureLogging(cfg deviceplugin.PluginConfig) error { diff --git a/constants/constants.go b/constants/constants.go index 32f17c8b..6ea51650 100644 --- a/constants/constants.go +++ b/constants/constants.go @@ -84,6 +84,7 @@ var ( udsPodSock = "/afxdp.sock" /* BPF*/ + pinMapBaseDir = "/var/run/afxdp_dp/" bpfMapPodPath = "/tmp/afxdp_dp/" xsk_map = "/xsks_map" @@ -222,6 +223,7 @@ type uds struct { } type bpf struct { + PinMapBaseDir string BpfMapPodPath string Xsk_map string } @@ -349,6 +351,7 @@ func init() { } Bpf = bpf{ + PinMapBaseDir: pinMapBaseDir, BpfMapPodPath: bpfMapPodPath, Xsk_map: xsk_map, } diff --git a/deployments/daemonset.yml b/deployments/daemonset.yml index e2bb8606..a62de975 100644 --- a/deployments/daemonset.yml +++ b/deployments/daemonset.yml @@ -62,6 +62,7 @@ spec: image: intel/afxdp-plugins-for-kubernetes:latest imagePullPolicy: IfNotPresent securityContext: + #privileged: true # NEEDED WHEN USING BPF MAP PINNING FOR BIDIR PROPAGATION capabilities: drop: - all @@ -78,6 +79,9 @@ spec: volumeMounts: - name: unixsock mountPath: /tmp/afxdp_dp/ + - name: bpfmappinning + mountPath: /var/run/afxdp_dp/ + #mountPropagation: Bidirectional - name: devicesock mountPath: /var/lib/kubelet/device-plugins/ - name: resources @@ -92,6 +96,9 @@ spec: - name: unixsock hostPath: path: /tmp/afxdp_dp/ + - name: bpfmappinning + hostPath: + path: /var/run/afxdp_dp/ - name: devicesock hostPath: path: /var/lib/kubelet/device-plugins/ diff --git a/images/amd64.dockerfile b/images/amd64.dockerfile index 4e299231..7a84be90 100644 --- a/images/amd64.dockerfile +++ b/images/amd64.dockerfile @@ -28,13 +28,14 @@ WORKDIR /usr/src/afxdp_k8s_plugins RUN apk add --no-cache build-base~=0.5-r3 \ && apk add --no-cache libbsd-dev~=0.11.7 \ && apk add --no-cache libxdp-dev~=1.2.10-r0 \ +&& apk add --no-cache libbpf-dev~=1.0.1-r0 \ && apk add --no-cache llvm15~=15.0.7-r0 \ && apk add --no-cache clang15~=15.0.7-r0 \ && make builddp FROM amd64/alpine:3.18@sha256:25fad2a32ad1f6f510e528448ae1ec69a28ef81916a004d3629874104f8a7f70 RUN apk --no-cache -U add iproute2-rdma~=6.3.0-r0 acl~=2.3 \ - && apk add --no-cache libxdp~=1.2.10-r0 + && apk add --no-cache xdp-tools~=1.2.10-r0 COPY --from=cnibuilder /usr/src/afxdp_k8s_plugins/bin/afxdp /afxdp/afxdp COPY --from=dpbuilder /usr/src/afxdp_k8s_plugins/bin/afxdp-dp /afxdp/afxdp-dp COPY --from=dpbuilder /usr/src/afxdp_k8s_plugins/images/entrypoint.sh /afxdp/entrypoint.sh diff --git a/internal/bpf/bpfWrapper.c b/internal/bpf/bpfWrapper.c index 131e67f0..2ea64b07 100644 --- a/internal/bpf/bpfWrapper.c +++ b/internal/bpf/bpfWrapper.c @@ -158,13 +158,13 @@ int Clean_bpf(char *ifname) { mp = xdp_multiprog__get_from_ifindex(if_index); if (!mp) { - Log_Error("%s: unable to receive correct multi_prog reference : %s", __FUNCTION__, - mp); - return -1; + Log_Info("%s: No programs loaded on : %s", __FUNCTION__, + ifname); + return 0; } err = xdp_multiprog__detach(mp); - if (err) { + if (err && err != -EINVAL) { // -EINVAL == No program attached Log_Error("%s: Removal of xdp program failed, returned: " "returned: %d", __FUNCTION__, err); @@ -180,7 +180,6 @@ int Load_attach_bpf_xdp_pass(char *ifname) { char *filename = "/afxdp/xdp_pass.o"; struct bpf_object *obj; struct xdp_program *prog; - __u32 xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST | XDP_FLAGS_DRV_MODE; Log_Info("%s: disovering if_index for interface %s", __FUNCTION__, ifname); @@ -212,7 +211,7 @@ int Load_attach_bpf_xdp_pass(char *ifname) { } /* Attach the program to the interface at the xdp hook */ - err = xdp_program__attach(prog, ifindex, XDP_FLAGS_UPDATE_IF_NOEXIST, 0); + err = xdp_program__attach(prog, ifindex, XDP_MODE_NATIVE, 0); if (err) { libxdp_strerror(err, "Couldn't attach the xdp pass program", sizeof("Couldn't attach the xdp pass program")); @@ -225,51 +224,3 @@ int Load_attach_bpf_xdp_pass(char *ifname) { return 0; } -int Load_bpf_pin_xsk_map(char *ifname, char *pin_path) { - struct bpf_object *obj; - struct xdp_program *prog; - struct bpf_link *link; - int ifindex, map_fd = -1; - int err; - const char *prog_name = "xdp_afxdp_redirect"; - char *filename = "/afxdp/xdp_afxdp_redirect.o"; - DECLARE_LIBBPF_OPTS(bpf_object_open_opts, bpf_opts, .pin_root_path = pin_path); - - ifindex = if_nametoindex(ifname); - if (!ifindex) { - Log_Error("%s: if_index not valid: %s", __FUNCTION__, ifname); - return -1; - } - Log_Info("%s: if_index for interface %s is %d", __FUNCTION__, ifname, ifindex); - - if (access(filename, O_RDONLY) < 0) { - Log_Error("%s:error accessing file %s: %s\n", __FUNCTION__, filename, - strerror(errno)); - return err; - } - - Log_Info("%s: starting setup of xdp-redirect program on " - "interface %s (%d)", - __FUNCTION__, ifname, ifindex); - - /* Load the BPF program */ - prog = xdp_program__open_file(filename, NULL, NULL); - err = libxdp_get_error(prog); - if (err) { - libxdp_strerror(err, "Couldn’t load XDP program", - sizeof("Couldn’t load XDP program")); - Log_Error("%s: Couldn’t load XDP program\n", __FUNCTION__, filename); - return err; - } - - /* Attach the program to the interface at the xdp hook */ - err = xdp_program__attach(prog, ifindex, XDP_FLAGS_UPDATE_IF_NOEXIST, 0); - if (err) { - libxdp_strerror(err, "Couldn't attach the xdp pass program", - sizeof("Couldn't attach the xdp pass program")); - Log_Error("%s: Couldn't attach the XDP PASS PROGRAM TO %s\n", __FUNCTION__, ifname); - return err; - } - - return 0; -} diff --git a/internal/bpf/bpfWrapper.go b/internal/bpf/bpfWrapper.go index 110cc9dd..d63f2ff1 100755 --- a/internal/bpf/bpfWrapper.go +++ b/internal/bpf/bpfWrapper.go @@ -26,6 +26,8 @@ import "C" import ( "errors" + "fmt" + "os/exec" logging "github.com/sirupsen/logrus" ) @@ -85,10 +87,24 @@ func (r *handler) LoadAttachBpfXdpPass(ifname string) error { LoadBpfPinXskMap is the GoLang wrapper for the C function Load_bpf_send_xsk_map */ func (r *handler) LoadBpfPinXskMap(ifname, pin_path string) error { - err := int(C.Load_bpf_pin_xsk_map(C.CString(ifname), C.CString(pin_path))) - if err < 0 { - return errors.New("error loading BPF program onto interface") + bpfProg := "/afxdp/xdp_afxdp_redirect.o" + + loaderCmd := "xdp-loader load " + ifname + " " + bpfProg + " -p " + pin_path + + if err := r.Cleanbpf(ifname); err != nil { + err = fmt.Errorf("cmdDel(): error removing BPF program from device: %w", err) + logging.Errorf(err.Error()) + + return err + } + + logging.Infof("Loading XDP program using: %s", loaderCmd) + + cmd := exec.Command("xdp-loader", "load", ifname, bpfProg, "-p", pin_path) + if err := cmd.Run(); err != nil { + logging.Errorf("error loading and pinning BPF program onto interface %v", err) + return errors.New("error loading and pinning BPF program onto interface") } return nil @@ -111,6 +127,7 @@ func (r *handler) ConfigureBusyPoll(fd int, busyTimeout int, busyBudget int) err Cleanbpf is the GoLang wrapper for the C function Clean_bpf */ func (r *handler) Cleanbpf(ifname string) error { + ret := C.Clean_bpf(C.CString(ifname)) if ret != 0 { diff --git a/internal/bpf/bpfWrapper.h b/internal/bpf/bpfWrapper.h index dbe181f7..bf41107d 100644 --- a/internal/bpf/bpfWrapper.h +++ b/internal/bpf/bpfWrapper.h @@ -19,7 +19,6 @@ int Load_bpf_send_xsk_map(char *ifname); int Load_attach_bpf_xdp_pass(char *ifname); -int Load_bpf_pin_xsk_map(char *ifname, char *pin_path); int Configure_busy_poll(int fd, int busy_timeout, int busy_budget); int Clean_bpf(char *ifname); diff --git a/internal/bpf/mapManager.go b/internal/bpf/mapManager.go index b505cbfb..f1e54259 100644 --- a/internal/bpf/mapManager.go +++ b/internal/bpf/mapManager.go @@ -21,6 +21,7 @@ import ( "syscall" "github.com/google/uuid" + "github.com/intel/afxdp-plugins-for-kubernetes/constants" "github.com/intel/afxdp-plugins-for-kubernetes/internal/host" "github.com/moby/sys/mount" "github.com/pkg/errors" @@ -28,7 +29,6 @@ import ( ) const ( - pinnedMapBaseDir = "/var/run/afxdp_dp/" pinnedMapDirFileMode = os.FileMode(0755) bpffsDirFileMode = os.FileMode(0755) ) @@ -38,7 +38,7 @@ MapManager is the interface defining the MAP MANAGER. Implementations of this interface are the main type of this MapManager package. TODO UPDATE */ type MapManager interface { - CreateBPFFS(dev, path string) (string, error) + CreateBPFFS() (string, error) DeleteBPFFS(dev string) error AddMap(dev, path string) GetMaps() (map[string]string, error) @@ -48,7 +48,6 @@ type MapManager interface { type PoolBpfMapManager struct { Manager MapManager - Path string } /* @@ -58,7 +57,7 @@ container is created the factory will create a MapManager to serve the associated pinned BPF Map. TODO UPDATE THIS.... */ type MapManagerFactory interface { - CreateMapManager(poolName, user string) (MapManager, string, error) + CreateMapManager(poolName, user string) (MapManager, error) } /* @@ -89,15 +88,15 @@ func NewMapMangerFactory() MapManagerFactory { CreateMapManager creates, initialises, and returns an implementation of the MapManager interface. It also returns the filepath for bpf maps to be pinned. */ -func (f *mapManagerFactory) CreateMapManager(poolName, user string) (MapManager, string, error) { +func (f *mapManagerFactory) CreateMapManager(poolName, user string) (MapManager, error) { - logging.Debugf(" CreateMapManager ") + logging.Debugf(" CreateMapManager %s ", poolName) if poolName == "" || user == "" { - return nil, "", errors.New("Error poolname or user not set") + return nil, errors.New("Error poolname or user not set") } p, err := createBPFFSBaseDirectory(poolName, user) if err != nil { - return nil, "", errors.Wrapf(err, "Error creating BPFFS base directory %v", err.Error()) + return nil, errors.Wrapf(err, "Error creating BPFFS base directory %v", err.Error()) } logging.Infof("Created BPFFS Base directory %s", p) @@ -108,7 +107,7 @@ func (f *mapManagerFactory) CreateMapManager(poolName, user string) (MapManager, name: poolName, } - return manager, p, nil + return manager, nil } func giveBpffsBasePermissions(path, user string) error { @@ -125,17 +124,17 @@ func giveBpffsBasePermissions(path, user string) error { func createBPFFSBaseDirectory(p, user string) (string, error) { - logging.Infof("Creating BPFFS Base directory %s", p) + path := constants.Bpf.PinMapBaseDir + p + "/" - path := pinnedMapBaseDir + p + logging.Infof("Creating BPFFS Base directory %s", path) if _, err := os.Stat(path); os.IsNotExist(err) { //create base directory if it not exists, with correct file permissions if err = os.MkdirAll(path, pinnedMapDirFileMode); err != nil { - return "", errors.Wrapf(err, "Error creating BPFFS base directory %s: %v", pinnedMapBaseDir, err.Error()) + return "", errors.Wrapf(err, "Error creating BPFFS base directory %s: %v", constants.Bpf.PinMapBaseDir, err.Error()) } if err = giveBpffsBasePermissions(path, user); err != nil { - return "", errors.Wrapf(err, "Error creating BPFFS base directory %s: %v", pinnedMapBaseDir, err.Error()) + return "", errors.Wrapf(err, "Error creating BPFFS base directory %s: %v", constants.Bpf.PinMapBaseDir, err.Error()) } } @@ -143,22 +142,22 @@ func createBPFFSBaseDirectory(p, user string) (string, error) { return path, nil } -func (m mapManager) CreateBPFFS(device, path string) (string, error) { - if _, err := os.Stat(path); os.IsNotExist(err) { - return "", errors.Wrapf(err, "Error creating BPFFS mount point base directory %s doesn't exist: %v", pinnedMapBaseDir, err.Error()) +func (m mapManager) CreateBPFFS() (string, error) { + if _, err := os.Stat(m.bpffsPath); os.IsNotExist(err) { + return "", errors.Wrapf(err, "Error creating BPFFS mount point base directory %s doesn't exist: %v", m.bpffsPath, err.Error()) } bpffsPath, err := generateRandomBpffsName(m.bpffsPath) if err != nil { - return "", errors.Wrapf(err, "Error generating BPFFS path: %s: %v", pinnedMapBaseDir, err.Error()) + return "", errors.Wrapf(err, "Error generating BPFFS path: %s: %v", bpffsPath, err.Error()) } if err = os.MkdirAll(bpffsPath, bpffsDirFileMode); err != nil { - return "", errors.Wrapf(err, "Error creating BPFFS base directory %s: %v", pinnedMapBaseDir, err.Error()) + return "", errors.Wrapf(err, "Error creating BPFFS base directory %s: %v", bpffsPath, err.Error()) } if err = giveBpffsBasePermissions(bpffsPath, m.uid); err != nil { - return "", errors.Wrapf(err, "Error creating BPFFS base directory %s: %v", pinnedMapBaseDir, err.Error()) + return "", errors.Wrapf(err, "Error creating BPFFS base directory %s: %v", bpffsPath, err.Error()) } logging.Infof("created a directory %s", bpffsPath) diff --git a/internal/cni/cni.go b/internal/cni/cni.go index b5841a2c..2c4b878b 100644 --- a/internal/cni/cni.go +++ b/internal/cni/cni.go @@ -338,6 +338,14 @@ func CmdDel(args *skel.CmdArgs) error { } } + if cfg.DPSyncer { + logging.Infof("cmdDel(): Asking Device Plugin to delete any BPF maps for %s", cfg.Device) + err := dpcnisyncer.DeleteNetDev(cfg.Device) + if err != nil { + logging.Errorf("cmdDel(): DeleteNetDev from Syncer Server Failed for %s: %v", cfg.Device, err) + } + } + if !cfg.SkipUnloadBpf { logging.Infof("cmdDel(): removing BPF program from device") if err := bpfHandler.Cleanbpf(cfg.Device); err != nil { @@ -365,14 +373,6 @@ func CmdDel(args *skel.CmdArgs) error { } } - if cfg.DPSyncer { - logging.Infof("cmdDel(): Asking Device Plugin to delete any BPF maps for %s", cfg.Device) - err := dpcnisyncer.DeleteNetDev(cfg.Device) - if err != nil { - logging.Errorf("cmdDel(): DeleteNetDev from Syncer Server Failed for %s: %v", cfg.Device, err) - } - } - if cfg.Mode == "cdq" { isSf, err := netHandler.IsCdqSubfunction(cfg.Device) if err != nil { diff --git a/internal/deviceplugin/poolManager.go b/internal/deviceplugin/poolManager.go index 60990180..c4982214 100644 --- a/internal/deviceplugin/poolManager.go +++ b/internal/deviceplugin/poolManager.go @@ -107,8 +107,8 @@ func (pm *PoolManager) Init(config PoolConfig) error { if pm.BpfMapPinningEnable { var err error - logging.Infof("Creating new BPF Map manager %s %s", pm.DevicePrefix+"-maps/", pm.UID) - pm.Pbm.Manager, pm.Pbm.Path, err = pm.MapManagerFactory.CreateMapManager(pm.DevicePrefix+"-maps/", pm.UID) + logging.Infof("Creating new BPF Map manager %s %s", pm.Name, pm.UID) + pm.Pbm.Manager, err = pm.MapManagerFactory.CreateMapManager(pm.Name, pm.UID) if err != nil { logging.Errorf("Error new BPF Map manager: %v", err) return err @@ -251,7 +251,7 @@ func (pm *PoolManager) Allocate(ctx context.Context, if pm.BpfMapPinningEnable { logging.Infof("Loading BPF program on device: %s and pinning the map", device.Name()) - pinPath, err := pm.Pbm.Manager.CreateBPFFS(device.Name(), pm.Pbm.Path) + pinPath, err := pm.Pbm.Manager.CreateBPFFS() if err != nil { logging.Errorf("Error Creating the BPFFS: %v", err) return &response, err