diff --git a/internal/runtimehandlerhooks/high_performance_hooks_linux.go b/internal/runtimehandlerhooks/high_performance_hooks_linux.go index 9aca130a295..466d28f8c56 100644 --- a/internal/runtimehandlerhooks/high_performance_hooks_linux.go +++ b/internal/runtimehandlerhooks/high_performance_hooks_linux.go @@ -61,11 +61,56 @@ const ( SharedCPUsEnvVar = "OPENSHIFT_SHARED_CPUS" ) +// ServiceManager interface for managing system services. +type ServiceManager interface { + IsServiceEnabled(serviceName string) bool + RestartService(serviceName string) error +} + +// CommandRunner interface for running external commands. +type CommandRunner interface { + LookPath(file string) (string, error) + RunCommand(name string, env []string, arg ...string) error +} + +// Default implementations. +type defaultServiceManager struct{} + +func (d *defaultServiceManager) IsServiceEnabled(serviceName string) bool { + return isServiceEnabled(serviceName) +} + +func (d *defaultServiceManager) RestartService(serviceName string) error { + return restartService(serviceName) +} + +type defaultCommandRunner struct{} + +func (d *defaultCommandRunner) LookPath(file string) (string, error) { + return exec.LookPath(file) +} + +func (d *defaultCommandRunner) RunCommand(name string, env []string, arg ...string) error { + cmd := cmdrunner.Command(name, arg...) + if len(env) > 0 { + cmd.Env = env + } + + return cmd.Run() +} + +var ( + serviceManager ServiceManager = &defaultServiceManager{} + commandRunner CommandRunner = &defaultCommandRunner{} +) + // HighPerformanceHooks used to run additional hooks that will configure a system for the latency sensitive workloads. type HighPerformanceHooks struct { - irqBalanceConfigFile string - cpusetLock sync.Mutex - sharedCPUs string + irqBalanceConfigFile string + cpusetLock sync.Mutex + updateIRQSMPAffinityLock sync.Mutex + sharedCPUs string + irqSMPAffinityFile string } func (h *HighPerformanceHooks) PreCreate(ctx context.Context, specgen *generate.Generator, s *sandbox.Sandbox, c *oci.Container) error { @@ -133,7 +178,8 @@ func (h *HighPerformanceHooks) PreStart(ctx context.Context, c *oci.Container, s // disable the IRQ smp load balancing for the container CPUs if shouldIRQLoadBalancingBeDisabled(ctx, s.Annotations()) { log.Infof(ctx, "Disable irq smp balancing for container %q", c.ID()) - if err := setIRQLoadBalancing(ctx, c, false, IrqSmpAffinityProcFile, h.irqBalanceConfigFile); err != nil { + + if err := h.setIRQLoadBalancing(ctx, c, false); err != nil { return fmt.Errorf("set IRQ load balancing: %w", err) } } @@ -183,13 +229,6 @@ func (h *HighPerformanceHooks) PreStop(ctx context.Context, c *oci.Container, s return nil } - // enable the IRQ smp balancing for the container CPUs - if shouldIRQLoadBalancingBeDisabled(ctx, s.Annotations()) { - if err := setIRQLoadBalancing(ctx, c, true, IrqSmpAffinityProcFile, h.irqBalanceConfigFile); err != nil { - return fmt.Errorf("set IRQ load balancing: %w", err) - } - } - // disable the CPU load balancing for the container CPUs if shouldCPULoadBalancingBeDisabled(ctx, s.Annotations()) { podManager, containerManagers, err := libctrManagersForPodAndContainerCgroup(c, s.CgroupParent()) @@ -225,12 +264,23 @@ func (h *HighPerformanceHooks) PreStop(ctx context.Context, c *oci.Container, s } // If CPU load balancing is enabled, then *all* containers must run this PostStop hook. -func (*HighPerformanceHooks) PostStop(ctx context.Context, c *oci.Container, s *sandbox.Sandbox) error { +func (h *HighPerformanceHooks) PostStop(ctx context.Context, c *oci.Container, s *sandbox.Sandbox) error { + cSpec := c.Spec() + if shouldRunHooks(ctx, c.ID(), &cSpec, s) { + // enable the IRQ smp balancing for the container CPUs + if shouldIRQLoadBalancingBeDisabled(ctx, s.Annotations()) { + if err := h.setIRQLoadBalancing(ctx, c, true); err != nil { + return fmt.Errorf("set IRQ load balancing: %w", err) + } + } + } + // We could check if `!cpuLoadBalancingAllowed()` here, but it requires access to the config, which would be // odd to plumb. Instead, always assume if they're using a HighPerformanceHook, they have CPULoadBalanceDisabled // annotation allowed. - h := &DefaultCPULoadBalanceHooks{} - return h.PostStop(ctx, c, s) + dh := &DefaultCPULoadBalanceHooks{} + + return dh.PostStop(ctx, c, s) } func shouldCPULoadBalancingBeDisabled(ctx context.Context, annotations fields.Set) bool { @@ -537,7 +587,17 @@ func disableCPULoadBalancingV1(containerManagers []cgroups.Manager) error { return nil } -func setIRQLoadBalancing(ctx context.Context, c *oci.Container, enable bool, irqSmpAffinityFile, irqBalanceConfigFile string) error { +// setIRQLoadBalancing configures interrupt load balancing for container CPUs by updating +// the IRQ SMP affinity mask and IRQ balance service configuration. It then handles IRQ balance restart. +// When enable is false (= container added), removes container CPUs from interrupt handling to reduce latency; +// when true (= container removed), restores them. +// The entire function after reading IRQ SMP affinity must be wrapped inside a single lock to avoid race conditions. +// The reason for this is that once we read from the SMP IRQ affinity file, we have to calculate new masks and +// write those masks to /proc/irq/default_smp_affinity and /etc/sysconfig/irqbalance. +// We also must restart irqbalance or run irqbalance --oneshot within the same lock. +// Without this lock, 2 threads could read from the file and calculate the new mask but overwrite the +// results of each other, or start irbalance --oneshot with different values. +func (h *HighPerformanceHooks) setIRQLoadBalancing(ctx context.Context, c *oci.Container, enable bool) error { lspec := c.Spec().Linux if lspec == nil || lspec.Resources == nil || @@ -546,44 +606,110 @@ func setIRQLoadBalancing(ctx context.Context, c *oci.Container, enable bool, irq return fmt.Errorf("find container %s CPUs", c.ID()) } - content, err := os.ReadFile(irqSmpAffinityFile) + h.updateIRQSMPAffinityLock.Lock() + defer h.updateIRQSMPAffinityLock.Unlock() + + newIRQBalanceSetting, err := h.updateNewIRQSMPAffinityMask(ctx, c.ID(), c.Name(), lspec.Resources.CPU.Cpus, enable) if err != nil { return err } - currentIRQSMPSetting := strings.TrimSpace(string(content)) - newIRQSMPSetting, newIRQBalanceSetting, err := UpdateIRQSmpAffinityMask(lspec.Resources.CPU.Cpus, currentIRQSMPSetting, enable) + // Now, restart the irqbalance service or run irqbalance --oneshot command. + // On failure, this will log errors but will not return them, as it is not critical for the pod to start. + if !h.handleIRQBalanceRestart(ctx, c.Name()) { + h.handleIRQBalanceOneShot(ctx, c.Name(), newIRQBalanceSetting) + } + return nil +} + +// handleIRQBalanceRestart handles the restart of the irqbalance service. +func (h *HighPerformanceHooks) handleIRQBalanceRestart(ctx context.Context, cName string) bool { + // If the irqbalance service is enabled, restart it and return. + // systemd's StartLimitBurst might cause issues here when container restarts occur in very + // quick succession and the parameter must be reconfigured for this to work correctly. + // See: + // https://github.com/cri-o/cri-o/pull/8834/commits/b96928dcbb7956e0ebde42238e88955831411216 + if !serviceManager.IsServiceEnabled(irqBalancedName) || !fileExists(h.irqBalanceConfigFile) { + return false + } + + log.Debugf(ctx, "Container %q restarting irqbalance service", cName) + + if err := serviceManager.RestartService(irqBalancedName); err != nil { + log.Warnf(ctx, "Irqbalance service restart failed: %v", err) + + return false + } + + return true +} + +// handleIRQBalanceOneShot runs irqbalance --oneshot command. +func (h *HighPerformanceHooks) handleIRQBalanceOneShot(ctx context.Context, cName, newIRQBalanceSetting string) { + irqBalanceFullPath, err := commandRunner.LookPath(irqBalancedName) if err != nil { - return err + // irqbalance is not installed, skip the rest; pod should still start, so return nil instead. + log.Warnf(ctx, "Irqbalance binary not found: %v", err) + + return } - if err := os.WriteFile(irqSmpAffinityFile, []byte(newIRQSMPSetting), 0o644); err != nil { - return err + + env := fmt.Sprintf("%s=%s", irqBalanceBannedCpus, newIRQBalanceSetting) + log.Debugf(ctx, "Container %q running '%s %s %s'", cName, env, irqBalanceFullPath, "--oneshot") + + if err := commandRunner.RunCommand( + irqBalanceFullPath, + []string{env}, + "--oneshot", + ); err != nil { + log.Warnf(ctx, "Container %q failed to run '%s %s %s', err: %q", + cName, env, irqBalanceFullPath, "--oneshot", err) + } +} + +// updateNewIRQSMPAffinityMask updates SMP IRQ affinity and IRQ balance configuration files. +func (h *HighPerformanceHooks) updateNewIRQSMPAffinityMask(ctx context.Context, cID, cName, cpus string, enable bool) (newIRQBalanceSetting string, err error) { + content, err := os.ReadFile(h.irqSMPAffinityFile) + if err != nil { + return "", err } - isIrqConfigExists := fileExists(irqBalanceConfigFile) + originalIRQSMPSetting := strings.TrimSpace(string(content)) - if isIrqConfigExists { - if err := updateIrqBalanceConfigFile(irqBalanceConfigFile, newIRQBalanceSetting); err != nil { - return err - } + newIRQSMPSetting, newIRQBalanceSetting, err := calcIRQSMPAffinityMask(cpus, originalIRQSMPSetting, enable) + if err != nil { + return "", err } - if !isServiceEnabled(irqBalancedName) || !isIrqConfigExists { - if _, err := exec.LookPath(irqBalancedName); err != nil { - // irqbalance is not installed, skip the rest; pod should still start, so return nil instead - log.Warnf(ctx, "Irqbalance binary not found: %v", err) - return nil + log.Debugf(ctx, "Container %q (%q) enable %t cpus %q set %q: %q -> %q; %q: %q", + cID, cName, enable, cpus, + h.irqSMPAffinityFile, originalIRQSMPSetting, newIRQSMPSetting, + h.irqBalanceConfigFile, newIRQBalanceSetting, + ) + + if err := os.WriteFile(h.irqSMPAffinityFile, []byte(newIRQSMPSetting), 0o644); err != nil { + return "", err + } + + // Rollback IRQ SMP affinity file to maintain consistency if something goes wrong. + defer func() { + if err != nil { + if rollbackErr := os.WriteFile(h.irqSMPAffinityFile, []byte(originalIRQSMPSetting), 0o644); rollbackErr != nil { + log.Errorf(ctx, "Failed to rollback IRQ SMP affinity file after config update failure: err: %q, rollback err: %q", + err, rollbackErr) + } } - // run irqbalance in daemon mode, so this won't cause delay - cmd := cmdrunner.Command(irqBalancedName, "--oneshot") - additionalEnv := irqBalanceBannedCpus + "=" + newIRQBalanceSetting - cmd.Env = append(os.Environ(), additionalEnv) - return cmd.Run() + }() + + // Nothing else to do if irq balance config file does not exist. + if !fileExists(h.irqBalanceConfigFile) { + return newIRQBalanceSetting, nil } - if err := restartIrqBalanceService(); err != nil { - log.Warnf(ctx, "Irqbalance service restart failed: %v", err) + if err := updateIrqBalanceConfigFile(h.irqBalanceConfigFile, newIRQBalanceSetting); err != nil { + return "", err } - return nil + + return newIRQBalanceSetting, nil } func setCPUQuota(podManager cgroups.Manager, containerManagers []cgroups.Manager) error { @@ -957,26 +1083,15 @@ func RestoreIrqBalanceConfig(ctx context.Context, irqBalanceConfigFile, irqBanne if err := updateIrqBalanceConfigFile(irqBalanceConfigFile, origBannedCPUMasks); err != nil { return err } - if isServiceEnabled(irqBalancedName) { - if err := restartIrqBalanceService(); err != nil { + + if serviceManager.IsServiceEnabled(irqBalancedName) { + if err := serviceManager.RestartService(irqBalancedName); err != nil { log.Warnf(ctx, "Irqbalance service restart failed: %v", err) } } return nil } -func ShouldCPUQuotaBeDisabled(ctx context.Context, cid string, cSpec *specs.Spec, s *sandbox.Sandbox, annotations fields.Set) bool { - if !shouldRunHooks(ctx, cid, cSpec, s) { - return false - } - if annotations[crioannotations.CPUQuotaAnnotation] == annotationTrue { - log.Warnf(ctx, "%s", annotationValueDeprecationWarning(crioannotations.CPUQuotaAnnotation)) - } - - return annotations[crioannotations.CPUQuotaAnnotation] == annotationTrue || - annotations[crioannotations.CPUQuotaAnnotation] == annotationDisable -} - func shouldRunHooks(ctx context.Context, id string, cSpec *specs.Spec, s *sandbox.Sandbox) bool { if isCgroupParentBurstable(s) { log.Infof(ctx, "Container %q is a burstable pod. Skip PreStart.", id) diff --git a/internal/runtimehandlerhooks/high_performance_hooks_test.go b/internal/runtimehandlerhooks/high_performance_hooks_test.go index 943b4dfbd96..6998a8b5249 100644 --- a/internal/runtimehandlerhooks/high_performance_hooks_test.go +++ b/internal/runtimehandlerhooks/high_performance_hooks_test.go @@ -2,9 +2,13 @@ package runtimehandlerhooks import ( "context" + "errors" + "fmt" "os" "path/filepath" + "strconv" "strings" + "sync" "time" . "github.com/onsi/ginkgo/v2" @@ -13,10 +17,12 @@ import ( "github.com/opencontainers/runtime-tools/generate" types "k8s.io/cri-api/pkg/apis/runtime/v1" + "github.com/cri-o/cri-o/internal/hostport" "github.com/cri-o/cri-o/internal/lib/sandbox" "github.com/cri-o/cri-o/internal/log" "github.com/cri-o/cri-o/internal/oci" crioannotations "github.com/cri-o/cri-o/pkg/annotations" + "github.com/cri-o/cri-o/pkg/config" ) const ( @@ -33,6 +39,66 @@ const ( governorUserspace = "userspace" ) +type mockServiceManager struct { + isServiceEnabled map[string]bool + restartService map[string]error + history []string +} + +func (m *mockServiceManager) IsServiceEnabled(serviceName string) bool { + m.history = append(m.history, "systemctl is-enabled "+serviceName) + if m.isServiceEnabled == nil { + return false + } + + return m.isServiceEnabled[serviceName] +} + +func (m *mockServiceManager) RestartService(serviceName string) error { + m.history = append(m.history, "systemctl restart "+serviceName) + if m.restartService == nil { + return errors.New("service not found") + } + + if _, ok := m.restartService[serviceName]; !ok { + return errors.New("service not found") + } + + return m.restartService[serviceName] +} + +type mockCommandRunner struct { + lookPath map[string]struct { + path string + err error + } + history []string +} + +func (m *mockCommandRunner) LookPath(file string) (string, error) { + m.history = append(m.history, "which "+file) + if m.lookPath == nil { + return "", errors.New("path not found") + } + + if _, ok := m.lookPath[file]; !ok { + return "", errors.New("path not found") + } + + return m.lookPath[file].path, m.lookPath[file].err +} + +func (m *mockCommandRunner) RunCommand(name string, env []string, arg ...string) error { + m.history = append(m.history, fmt.Sprintf( + "%s %s %s", + strings.Join(env, " "), + name, + strings.Join(arg, " "), + )) + + return nil +} + // The actual test suite. var _ = Describe("high_performance_hooks", func() { container, err := oci.NewContainer("containerID", "", "", "", @@ -58,7 +124,11 @@ var _ = Describe("high_performance_hooks", func() { irqSmpAffinityFile := filepath.Join(fixturesDir, "irq_smp_affinity") irqBalanceConfigFile := filepath.Join(fixturesDir, "irqbalance") verifySetIRQLoadBalancing := func(enabled bool, expected string) { - err := setIRQLoadBalancing(context.TODO(), container, enabled, irqSmpAffinityFile, irqBalanceConfigFile) + h := &HighPerformanceHooks{ + irqBalanceConfigFile: irqBalanceConfigFile, + irqSMPAffinityFile: irqSmpAffinityFile, + } + err := h.setIRQLoadBalancing(context.TODO(), container, enabled) Expect(err).ToNot(HaveOccurred()) content, err := os.ReadFile(irqSmpAffinityFile) @@ -111,7 +181,11 @@ var _ = Describe("high_performance_hooks", func() { irqSmpAffinityFile := filepath.Join(fixturesDir, "irq_smp_affinity") irqBalanceConfigFile := filepath.Join(fixturesDir, "irqbalance") verifySetIRQLoadBalancing := func(enabled bool, expectedSmp, expectedBan string) { - err = setIRQLoadBalancing(context.TODO(), container, enabled, irqSmpAffinityFile, irqBalanceConfigFile) + h := &HighPerformanceHooks{ + irqBalanceConfigFile: irqBalanceConfigFile, + irqSMPAffinityFile: irqSmpAffinityFile, + } + err = h.setIRQLoadBalancing(context.TODO(), container, enabled) Expect(err).ToNot(HaveOccurred()) content, err := os.ReadFile(irqSmpAffinityFile) @@ -539,9 +613,12 @@ var _ = Describe("high_performance_hooks", func() { }) Describe("restoreIrqBalanceConfig", func() { + var mockSvcMgr *mockServiceManager + irqSmpAffinityFile := filepath.Join(fixturesDir, "irq_smp_affinity") irqBalanceConfigFile := filepath.Join(fixturesDir, "irqbalance") irqBannedCPUConfigFile := filepath.Join(fixturesDir, "orig_irq_banned_cpus") + verifyRestoreIrqBalanceConfig := func(expectedOrigBannedCPUs, expectedBannedCPUs string) { err = RestoreIrqBalanceConfig(context.TODO(), irqBalanceConfigFile, irqBannedCPUConfigFile, irqSmpAffinityFile) ExpectWithOffset(1, err).ToNot(HaveOccurred()) @@ -567,6 +644,21 @@ var _ = Describe("high_performance_hooks", func() { bannedCPUs, err := retrieveIrqBannedCPUMasks(irqBalanceConfigFile) Expect(err).ToNot(HaveOccurred()) Expect(bannedCPUs).To(Equal("0000ffff,ffffcfcc")) + + mockSvcMgr = &mockServiceManager{ + isServiceEnabled: map[string]bool{ + "irqbalance": true, + }, + restartService: map[string]error{ + "irqbalance": nil, + }, + history: []string{}, + } + serviceManager = mockSvcMgr + }) + + JustAfterEach(func() { + serviceManager = &defaultServiceManager{} }) Context("when banned cpu config file doesn't exist", func() { @@ -577,6 +669,8 @@ var _ = Describe("high_performance_hooks", func() { It("should set banned cpu config file from irq balance config", func() { verifyRestoreIrqBalanceConfig("0000ffff,ffffcfcc", "0000ffff,ffffcfcc") + Expect(mockSvcMgr.history).NotTo(ContainElement("systemctl is-enabled irqbalance")) + Expect(mockSvcMgr.history).NotTo(ContainElement("systemctl restart irqbalance")) }) }) @@ -590,10 +684,189 @@ var _ = Describe("high_performance_hooks", func() { It("should restore irq balance config with content from banned cpu config file", func() { verifyRestoreIrqBalanceConfig("00000000,00000000", "00000000,00000000") + Expect(mockSvcMgr.history).To(ContainElement("systemctl is-enabled irqbalance")) + Expect(mockSvcMgr.history).To(ContainElement("systemctl restart irqbalance")) }) }) }) + Describe("handleIRQBalanceRestart", func() { + irqBalanceConfigFile := filepath.Join(fixturesDir, "irqbalance") + + h := &HighPerformanceHooks{ + irqBalanceConfigFile: irqBalanceConfigFile, + } + + type parameters struct { + isServiceEnabled bool + irqBalanceFileExists bool + restartServiceSucceeds bool + pathLookupError bool + calculatedIRQBalanceMask string + } + + DescribeTable("handleIRQBalanceRestart scenarios", + func(p parameters, serviceMgrHistory, cmdRunnerHistory []string) { + defer func() { + // Reset global mocks. + serviceManager = &defaultServiceManager{} + commandRunner = &defaultCommandRunner{} + }() + + // Setup mocks according to parameters and irqbalance config file. + mockSvcMgr := &mockServiceManager{ + isServiceEnabled: map[string]bool{ + "irqbalance": p.isServiceEnabled, + }, + history: []string{}, + } + mockCmdRunner := &mockCommandRunner{ + history: []string{}, + } + + if p.restartServiceSucceeds { + mockSvcMgr.restartService = map[string]error{ + "irqbalance": nil, + } + } else { + mockSvcMgr.restartService = map[string]error{ + "irqbalance": errors.New("restart failed"), + } + } + + if p.pathLookupError { + mockCmdRunner.lookPath = map[string]struct { + path string + err error + }{ + "irqbalance": {path: "", err: errors.New("not found")}, + } + } else { + mockCmdRunner.lookPath = map[string]struct { + path string + err error + }{ + "irqbalance": {path: "/usr/bin/irqbalance", err: nil}, + } + } + if p.irqBalanceFileExists { + err = os.WriteFile(irqBalanceConfigFile, []byte(""), 0o644) + Expect(err).ToNot(HaveOccurred()) + err = updateIrqBalanceConfigFile(irqBalanceConfigFile, p.calculatedIRQBalanceMask) + Expect(err).ToNot(HaveOccurred()) + } + serviceManager = mockSvcMgr + commandRunner = mockCmdRunner + + // Execute application logic. + if !h.handleIRQBalanceRestart(context.TODO(), "container-name") { + h.handleIRQBalanceOneShot(context.TODO(), "container-name", p.calculatedIRQBalanceMask) + } + + // Verify behavior based on scenario. + Expect(mockSvcMgr.history).To(Equal(serviceMgrHistory)) + Expect(mockCmdRunner.history).To(Equal(cmdRunnerHistory)) + }, + Entry("irqbalance is enabled and succeeds", + parameters{isServiceEnabled: true, irqBalanceFileExists: true, restartServiceSucceeds: true, pathLookupError: false, calculatedIRQBalanceMask: "ffff,ffff"}, + []string{ + "systemctl is-enabled irqbalance", + "systemctl restart irqbalance", + }, + []string{}), + Entry("irqbalance is enabled but irqbalance file does not exist", + parameters{isServiceEnabled: true, irqBalanceFileExists: false, restartServiceSucceeds: false, pathLookupError: false, calculatedIRQBalanceMask: "ffff,ffff"}, + []string{ + "systemctl is-enabled irqbalance", + }, + []string{ + "which irqbalance", + "IRQBALANCE_BANNED_CPUS=ffff,ffff /usr/bin/irqbalance --oneshot", + }), + Entry("irqbalance is enabled and fails but oneshot works", + parameters{isServiceEnabled: true, irqBalanceFileExists: true, restartServiceSucceeds: false, pathLookupError: false, calculatedIRQBalanceMask: "ffff,ffff"}, + []string{ + "systemctl is-enabled irqbalance", + "systemctl restart irqbalance", + }, + []string{ + "which irqbalance", + "IRQBALANCE_BANNED_CPUS=ffff,ffff /usr/bin/irqbalance --oneshot", + }), + Entry("irqbalance is disabled but irqBalance file exists", + parameters{isServiceEnabled: false, irqBalanceFileExists: true, restartServiceSucceeds: false, pathLookupError: false, calculatedIRQBalanceMask: "ffff,ffff"}, + []string{ + "systemctl is-enabled irqbalance", + }, + []string{ + "which irqbalance", + "IRQBALANCE_BANNED_CPUS=ffff,ffff /usr/bin/irqbalance --oneshot", + }), + Entry("irqbalance is disabled, oneshot lookup fails", + parameters{isServiceEnabled: false, irqBalanceFileExists: true, restartServiceSucceeds: false, pathLookupError: true, calculatedIRQBalanceMask: "ffff,ffff"}, + []string{ + "systemctl is-enabled irqbalance", + }, + []string{ + "which irqbalance", + }), + ) + }) + + Describe("updateNewIRQSMPAffinityMask rollback", func() { + irqBalanceConfigFile := filepath.Join(fixturesDir, "irqbalance") + irqSMPAffinityFile := filepath.Join(fixturesDir, "irqsmpaffinity") + + h := &HighPerformanceHooks{ + irqSMPAffinityFile: irqSMPAffinityFile, + irqBalanceConfigFile: irqBalanceConfigFile, + } + + type parameters struct { + irqBalanceFileRO bool + originalIRQSMPAffinityMask string + expectedIRQSMPAffinityMask string + } + + DescribeTable("test rollback", + func(p parameters) { + err := os.WriteFile(irqSMPAffinityFile, []byte(p.originalIRQSMPAffinityMask), 0o644) + Expect(err).ToNot(HaveOccurred()) + + if p.irqBalanceFileRO { + err = os.Symlink("/proc/version", irqBalanceConfigFile) + Expect(err).ToNot(HaveOccurred()) + } else { + err = os.WriteFile(irqBalanceConfigFile, []byte(""), 0o644) + Expect(err).ToNot(HaveOccurred()) + } + + _, err = h.updateNewIRQSMPAffinityMask(context.TODO(), "cID", "CName", "2-3", false) + if p.irqBalanceFileRO { + Expect(err).To(HaveOccurred()) + } else { + Expect(err).NotTo(HaveOccurred()) + } + + writtenMask, err := os.ReadFile(irqSMPAffinityFile) + Expect(err).NotTo(HaveOccurred()) + Expect(string(writtenMask)).To(Equal(p.expectedIRQSMPAffinityMask)) + }, + Entry("writing IRQ balance file fails", + parameters{ + irqBalanceFileRO: true, + originalIRQSMPAffinityMask: "ffffffff", + expectedIRQSMPAffinityMask: "ffffffff", + }), + Entry("writing IRQ balance file succeeds", + parameters{ + irqBalanceFileRO: false, + originalIRQSMPAffinityMask: "ffffffff", + expectedIRQSMPAffinityMask: "fffffff3", + }), + ) + }) + Describe("convertAnnotationToLatency", func() { verifyConvertAnnotationToLatency := func(annotation string, expected string, expect_error bool) { latency, err := convertAnnotationToLatency(annotation) @@ -738,4 +1011,340 @@ var _ = Describe("high_performance_hooks", func() { Expect(env).To(ContainElements("OPENSHIFT_ISOLATED_CPUS=1-2", "OPENSHIFT_SHARED_CPUS=3-4")) }) }) + Describe("Make sure that correct runtime handler hooks are set", func() { + var runtimeName string + var sandboxAnnotations map[string]string + var sb *sandbox.Sandbox + var cfg *config.Config + var hooksRetriever *HooksRetriever + + formatIRQBalanceBannedCPUs := func(v string) string { + return fmt.Sprintf("%s=%q", irqBalanceBannedCpus, v) + } + + irqSmpAffinityFile := filepath.Join(fixturesDir, "irq_smp_affinity") + irqBalanceConfigFile := filepath.Join(fixturesDir, "irqbalance") + flags = "0000,0000ffff" + bannedCPUFlags = "ffffffff,ffff0000" + + ctx := context.Background() + + verifySetIRQLoadBalancing := func(expectedIrqSmp, expectedIrqBalance string) { + content, err := os.ReadFile(irqSmpAffinityFile) + Expect(err).ToNot(HaveOccurred()) + Expect(strings.Trim(string(content), "\n")).To(Equal(expectedIrqSmp)) + + content, err = os.ReadFile(irqBalanceConfigFile) + Expect(err).ToNot(HaveOccurred()) + Expect(strings.Trim(string(content), "\n")).To(Equal(formatIRQBalanceBannedCPUs(expectedIrqBalance))) + } + + createContainer := func(cpus string) (*oci.Container, error) { + container, err := oci.NewContainer("containerID", "", "", "", + make(map[string]string), make(map[string]string), + make(map[string]string), "pauseImage", nil, nil, "", + &types.ContainerMetadata{}, "sandboxID", false, false, + false, "", "", time.Now(), "") + if err != nil { + return nil, err + } + var cpuShares uint64 = 1024 + container.SetSpec( + &specs.Spec{ + Linux: &specs.Linux{ + Resources: &specs.LinuxResources{ + CPU: &specs.LinuxCPU{ + Cpus: cpus, + Shares: &cpuShares, + }, + }, + }, + }, + ) + + return container, nil + } + + JustBeforeEach(func() { + // Simulate a restart of crio each time as we're modifying the config between runs. + cpuLoadBalancingAllowedAnywhereOnce = sync.Once{} + + hooksRetriever = NewHooksRetriever(ctx, cfg) + + // create tests affinity file + err = os.WriteFile(irqSmpAffinityFile, []byte(flags), 0o644) + Expect(err).ToNot(HaveOccurred()) + err = os.WriteFile(irqBalanceConfigFile, []byte(formatIRQBalanceBannedCPUs(bannedCPUFlags)), 0o644) + Expect(err).ToNot(HaveOccurred()) + + sbox := sandbox.NewBuilder() + createdAt := time.Now() + sbox.SetCreatedAt(createdAt) + sbox.SetID("sandboxID") + sbox.SetName("sandboxName") + sbox.SetLogDir("test") + sbox.SetShmPath("test") + sbox.SetNamespace("") + sbox.SetKubeName("") + sbox.SetMountLabel("test") + sbox.SetProcessLabel("test") + sbox.SetCgroupParent("") + sbox.SetRuntimeHandler(runtimeName) + sbox.SetResolvPath("") + sbox.SetHostname("") + sbox.SetPortMappings([]*hostport.PortMapping{}) + sbox.SetHostNetwork(false) + sbox.SetUsernsMode("") + sbox.SetPodLinuxOverhead(nil) + sbox.SetPodLinuxResources(nil) + err = sbox.SetCRISandbox( + sbox.ID(), + map[string]string{}, + sandboxAnnotations, + &types.PodSandboxMetadata{}, + ) + Expect(err).ToNot(HaveOccurred()) + sbox.SetPrivileged(false) + sbox.SetHostNetwork(false) + sbox.SetCreatedAt(createdAt) + sb, err = sbox.GetSandbox() + Expect(err).ToNot(HaveOccurred()) + }) + + Context("with runtime name high-performance and sandbox disable annotation", func() { + BeforeEach(func() { + runtimeName = "high-performance" + sandboxAnnotations = map[string]string{crioannotations.IRQLoadBalancingAnnotation: "disable"} + cfg = &config.Config{ + RuntimeConfig: config.RuntimeConfig{ + IrqBalanceConfigFile: irqBalanceConfigFile, + Runtimes: config.Runtimes{ + "high-performance": { + AllowedAnnotations: []string{}, + }, + "default": {}, + }, + }, + } + }) + + It("should set the correct irq bit mask with concurrency", func() { + hooks := hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()) + Expect(hooks).NotTo(BeNil()) + if hph, ok := hooks.(*HighPerformanceHooks); ok { + hph.irqSMPAffinityFile = irqSmpAffinityFile + hph.irqBalanceConfigFile = irqBalanceConfigFile + } + var wg sync.WaitGroup + for cpu := range 16 { + wg.Add(1) + go func() { + defer wg.Done() + container, err := createContainer(strconv.Itoa(cpu)) + Expect(err).ToNot(HaveOccurred()) + err = hooks.PreStart(ctx, container, sb) + Expect(err).ToNot(HaveOccurred()) + }() + } + wg.Wait() + verifySetIRQLoadBalancing("00000000,00000000", "ffffffff,ffffffff") + }) + }) + + Context("with runtime name high-performance and sandbox without any annotation", func() { + BeforeEach(func() { + runtimeName = "high-performance" + sandboxAnnotations = map[string]string{} + cfg = &config.Config{ + RuntimeConfig: config.RuntimeConfig{ + IrqBalanceConfigFile: irqBalanceConfigFile, + Runtimes: config.Runtimes{ + "high-performance": { + AllowedAnnotations: []string{}, + }, + "default": {}, + }, + }, + } + }) + + It("should keep the current irq bit mask but return a high performance hooks", func() { + hooks := hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()) + Expect(hooks).NotTo(BeNil()) + hph, ok := hooks.(*HighPerformanceHooks) + Expect(ok).To(BeTrue()) + hph.irqSMPAffinityFile = irqSmpAffinityFile + hph.irqBalanceConfigFile = irqBalanceConfigFile + + var wg sync.WaitGroup + for cpu := range 16 { + wg.Add(1) + go func() { + defer wg.Done() + container, err := createContainer(strconv.Itoa(cpu)) + Expect(err).ToNot(HaveOccurred()) + err = hooks.PreStart(ctx, container, sb) + Expect(err).ToNot(HaveOccurred()) + }() + } + wg.Wait() + verifySetIRQLoadBalancing(flags, bannedCPUFlags) + }) + }) + + Context("with runtime name hp and sandbox disable annotation", func() { + BeforeEach(func() { + runtimeName = "hp" + sandboxAnnotations = map[string]string{crioannotations.IRQLoadBalancingAnnotation: "disable"} + cfg = &config.Config{ + RuntimeConfig: config.RuntimeConfig{ + IrqBalanceConfigFile: irqBalanceConfigFile, + Runtimes: config.Runtimes{ + "hp": { + AllowedAnnotations: []string{ + crioannotations.IRQLoadBalancingAnnotation, + }, + }, + "default": {}, + }, + }, + } + }) + + It("should set the correct irq bit mask with concurrency", func() { + hooks := hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()) + Expect(hooks).NotTo(BeNil()) + if hph, ok := hooks.(*HighPerformanceHooks); ok { + hph.irqSMPAffinityFile = irqSmpAffinityFile + hph.irqBalanceConfigFile = irqBalanceConfigFile + } + var wg sync.WaitGroup + for cpu := range 16 { + wg.Add(1) + go func() { + defer wg.Done() + container, err := createContainer(strconv.Itoa(cpu)) + Expect(err).ToNot(HaveOccurred()) + err = hooks.PreStart(ctx, container, sb) + Expect(err).ToNot(HaveOccurred()) + }() + } + wg.Wait() + verifySetIRQLoadBalancing("00000000,00000000", "ffffffff,ffffffff") + }) + }) + + Context("with runtime name hp and sandbox without any annotation", func() { + BeforeEach(func() { + runtimeName = "hp" + sandboxAnnotations = map[string]string{} + cfg = &config.Config{ + RuntimeConfig: config.RuntimeConfig{ + IrqBalanceConfigFile: irqBalanceConfigFile, + Runtimes: config.Runtimes{ + "hp": { + AllowedAnnotations: []string{ + crioannotations.IRQLoadBalancingAnnotation, + }, + }, + "default": {}, + }, + }, + } + }) + + It("should return a nil hook", func() { + hooks := hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()) + Expect(hooks).To(BeNil()) + }) + }) + + // The following test case should never happen in the real world. However, it makes sure that the checks + // actually look at the runtime name and at the sandbox annotation and if _either_ signals that high performance + // hooks should be enabled then enable them. + Context("with runtime name default and sandbox disable annotation", func() { + BeforeEach(func() { + runtimeName = "default" + sandboxAnnotations = map[string]string{crioannotations.IRQLoadBalancingAnnotation: "disable"} + cfg = &config.Config{ + RuntimeConfig: config.RuntimeConfig{ + IrqBalanceConfigFile: irqBalanceConfigFile, + Runtimes: config.Runtimes{ + "default": {}, + }, + }, + } + }) + + It("should set the correct irq bit mask with concurrency", func() { + hooks := hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()) + Expect(hooks).NotTo(BeNil()) + if hph, ok := hooks.(*HighPerformanceHooks); ok { + hph.irqSMPAffinityFile = irqSmpAffinityFile + hph.irqBalanceConfigFile = irqBalanceConfigFile + } + var wg sync.WaitGroup + for cpu := range 16 { + wg.Add(1) + go func() { + defer wg.Done() + container, err := createContainer(strconv.Itoa(cpu)) + Expect(err).ToNot(HaveOccurred()) + err = hooks.PreStart(ctx, container, sb) + Expect(err).ToNot(HaveOccurred()) + }() + } + wg.Wait() + verifySetIRQLoadBalancing("00000000,00000000", "ffffffff,ffffffff") + }) + }) + + Context("with runtime name default, CPU balancing annotation present and sandbox without any annotation", func() { + BeforeEach(func() { + runtimeName = "default" + sandboxAnnotations = map[string]string{} + cfg = &config.Config{ + RuntimeConfig: config.RuntimeConfig{ + IrqBalanceConfigFile: irqBalanceConfigFile, + Runtimes: config.Runtimes{ + "high-performance": { + AllowedAnnotations: []string{}, + }, + "hp": { + AllowedAnnotations: []string{ + crioannotations.IRQLoadBalancingAnnotation, + }, + }, + "cpu-balancing-anywhere": { + AllowedAnnotations: []string{ + crioannotations.CPULoadBalancingAnnotation, + }, + }, + "default": {}, + }, + }, + } + }) + + It("should yield a DefaultCPULoadBalanceHooks which keeps the old mask", func() { + hooks := hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()) + Expect(hooks).NotTo(BeNil()) + _, ok := (hooks).(*DefaultCPULoadBalanceHooks) + Expect(ok).To(BeTrue()) + var wg sync.WaitGroup + for cpu := range 16 { + wg.Add(1) + go func() { + defer wg.Done() + container, err := createContainer(strconv.Itoa(cpu)) + Expect(err).ToNot(HaveOccurred()) + err = hooks.PreStart(ctx, container, sb) + Expect(err).ToNot(HaveOccurred()) + }() + } + wg.Wait() + verifySetIRQLoadBalancing(flags, bannedCPUFlags) + }) + }) + }) }) diff --git a/internal/runtimehandlerhooks/runtime_handler_hooks.go b/internal/runtimehandlerhooks/runtime_handler_hooks.go index 3c286c4bbee..5a503a7ef18 100644 --- a/internal/runtimehandlerhooks/runtime_handler_hooks.go +++ b/internal/runtimehandlerhooks/runtime_handler_hooks.go @@ -8,6 +8,7 @@ import ( "github.com/cri-o/cri-o/internal/lib/sandbox" "github.com/cri-o/cri-o/internal/oci" + libconfig "github.com/cri-o/cri-o/pkg/config" ) var ( @@ -27,3 +28,9 @@ type RuntimeHandlerHooks interface { type HighPerformanceHook interface { RuntimeHandlerHooks } + +// HooksRetriever allows retrieving the runtime hooks for a given sandbox. +type HooksRetriever struct { + config *libconfig.Config + highPerformanceHooks RuntimeHandlerHooks +} diff --git a/internal/runtimehandlerhooks/runtime_handler_hooks_linux.go b/internal/runtimehandlerhooks/runtime_handler_hooks_linux.go index a3aca0f40cb..92220447ced 100644 --- a/internal/runtimehandlerhooks/runtime_handler_hooks_linux.go +++ b/internal/runtimehandlerhooks/runtime_handler_hooks_linux.go @@ -10,23 +10,56 @@ import ( libconfig "github.com/cri-o/cri-o/pkg/config" ) -// GetRuntimeHandlerHooks returns RuntimeHandlerHooks implementation by the runtime handler name. -func GetRuntimeHandlerHooks(ctx context.Context, config *libconfig.Config, handler string, annotations map[string]string) (RuntimeHandlerHooks, error) { +// NewHooksRetriever returns a pointer to a new retriever. +// Log a warning if deprecated configuration is detected. +func NewHooksRetriever(ctx context.Context, config *libconfig.Config) *HooksRetriever { ctx, span := log.StartSpan(ctx) defer span.End() - if strings.Contains(handler, HighPerformance) { - log.Warnf(ctx, "The usage of the handler %q without adding high-performance feature annotations under allowed_annotations will be deprecated under 1.21", HighPerformance) - return &HighPerformanceHooks{irqBalanceConfigFile: config.IrqBalanceConfigFile, cpusetLock: sync.Mutex{}, sharedCPUs: config.SharedCPUSet}, nil + + rhh := &HooksRetriever{ + config: config, + highPerformanceHooks: nil, } - if highPerformanceAnnotationsSpecified(annotations) { - log.Warnf(ctx, "The usage of the handler %q without adding high-performance feature annotations under allowed_annotations will be deprecated under 1.21", HighPerformance) - return &HighPerformanceHooks{irqBalanceConfigFile: config.IrqBalanceConfigFile, cpusetLock: sync.Mutex{}, sharedCPUs: config.SharedCPUSet}, nil + + for name, runtime := range config.Runtimes { + annotationMap := map[string]string{} + for _, v := range runtime.AllowedAnnotations { + annotationMap[v] = "" + } + + if strings.Contains(name, HighPerformance) && !highPerformanceAnnotationsSpecified(annotationMap) { + log.Warnf(ctx, "The usage of the handler %q without adding high-performance feature annotations under "+ + "allowed_annotations is deprecated since 1.21", HighPerformance) + } } - if cpuLoadBalancingAllowed(config) { - return &DefaultCPULoadBalanceHooks{}, nil + + return rhh +} + +// Get checks runtime name or the sandbox's annotations for allowed high performance annotations. If present, it returns +// the single instance of highPerformanceHooks. +// Otherwise, if crio's config allows CPU load balancing anywhere, return a DefaultCPULoadBalanceHooks. +// Otherwise, return nil. +func (hr *HooksRetriever) Get(runtimeName string, sandboxAnnotations map[string]string) RuntimeHandlerHooks { + if strings.Contains(runtimeName, HighPerformance) || highPerformanceAnnotationsSpecified(sandboxAnnotations) { + if hr.highPerformanceHooks == nil { + hr.highPerformanceHooks = &HighPerformanceHooks{ + irqBalanceConfigFile: hr.config.IrqBalanceConfigFile, + cpusetLock: sync.Mutex{}, + updateIRQSMPAffinityLock: sync.Mutex{}, + sharedCPUs: hr.config.SharedCPUSet, + irqSMPAffinityFile: IrqSmpAffinityProcFile, + } + } + + return hr.highPerformanceHooks + } + + if cpuLoadBalancingAllowed(hr.config) { + return &DefaultCPULoadBalanceHooks{} } - return nil, nil + return nil } func highPerformanceAnnotationsSpecified(annotations map[string]string) bool { diff --git a/internal/runtimehandlerhooks/runtime_handler_hooks_unsupported.go b/internal/runtimehandlerhooks/runtime_handler_hooks_unsupported.go index a35b8cee4bd..4893408b15a 100644 --- a/internal/runtimehandlerhooks/runtime_handler_hooks_unsupported.go +++ b/internal/runtimehandlerhooks/runtime_handler_hooks_unsupported.go @@ -13,11 +13,22 @@ const ( IrqSmpAffinityProcFile = "" ) -// GetRuntimeHandlerHooks returns RuntimeHandlerHooks implementation by the runtime handler name -func GetRuntimeHandlerHooks(ctx context.Context, config *libconfig.Config, handler string, annotations map[string]string) (RuntimeHandlerHooks, error) { +// NewHooksRetriever returns a pointer to a new retriever. +func NewHooksRetriever(ctx context.Context, config *libconfig.Config) *HooksRetriever { ctx, span := log.StartSpan(ctx) defer span.End() - return &DefaultCPULoadBalanceHooks{}, nil + + rhh := &HooksRetriever{ + config: config, + highPerformanceHooks: nil, + } + + return rhh +} + +// Get always returns DefaultCPULoadBalanceHooks for non-linux architectures. +func (hr *HooksRetriever) Get(runtimeName string, sandboxAnnotations map[string]string) RuntimeHandlerHooks { + return &DefaultCPULoadBalanceHooks{} } // RestoreIrqBalanceConfig restores irqbalance service with original banned cpu mask settings diff --git a/internal/runtimehandlerhooks/utils_linux.go b/internal/runtimehandlerhooks/utils_linux.go index beea59985fb..4d05d5c70c6 100644 --- a/internal/runtimehandlerhooks/utils_linux.go +++ b/internal/runtimehandlerhooks/utils_linux.go @@ -94,10 +94,10 @@ func isAllBitSet(in []byte) bool { return true } -// UpdateIRQSmpAffinityMask take input cpus that need to change irq affinity mask and +// calcIRQSMPAffinityMask take input cpus that need to change irq affinity mask and // the current mask string, return an update mask string and inverted mask, with those cpus // enabled or disable in the mask. -func UpdateIRQSmpAffinityMask(cpus, current string, set bool) (cpuMask, bannedCPUMask string, err error) { +func calcIRQSMPAffinityMask(cpus, current string, set bool) (cpuMask, bannedCPUMask string, err error) { podcpuset, err := cpuset.Parse(cpus) if err != nil { return cpus, "", err @@ -143,8 +143,8 @@ func UpdateIRQSmpAffinityMask(cpus, current string, set bool) (cpuMask, bannedCP return maskStringWithComma, invertedMaskStringWithComma, nil } -func restartIrqBalanceService() error { - return cmdrunner.Command("systemctl", "restart", "irqbalance").Run() +func restartService(serviceName string) error { + return cmdrunner.Command("systemctl", "restart", serviceName).Run() } func isServiceEnabled(serviceName string) bool { diff --git a/internal/runtimehandlerhooks/utils_test.go b/internal/runtimehandlerhooks/utils_test.go index 866e4cf3bb1..97f5fa6a51d 100644 --- a/internal/runtimehandlerhooks/utils_test.go +++ b/internal/runtimehandlerhooks/utils_test.go @@ -27,7 +27,7 @@ var _ = Describe("Utils", func() { DescribeTable("testing cpu mask", func(c TestData) { - mask, invMask, err := UpdateIRQSmpAffinityMask(c.input.cpus, c.input.mask, c.input.set) + mask, invMask, err := calcIRQSMPAffinityMask(c.input.cpus, c.input.mask, c.input.set) Expect(err).ToNot(HaveOccurred()) Expect(mask).To(Equal(c.expected.mask)) Expect(invMask).To(Equal(c.expected.invMask)) diff --git a/server/container_create_linux.go b/server/container_create_linux.go index b75067d5aa4..05aa170fbac 100644 --- a/server/container_create_linux.go +++ b/server/container_create_linux.go @@ -35,7 +35,6 @@ import ( "github.com/cri-o/cri-o/internal/linklogs" "github.com/cri-o/cri-o/internal/log" oci "github.com/cri-o/cri-o/internal/oci" - "github.com/cri-o/cri-o/internal/runtimehandlerhooks" "github.com/cri-o/cri-o/internal/storage" "github.com/cri-o/cri-o/internal/storage/references" crioann "github.com/cri-o/cri-o/pkg/annotations" @@ -799,10 +798,7 @@ func (s *Server) createSandboxContainer(ctx context.Context, ctr ctrfactory.Cont makeOCIConfigurationRootless(specgen) } - hooks, err := runtimehandlerhooks.GetRuntimeHandlerHooks(ctx, &s.config, sb.RuntimeHandler(), sb.Annotations()) - if err != nil { - return nil, fmt.Errorf("failed to get runtime handler %q hooks", sb.RuntimeHandler()) - } + hooks := s.hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()) if err := s.nri.createContainer(ctx, specgen, sb, ociContainer); err != nil { return nil, err diff --git a/server/container_start.go b/server/container_start.go index 93b906ea785..602b469608e 100644 --- a/server/container_start.go +++ b/server/container_start.go @@ -12,7 +12,6 @@ import ( "github.com/cri-o/cri-o/internal/lib" "github.com/cri-o/cri-o/internal/log" oci "github.com/cri-o/cri-o/internal/oci" - "github.com/cri-o/cri-o/internal/runtimehandlerhooks" ) // StartContainer starts the container. @@ -63,10 +62,8 @@ func (s *Server) StartContainer(ctx context.Context, req *types.StartContainerRe } sandbox := s.getSandbox(ctx, c.Sandbox()) - hooks, err := runtimehandlerhooks.GetRuntimeHandlerHooks(ctx, &s.config, sandbox.RuntimeHandler(), sandbox.Annotations()) - if err != nil { - return nil, fmt.Errorf("failed to get runtime handler %q hooks", sandbox.RuntimeHandler()) - } + + hooks := s.hooksRetriever.Get(sandbox.RuntimeHandler(), sandbox.Annotations()) if err := s.nri.startContainer(ctx, sandbox, c); err != nil { log.Warnf(ctx, "NRI start failed for container %q: %v", c.ID(), err) diff --git a/server/container_stop.go b/server/container_stop.go index e9928429513..28c78796ae4 100644 --- a/server/container_stop.go +++ b/server/container_stop.go @@ -12,7 +12,6 @@ import ( "github.com/cri-o/cri-o/internal/log" "github.com/cri-o/cri-o/internal/oci" - "github.com/cri-o/cri-o/internal/runtimehandlerhooks" ) // StopContainer stops a running container with a grace period (i.e., timeout). @@ -46,11 +45,7 @@ func (s *Server) stopContainer(ctx context.Context, ctr *oci.Container, timeout sb := s.getSandbox(ctx, ctr.Sandbox()) - hooks, err := runtimehandlerhooks.GetRuntimeHandlerHooks(ctx, &s.config, sb.RuntimeHandler(), sb.Annotations()) - if err != nil { - return fmt.Errorf("failed to get runtime handler %q hooks", sb.RuntimeHandler()) - } - + hooks := s.hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()) if hooks != nil { if err := hooks.PreStop(ctx, ctr, sb); err != nil { return fmt.Errorf("failed to run pre-stop hook for container %q: %w", ctr.ID(), err) diff --git a/server/sandbox_run_linux.go b/server/sandbox_run_linux.go index fdd12cff136..7e60d5a132e 100644 --- a/server/sandbox_run_linux.go +++ b/server/sandbox_run_linux.go @@ -33,7 +33,6 @@ import ( "github.com/cri-o/cri-o/internal/memorystore" oci "github.com/cri-o/cri-o/internal/oci" "github.com/cri-o/cri-o/internal/resourcestore" - "github.com/cri-o/cri-o/internal/runtimehandlerhooks" "github.com/cri-o/cri-o/pkg/annotations" libconfig "github.com/cri-o/cri-o/pkg/config" "github.com/cri-o/cri-o/utils" @@ -1038,11 +1037,7 @@ func (s *Server) runPodSandbox(ctx context.Context, req *types.RunPodSandboxRequ return nil, err } - hooks, err := runtimehandlerhooks.GetRuntimeHandlerHooks(ctx, &s.config, sb.RuntimeHandler(), sb.Annotations()) - if err != nil { - return nil, fmt.Errorf("failed to get runtime handler %q hooks", sb.RuntimeHandler()) - } - if hooks != nil { + if hooks := s.hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()); hooks != nil { if err := hooks.PreStart(ctx, container, sb); err != nil { return nil, fmt.Errorf("failed to run pre-stop hook for container %q: %w", sb.ID(), err) } diff --git a/server/server.go b/server/server.go index df32d1b34bb..ea0a1284e2d 100644 --- a/server/server.go +++ b/server/server.go @@ -91,6 +91,8 @@ type Server struct { // NRI runtime interface nri *nriAPI + // hooksRetriever allows getting the runtime hooks for the sandboxes. + hooksRetriever *runtimehandlerhooks.HooksRetriever } // pullArguments are used to identify a pullOperation via an input image name and @@ -425,7 +427,9 @@ func New( minimumMappableGID: config.MinimumMappableGID, pullOperationsInProgress: make(map[pullArguments]*pullOperation), resourceStore: resourcestore.New(), + hooksRetriever: runtimehandlerhooks.NewHooksRetriever(ctx, config), } + if s.config.EnablePodEvents { // creating a container events channel only if the evented pleg is enabled s.ContainerEventsChan = make(chan types.ContainerEventResponse, 1000) @@ -813,10 +817,7 @@ func (s *Server) handleExit(ctx context.Context, event fsnotify.Event) { } } - hooks, err := runtimehandlerhooks.GetRuntimeHandlerHooks(ctx, &s.config, sb.RuntimeHandler(), sb.Annotations()) - if err != nil { - log.Warnf(ctx, "Failed to get runtime handler %q hooks", sb.RuntimeHandler()) - } else if hooks != nil { + if hooks := s.hooksRetriever.Get(sb.RuntimeHandler(), sb.Annotations()); hooks != nil { if err := hooks.PostStop(ctx, c, sb); err != nil { log.Errorf(ctx, "Failed to run post-stop hook for container %s: %v", c.ID(), err) }