diff --git a/contrib/test/ci/vars.yml b/contrib/test/ci/vars.yml index 242e09e5286..17fcb89f9db 100644 --- a/contrib/test/ci/vars.yml +++ b/contrib/test/ci/vars.yml @@ -154,6 +154,7 @@ kata_skip_network_tests: - 'test "Connect to pod hostport from the host"' - 'test "Clean up network if pod sandbox fails"' - 'test "Clean up network if pod sandbox gets killed"' + - 'test "Network recovery after reboot with destroyed netns"' kata_skip_pod_tests: - 'test "pass pod sysctls to runtime"' - 'test "pass pod sysctls to runtime when in userns"' diff --git a/dependencies.yaml b/dependencies.yaml index 950e41e8750..25c478fea10 100644 --- a/dependencies.yaml +++ b/dependencies.yaml @@ -13,7 +13,7 @@ dependencies: # tag the .0 release if it does not already exist. If the .0 release is done, # increase the development version to the next minor (1.x.0). - name: development version - version: 1.33.0 + version: 1.33.2 refPaths: - path: internal/version/version.go match: Version diff --git a/internal/hostport/meta_hostport_manager.go b/internal/hostport/meta_hostport_manager.go index c7fdffcc033..68dba4d7fa1 100644 --- a/internal/hostport/meta_hostport_manager.go +++ b/internal/hostport/meta_hostport_manager.go @@ -28,10 +28,6 @@ type hostportManagers struct { // NewMetaHostportManager creates a new HostPortManager. func NewMetaHostportManager(ctx context.Context) (HostPortManager, error) { - mh := &metaHostportManager{ - managers: make(map[utilnet.IPFamily]*hostportManagers), - } - iptv4, iptErr := newHostportManagerIPTables(ctx, utiliptables.ProtocolIPv4) nftv4, nftErr := newHostportManagerNFTables(knftables.IPv4Family) @@ -39,15 +35,8 @@ func NewMetaHostportManager(ctx context.Context) (HostPortManager, error) { return nil, fmt.Errorf("can't create HostPortManager: no support for iptables (%w) or nftables (%w)", iptErr, nftErr) } - mh.managers[utilnet.IPv4] = &hostportManagers{ - iptables: iptv4, - nftables: nftv4, - } - - // IPv6 may fail if there's no kernel support, or no ip6tables binaries. We leave - // mh.managers[utilnet.IPv6] nil if there's no IPv6 support. + // IPv6 may fail if there's no kernel support, or no ip6tables binaries. iptv6, iptErr := newHostportManagerIPTables(ctx, utiliptables.ProtocolIPv6) - nftv6, nftErr := newHostportManagerNFTables(knftables.IPv6Family) switch { @@ -55,14 +44,45 @@ func NewMetaHostportManager(ctx context.Context) (HostPortManager, error) { logrus.Infof("No kernel support for IPv6: %v", nftErr) case iptv6 == nil: logrus.Infof("No iptables support for IPv6: %v", iptErr) - default: - mh.managers[utilnet.IPv6] = &hostportManagers{ - iptables: iptv6, - nftables: nftv6, + } + + return newMetaHostportManagerInternal(iptv4, iptv6, nftv4, nftv6), nil +} + +// internal metaHostportManager constructor; requires that at least one of the +// sub-managers is non-nil. +func newMetaHostportManagerInternal(iptv4, iptv6 *hostportManagerIPTables, nftv4, nftv6 *hostportManagerNFTables) HostPortManager { + mh := &metaHostportManager{ + managers: make(map[utilnet.IPFamily]*hostportManagers), + } + + if iptv4 != nil || nftv4 != nil { + managers := &hostportManagers{} + if iptv4 != nil { + managers.iptables = iptv4 + } + + if nftv4 != nil { + managers.nftables = nftv4 + } + + mh.managers[utilnet.IPv4] = managers + } + + if iptv6 != nil || nftv6 != nil { + managers := &hostportManagers{} + if iptv6 != nil { + managers.iptables = iptv6 } + + if nftv6 != nil { + managers.nftables = nftv6 + } + + mh.managers[utilnet.IPv6] = managers } - return mh, nil + return mh } var netlinkFamily = map[utilnet.IPFamily]netlink.InetFamily{ diff --git a/internal/hostport/meta_hostport_manager_test.go b/internal/hostport/meta_hostport_manager_test.go index 3eb59c0d319..8eb6a1a0ef5 100644 --- a/internal/hostport/meta_hostport_manager_test.go +++ b/internal/hostport/meta_hostport_manager_test.go @@ -28,20 +28,12 @@ var _ = t.Describe("MetaHostportManager", func() { ip6tables := newFakeIPTables() ip6tables.protocol = utiliptables.ProtocolIPv6 - manager := metaHostportManager{ - managers: map[utilnet.IPFamily]*hostportManagers{ - utilnet.IPv4: { - iptables: &hostportManagerIPTables{ - iptables: iptables, - }, - }, - utilnet.IPv6: { - iptables: &hostportManagerIPTables{ - iptables: ip6tables, - }, - }, - }, - } + manager := newMetaHostportManagerInternal( + &hostportManagerIPTables{iptables: iptables}, + &hostportManagerIPTables{iptables: ip6tables}, + nil, + nil, + ) // Add Hostports for _, tc := range metaTestCases { @@ -67,22 +59,13 @@ var _ = t.Describe("MetaHostportManager", func() { It("should work when only nftables is available", func() { nft4 := knftables.NewFake(knftables.IPv4Family, hostPortsTable) nft6 := knftables.NewFake(knftables.IPv6Family, hostPortsTable) - manager := metaHostportManager{ - managers: map[utilnet.IPFamily]*hostportManagers{ - utilnet.IPv4: { - nftables: &hostportManagerNFTables{ - nft: nft4, - family: knftables.IPv4Family, - }, - }, - utilnet.IPv6: { - nftables: &hostportManagerNFTables{ - nft: nft6, - family: knftables.IPv6Family, - }, - }, - }, - } + + manager := newMetaHostportManagerInternal( + nil, + nil, + &hostportManagerNFTables{nft: nft4, family: knftables.IPv4Family}, + &hostportManagerNFTables{nft: nft6, family: knftables.IPv6Family}, + ) // Add Hostports for _, tc := range metaTestCases { @@ -110,28 +93,13 @@ var _ = t.Describe("MetaHostportManager", func() { ip6tables.protocol = utiliptables.ProtocolIPv6 nft4 := knftables.NewFake(knftables.IPv4Family, hostPortsTable) nft6 := knftables.NewFake(knftables.IPv6Family, hostPortsTable) - manager := metaHostportManager{ - managers: map[utilnet.IPFamily]*hostportManagers{ - utilnet.IPv4: { - iptables: &hostportManagerIPTables{ - iptables: iptables, - }, - nftables: &hostportManagerNFTables{ - nft: nft4, - family: knftables.IPv4Family, - }, - }, - utilnet.IPv6: { - iptables: &hostportManagerIPTables{ - iptables: ip6tables, - }, - nftables: &hostportManagerNFTables{ - nft: nft6, - family: knftables.IPv6Family, - }, - }, - }, - } + + manager := newMetaHostportManagerInternal( + &hostportManagerIPTables{iptables: iptables}, + &hostportManagerIPTables{iptables: ip6tables}, + &hostportManagerNFTables{nft: nft4, family: knftables.IPv4Family}, + &hostportManagerNFTables{nft: nft6, family: knftables.IPv6Family}, + ) // Add Hostports for _, tc := range metaTestCases { @@ -203,20 +171,13 @@ var _ = t.Describe("MetaHostportManager", func() { iptables.protocol = utiliptables.ProtocolIPv4 ip6tables := newFakeIPTables() ip6tables.protocol = utiliptables.ProtocolIPv6 - manager := metaHostportManager{ - managers: map[utilnet.IPFamily]*hostportManagers{ - utilnet.IPv4: { - iptables: &hostportManagerIPTables{ - iptables: iptables, - }, - }, - utilnet.IPv6: { - iptables: &hostportManagerIPTables{ - iptables: ip6tables, - }, - }, - }, - } + + manager := newMetaHostportManagerInternal( + &hostportManagerIPTables{iptables: iptables}, + &hostportManagerIPTables{iptables: ip6tables}, + nil, + nil, + ) // Add the legacy mappings. for _, tc := range legacyIPTablesTestCases { @@ -231,28 +192,13 @@ var _ = t.Describe("MetaHostportManager", func() { // existing fakeIPTables state, but now with nftables support as well. nft4 := knftables.NewFake(knftables.IPv4Family, hostPortsTable) nft6 := knftables.NewFake(knftables.IPv6Family, hostPortsTable) - manager = metaHostportManager{ - managers: map[utilnet.IPFamily]*hostportManagers{ - utilnet.IPv4: { - iptables: &hostportManagerIPTables{ - iptables: iptables, - }, - nftables: &hostportManagerNFTables{ - nft: nft4, - family: knftables.IPv4Family, - }, - }, - utilnet.IPv6: { - iptables: &hostportManagerIPTables{ - iptables: ip6tables, - }, - nftables: &hostportManagerNFTables{ - nft: nft6, - family: knftables.IPv6Family, - }, - }, - }, - } + + manager = newMetaHostportManagerInternal( + &hostportManagerIPTables{iptables: iptables}, + &hostportManagerIPTables{iptables: ip6tables}, + &hostportManagerNFTables{nft: nft4, family: knftables.IPv4Family}, + &hostportManagerNFTables{nft: nft6, family: knftables.IPv6Family}, + ) // Add the remaining hostports. for _, tc := range metaTestCases { @@ -289,19 +235,13 @@ var _ = t.Describe("MetaHostportManager", func() { iptables := newFakeIPTables() iptables.protocol = utiliptables.ProtocolIPv4 nft4 := knftables.NewFake(knftables.IPv4Family, hostPortsTable) - manager := metaHostportManager{ - managers: map[utilnet.IPFamily]*hostportManagers{ - utilnet.IPv4: { - iptables: &hostportManagerIPTables{ - iptables: iptables, - }, - nftables: &hostportManagerNFTables{ - nft: nft4, - family: knftables.IPv4Family, - }, - }, - }, - } + + manager := newMetaHostportManagerInternal( + &hostportManagerIPTables{iptables: iptables}, + nil, + &hostportManagerNFTables{nft: nft4, family: knftables.IPv4Family}, + nil, + ) // Add Hostports for _, tc := range metaTestCases { diff --git a/internal/iptables/iptables.go b/internal/iptables/iptables.go index 4c6fd8582cc..33665888e9e 100644 --- a/internal/iptables/iptables.go +++ b/internal/iptables/iptables.go @@ -220,12 +220,6 @@ type runner struct { // newInternal returns a new Interface which will exec iptables, and allows the // caller to change the iptables-restore lockfile path. func newInternal(ctx context.Context, exec utilexec.Interface, protocol Protocol, lockfilePath14x, lockfilePath16x string) Interface { - version, err := getIPTablesVersion(exec, protocol) - if err != nil { - log.Warnf(ctx, "Error checking iptables version, assuming version at least: %s (version=%q)", err, MinCheckVersion) - version = MinCheckVersion - } - if lockfilePath16x == "" { lockfilePath16x = LockfilePath16x } @@ -237,14 +231,25 @@ func newInternal(ctx context.Context, exec utilexec.Interface, protocol Protocol runner := &runner{ exec: exec, protocol: protocol, - hasCheck: version.AtLeast(MinCheckVersion), - hasRandomFully: version.AtLeast(RandomFullyMinVersion), - waitFlag: getIPTablesWaitFlag(version), - restoreWaitFlag: getIPTablesRestoreWaitFlag(version, exec, protocol), lockfilePath14x: lockfilePath14x, lockfilePath16x: lockfilePath16x, } + version, err := getIPTablesVersion(exec, protocol) + if err != nil { + // The only likely error is "no such file or directory", in which case any + // further commands will fail the same way, so we don't need to do + // anything special here. + log.Debugf(ctx, "Error checking iptables version: %v", err) + + return runner + } + + runner.hasCheck = version.AtLeast(MinCheckVersion) + runner.hasRandomFully = version.AtLeast(RandomFullyMinVersion) + runner.waitFlag = getIPTablesWaitFlag(version) + runner.restoreWaitFlag = getIPTablesRestoreWaitFlag(version, exec, protocol) + return runner } diff --git a/internal/lib/container_server.go b/internal/lib/container_server.go index 77ca6725d85..50e6959e38f 100644 --- a/internal/lib/container_server.go +++ b/internal/lib/container_server.go @@ -397,6 +397,11 @@ func (c *ContainerServer) LoadSandbox(ctx context.Context, id string) (sb *sandb } sb.SetCreated() + + if scontainer.State().Status == oci.ContainerStateStopped { + sb.SetStopped(ctx, true) + } + selinux.ReserveLabel(processLabel) if err := c.ctrIDIndex.Add(scontainer.ID()); err != nil { diff --git a/internal/lib/sandbox/sandbox.go b/internal/lib/sandbox/sandbox.go index 7f8756d5250..7926a2a26b5 100644 --- a/internal/lib/sandbox/sandbox.go +++ b/internal/lib/sandbox/sandbox.go @@ -59,15 +59,18 @@ type Sandbox struct { nsOpts *types.NamespaceOption dnsConfig *types.DNSConfig stopMutex sync.RWMutex - created bool - stopped bool - networkStopped bool - privileged bool - hostNetwork bool - usernsMode string - containerEnvPath string - podLinuxOverhead *types.LinuxContainerResources - podLinuxResources *types.LinuxContainerResources + // stateMutex protects the use of created, stopped and networkStopped bools + // which are all fields that can change at runtime + stateMutex sync.RWMutex + created bool + stopped bool + networkStopped bool + privileged bool + hostNetwork bool + usernsMode string + containerEnvPath string + podLinuxOverhead *types.LinuxContainerResources + podLinuxResources *types.LinuxContainerResources } // DefaultShmSize is the default shm size. @@ -329,6 +332,9 @@ func (s *Sandbox) SetStopped(ctx context.Context, createFile bool) { ctx, span := log.StartSpan(ctx) defer span.End() + s.stateMutex.Lock() + defer s.stateMutex.Unlock() + if s.stopped { return } @@ -344,16 +350,24 @@ func (s *Sandbox) SetStopped(ctx context.Context, createFile bool) { // Stopped returns whether the sandbox state has been // set to stopped. func (s *Sandbox) Stopped() bool { + s.stateMutex.RLock() + defer s.stateMutex.RUnlock() + return s.stopped } // SetCreated sets the created status of sandbox to true. func (s *Sandbox) SetCreated() { + s.stateMutex.Lock() + defer s.stateMutex.Unlock() s.created = true } // NetworkStopped returns whether the network has been stopped. func (s *Sandbox) NetworkStopped() bool { + s.stateMutex.RLock() + defer s.stateMutex.RUnlock() + return s.networkStopped } @@ -368,6 +382,9 @@ func (s *Sandbox) SetNetworkStopped(ctx context.Context, createFile bool) error ctx, span := log.StartSpan(ctx) defer span.End() + s.stateMutex.Lock() + defer s.stateMutex.Unlock() + if s.networkStopped { return nil } @@ -404,6 +421,7 @@ func (s *Sandbox) SetContainerEnvFile(ctx context.Context) error { return nil } +// This function assumes the state lock has been taken for this sandbox. func (s *Sandbox) createFileInInfraDir(ctx context.Context, filename string) error { // If the sandbox is not yet created, // this function is being called when @@ -433,6 +451,9 @@ func (s *Sandbox) createFileInInfraDir(ctx context.Context, filename string) err } func (s *Sandbox) RestoreStopped() { + s.stateMutex.Lock() + defer s.stateMutex.Unlock() + if s.fileExistsInInfraDir(sbStoppedFilename) { s.stopped = true } @@ -459,11 +480,14 @@ func (s *Sandbox) fileExistsInInfraDir(filename string) bool { // Created returns the created status of sandbox. func (s *Sandbox) Created() bool { + s.stateMutex.RLock() + defer s.stateMutex.RUnlock() + return s.created } func (s *Sandbox) State() types.PodSandboxState { - if s.Ready(false) { + if s.Ready() { return types.PodSandboxState_SANDBOX_READY } @@ -475,23 +499,9 @@ func (s *Sandbox) State() types.PodSandboxState { // `takeLock` should be set if we need to take the lock to get the infra container's state. // If there is no infra container, it is never considered ready. // If the infra container is spoofed, the pod is considered ready when it has been created, but not stopped. -func (s *Sandbox) Ready(takeLock bool) bool { - podInfraContainer := s.InfraContainer() - if podInfraContainer == nil { - return false - } - - if podInfraContainer.Spoofed() { - return s.created && !s.stopped - } - // Assume the sandbox is ready, unless it has an infra container that - // isn't running - var cState *oci.ContainerState - if takeLock { - cState = podInfraContainer.State() - } else { - cState = podInfraContainer.StateNoLock() - } +func (s *Sandbox) Ready() bool { + s.stateMutex.RLock() + defer s.stateMutex.RUnlock() - return cState.Status == oci.ContainerStateRunning + return s.created && !s.stopped } diff --git a/internal/lib/sandbox/sandbox_test.go b/internal/lib/sandbox/sandbox_test.go index d1d4422803c..449296077d8 100644 --- a/internal/lib/sandbox/sandbox_test.go +++ b/internal/lib/sandbox/sandbox_test.go @@ -139,6 +139,7 @@ var _ = t.Describe("Sandbox", func() { // Then Expect(testSandbox.Stopped()).To(BeTrue()) + Expect(testSandbox.Ready()).To(BeFalse()) }) }) @@ -184,6 +185,7 @@ var _ = t.Describe("Sandbox", func() { // Then Expect(testSandbox.Created()).To(BeTrue()) + Expect(testSandbox.Ready()).To(BeTrue()) }) }) diff --git a/internal/oci/container.go b/internal/oci/container.go index 6ba460f4543..e90a0096082 100644 --- a/internal/oci/container.go +++ b/internal/oci/container.go @@ -50,25 +50,29 @@ type Container struct { dir string stopSignal string // If set, _some_ name of the image imageID; it may have NO RELATIONSHIP to the users’ requested image name. - someNameOfTheImage *references.RegistryImageReference - imageID *storage.StorageImageID // nil for infra containers. - mountPoint string - seccompProfilePath string - conmonCgroupfsPath string - crioAnnotations fields.Set - state *ContainerState - opLock sync.RWMutex - spec *specs.Spec - idMappings *idtools.IDMappings - terminal bool - stdin bool - stdinOnce bool - created bool - spoofed bool - stopping bool - stopLock sync.Mutex + someNameOfTheImage *references.RegistryImageReference + imageID *storage.StorageImageID // nil for infra containers. + mountPoint string + seccompProfilePath string + conmonCgroupfsPath string + crioAnnotations fields.Set + state *ContainerState + opLock sync.RWMutex + spec *specs.Spec + idMappings *idtools.IDMappings + terminal bool + stdin bool + stdinOnce bool + created bool + spoofed bool + stopping bool + stopLock sync.Mutex + // stopTimeoutChan is used to update the stop timeout. + // After the container goes into the kill loop, the channel must not be used + // because it is not controlled by the timeout anymore. stopTimeoutChan chan int64 stopWatchers []chan struct{} + stopKillLoopBegun bool pidns nsmgr.Namespace restore bool restoreArchivePath string @@ -170,6 +174,7 @@ func NewContainer(id, name, bundlePath, logPath string, labels, crioAnnotations, stopSignal: stopSignal, stopTimeoutChan: make(chan int64, 10), stopWatchers: []chan struct{}{}, + stopKillLoopBegun: false, execPIDs: map[int]bool{}, } @@ -642,6 +647,13 @@ func (c *Container) SetAsStopping() (setToStopping bool) { return false } +// SetStopKillLoopBegun sets the stopKillLoopBegun flag to true. +func (c *Container) SetStopKillLoopBegun() { + c.stopLock.Lock() + defer c.stopLock.Unlock() + c.stopKillLoopBegun = true +} + func (c *Container) WaitOnStopTimeout(ctx context.Context, timeout int64) { c.stopLock.Lock() if !c.stopping { @@ -650,7 +662,16 @@ func (c *Container) WaitOnStopTimeout(ctx context.Context, timeout int64) { return } - c.stopTimeoutChan <- timeout + // Don't use the stopTimeoutChan when the container is in kill loop + // because values in the channel are no longer consumed. + if !c.stopKillLoopBegun { + // Use select and default not to block when the stopTimeoutChan is full. + // The channel is very unlikely to be full, but it could happen in theory. + select { + case c.stopTimeoutChan <- timeout: + default: + } + } watcher := make(chan struct{}, 1) c.stopWatchers = append(c.stopWatchers, watcher) diff --git a/internal/oci/runtime_oci.go b/internal/oci/runtime_oci.go index 9b6ab42d4b2..b38d1bfb1b1 100644 --- a/internal/oci/runtime_oci.go +++ b/internal/oci/runtime_oci.go @@ -1002,6 +1002,7 @@ func (r *runtimeOCI) StopLoopForContainer(c *Container, bm kwait.BackoffManager) case <-time.After(time.Until(targetTime)): log.Warnf(ctx, "Stopping container %s with stop signal(%s) timed out. Killing...", c.ID(), c.GetStopSignal()) + c.SetStopKillLoopBegun() goto killContainer @@ -1025,10 +1026,12 @@ killContainer: } if err := c.Living(); err != nil { + log.Debugf(ctx, "Container is no longer alive") stop() return } + log.Debugf(ctx, "Killing failed for some reasons, retrying...") // Reschedule the timer so that the periodic reminder can continue. blockedTimer.Reset(stopProcessBlockedInterval) }, bm, true, ctx.Done()) diff --git a/internal/storage/image_test.go b/internal/storage/image_test.go index 65edd52a4fd..b71d141b6ed 100644 --- a/internal/storage/image_test.go +++ b/internal/storage/image_test.go @@ -29,7 +29,6 @@ var _ = t.Describe("Image", func() { testDockerRegistry = "docker.io" testQuayRegistry = "quay.io" testRedHatRegistry = "registry.access.redhat.com" - testFedoraRegistry = "registry.fedoraproject.org" testImageName = "image" testImageAlias = "image-for-testing" testImageAliasResolved = "registry.crio.test.com/repo" @@ -193,7 +192,6 @@ var _ = t.Describe("Image", func() { Expect(refsToNames(refs)).To(Equal([]string{ testQuayRegistry + "/" + testImageName + ":latest", testRedHatRegistry + "/" + testImageName + ":latest", - testFedoraRegistry + "/" + testImageName + ":latest", testDockerRegistry + "/library/" + testImageName + ":latest", })) }) @@ -247,7 +245,6 @@ var _ = t.Describe("Image", func() { Expect(refsToNames(refs)).To(Equal([]string{ testQuayRegistry + "/" + testImageName + "@sha256:" + testSHA256, testRedHatRegistry + "/" + testImageName + "@sha256:" + testSHA256, - testFedoraRegistry + "/" + testImageName + "@sha256:" + testSHA256, testDockerRegistry + "/library/" + testImageName + "@sha256:" + testSHA256, })) }) diff --git a/internal/version/version.go b/internal/version/version.go index 808db5b5e2a..20cc6751494 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -21,7 +21,7 @@ import ( ) // Version is the version of the build. -const Version = "1.33.0" +const Version = "1.33.2" // ReleaseMinorVersions are the currently supported minor versions. var ReleaseMinorVersions = []string{"1.33", "1.32", "1.31", "1.30"} diff --git a/server/container_create.go b/server/container_create.go index d5e463f0cfe..e62ec1012b7 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -885,8 +885,10 @@ func (s *Server) createSandboxContainer(ctx context.Context, ctr container.Conta specgen.SetLinuxCgroupsPath(s.config.CgroupManager().ContainerCgroupPath(sb.CgroupParent(), containerID)) - securityContext.MaskedPaths = appendDefaultMaskedPaths(securityContext.MaskedPaths) - log.Debugf(ctx, "Using masked paths: %v", strings.Join(securityContext.MaskedPaths, ", ")) + if len(securityContext.MaskedPaths) != 0 { + securityContext.MaskedPaths = appendDefaultMaskedPaths(securityContext.MaskedPaths) + log.Debugf(ctx, "Using masked paths: %v", strings.Join(securityContext.MaskedPaths, ", ")) + } err = ctr.SpecSetPrivileges(ctx, securityContext, &s.config) if err != nil { diff --git a/server/container_portforward.go b/server/container_portforward.go index 7c59c350232..6d97d17e1e3 100644 --- a/server/container_portforward.go +++ b/server/container_portforward.go @@ -51,7 +51,7 @@ func (s *StreamService) PortForward(ctx context.Context, podSandboxID string, po return fmt.Errorf("could not find sandbox %s", podSandboxID) } - if !sb.Ready(true) { + if !sb.Ready() { return fmt.Errorf("sandbox %s is not running", podSandboxID) } diff --git a/server/sandbox_list_test.go b/server/sandbox_list_test.go index c17a89fd250..1f1a1e35afa 100644 --- a/server/sandbox_list_test.go +++ b/server/sandbox_list_test.go @@ -96,8 +96,9 @@ var _ = t.Describe("ListPodSandbox", func() { // Given mockDirs(testManifest) createDummyState() - _, err := sut.LoadSandbox(context.Background(), sandboxID) + sb, err := sut.LoadSandbox(context.Background(), sandboxID) Expect(err).ToNot(HaveOccurred()) + sb.SetStopped(context.Background(), false) // When response, err := sut.ListPodSandbox(context.Background(), diff --git a/server/sandbox_network.go b/server/sandbox_network.go index dd4e431f343..4c86f501c94 100644 --- a/server/sandbox_network.go +++ b/server/sandbox_network.go @@ -181,23 +181,42 @@ func (s *Server) networkStop(ctx context.Context, sb *sandbox.Sandbox) error { podNetwork, err := s.newPodNetwork(ctx, sb) if err != nil { - return err + return fmt.Errorf("failed to create pod network for sandbox %s(%s): %w", sb.Name(), sb.ID(), err) } - if err := s.config.CNIPlugin().TearDownPodWithContext(stopCtx, podNetwork); err != nil { - retErr := fmt.Errorf("failed to destroy network for pod sandbox %s(%s): %w", sb.Name(), sb.ID(), err) - + // Check if the network namespace file exists and is valid before attempting CNI teardown. + // If the file doesn't exist or is invalid, skip CNI teardown and mark network as stopped. + if podNetwork.NetNS != "" { if _, statErr := os.Stat(podNetwork.NetNS); statErr != nil { - return fmt.Errorf("%w: stat netns path %q: %w", retErr, podNetwork.NetNS, statErr) + // Network namespace file doesn't exist, mark network as stopped and return success + log.Debugf(ctx, "Network namespace file %s does not exist for pod sandbox %s(%s), skipping CNI teardown", + podNetwork.NetNS, sb.Name(), sb.ID()) + + return sb.SetNetworkStopped(ctx, true) + } + + if validateErr := s.validateNetworkNamespace(podNetwork.NetNS); validateErr != nil { + // Network namespace file exists but is invalid (e.g., corrupted or fake file) + log.Warnf(ctx, "Network namespace file %s is invalid for pod sandbox %s(%s): %v, removing and skipping CNI teardown", + podNetwork.NetNS, sb.Name(), sb.ID(), validateErr) + s.cleanupNetns(ctx, podNetwork.NetNS, sb) + + return sb.SetNetworkStopped(ctx, true) } + } - // The netns file may still exists, which means that it's likely - // corrupted. Remove it to allow cleanup of the network namespace: - if rmErr := os.RemoveAll(podNetwork.NetNS); rmErr != nil { - return fmt.Errorf("%w: failed to remove netns path: %w", retErr, rmErr) + if err := s.config.CNIPlugin().TearDownPodWithContext(stopCtx, podNetwork); err != nil { + log.Warnf(ctx, "Failed to destroy network for pod sandbox %s(%s): %v", sb.Name(), sb.ID(), err) + + // If the network namespace exists but CNI teardown failed, try to clean it up. + if podNetwork.NetNS != "" { + if _, statErr := os.Stat(podNetwork.NetNS); statErr == nil { + // Clean up the netns file since CNI teardown failed. + s.cleanupNetns(ctx, podNetwork.NetNS, sb) + } } - log.Warnf(ctx, "Removed invalid netns path %s from pod sandbox %s(%s)", podNetwork.NetNS, sb.Name(), sb.ID()) + return fmt.Errorf("network teardown failed for pod sandbox %s(%s): %w", sb.Name(), sb.ID(), err) } return sb.SetNetworkStopped(ctx, true) diff --git a/server/sandbox_network_freebsd.go b/server/sandbox_network_freebsd.go new file mode 100644 index 00000000000..511ee579dcb --- /dev/null +++ b/server/sandbox_network_freebsd.go @@ -0,0 +1,24 @@ +//go:build freebsd +// +build freebsd + +package server + +import ( + "context" + + "github.com/cri-o/cri-o/internal/lib/sandbox" + "github.com/cri-o/cri-o/internal/log" +) + +// validateNetworkNamespace checks if the given path is a valid network namespace +// On FreeBSD, this is a no-op since network namespaces are Linux-specific. +func (s *Server) validateNetworkNamespace(netnsPath string) error { + // Network namespaces are Linux-specific, so on FreeBSD we assume it's valid + return nil +} + +// cleanupNetns removes a network namespace file and logs the action +// On FreeBSD, this is a no-op since network namespaces are Linux-specific. +func (s *Server) cleanupNetns(ctx context.Context, netnsPath string, sb *sandbox.Sandbox) { + log.Debugf(ctx, "Network namespace cleanup not supported on this platform") +} diff --git a/server/sandbox_network_linux.go b/server/sandbox_network_linux.go new file mode 100644 index 00000000000..1d4ae931cd0 --- /dev/null +++ b/server/sandbox_network_linux.go @@ -0,0 +1,36 @@ +//go:build linux +// +build linux + +package server + +import ( + "context" + "fmt" + "os" + + "github.com/containernetworking/plugins/pkg/ns" + + "github.com/cri-o/cri-o/internal/lib/sandbox" + "github.com/cri-o/cri-o/internal/log" +) + +// validateNetworkNamespace checks if the given path is a valid network namespace. +func (s *Server) validateNetworkNamespace(netnsPath string) error { + netns, err := ns.GetNS(netnsPath) + if err != nil { + return fmt.Errorf("invalid network namespace: %w", err) + } + + defer netns.Close() + + return nil +} + +// cleanupNetns removes a network namespace file and logs the action. +func (s *Server) cleanupNetns(ctx context.Context, netnsPath string, sb *sandbox.Sandbox) { + if rmErr := os.RemoveAll(netnsPath); rmErr != nil { + log.Warnf(ctx, "Failed to remove netns path %s: %v", netnsPath, rmErr) + } else { + log.Infof(ctx, "Removed netns path %s from pod sandbox %s(%s)", netnsPath, sb.Name(), sb.ID()) + } +} diff --git a/server/sandbox_network_unsupported.go b/server/sandbox_network_unsupported.go new file mode 100644 index 00000000000..d9067ae46b6 --- /dev/null +++ b/server/sandbox_network_unsupported.go @@ -0,0 +1,23 @@ +//go:build !linux && !freebsd +// +build !linux,!freebsd + +package server + +import ( + "context" + + "github.com/cri-o/cri-o/internal/lib/sandbox" + "github.com/cri-o/cri-o/internal/log" +) + +// validateNetworkNamespace checks if the given path is a valid network namespace +// On unsupported platforms, this is a no-op since network namespaces are Linux-specific. +func (s *Server) validateNetworkNamespace(netnsPath string) error { + return nil +} + +// cleanupNetns removes a network namespace file and logs the action +// On unsupported platforms, this is a no-op since network namespaces are Linux-specific. +func (s *Server) cleanupNetns(ctx context.Context, netnsPath string, sb *sandbox.Sandbox) { + log.Debugf(ctx, "Network namespace cleanup not supported on this platform") +} diff --git a/server/sandbox_status.go b/server/sandbox_status.go index aa0bec8bb0c..2ee1289ad25 100644 --- a/server/sandbox_status.go +++ b/server/sandbox_status.go @@ -25,10 +25,7 @@ func (s *Server) PodSandboxStatus(ctx context.Context, req *types.PodSandboxStat return nil, status.Errorf(codes.NotFound, "could not find pod %q: %v", req.PodSandboxId, err) } - rStatus := types.PodSandboxState_SANDBOX_NOTREADY - if sb.Ready(true) { - rStatus = types.PodSandboxState_SANDBOX_READY - } + rStatus := sb.State() var linux *types.LinuxPodSandboxStatus if sb.NamespaceOptions() != nil { diff --git a/server/server.go b/server/server.go index 246dccc9dba..e3771aad671 100644 --- a/server/server.go +++ b/server/server.go @@ -862,6 +862,12 @@ func (s *Server) handleExit(ctx context.Context, event fsnotify.Event) { c = sb.InfraContainer() resource = "sandbox infra" + // We discovered the infra container stopped (potentially unexpectedly). + // Since sandboxes status is now being judged by the sb.stopped boolean, + // rather than the infra container's status, we have to manually set stopped here. + // It's likely we're doing double the work here, but that's better than missing it + // if the infra container crashed. + sb.SetStopped(ctx, true) } else { sb = s.GetSandbox(c.Sandbox()) } diff --git a/test/ctr.bats b/test/ctr.bats index 0375cc9cda5..4e9ddebdc65 100644 --- a/test/ctr.bats +++ b/test/ctr.bats @@ -1558,7 +1558,20 @@ EOF start_crio ctr_id=$(crictl run "$TESTDATA"/container_redis.json "$TESTDATA"/sandbox_config.json) - # verify that at least a default masked path exists + # verify that no default masked path exist + INSPECT=$(crictl inspect "$ctr_id") + run ! jq "$INSPECT" -e '.info.runtimeSpec.linux.maskedPaths | index("/proc/acpi")' +} + +@test "ctr masked defaults set if any are set" { + start_crio + # Start a container that traps SIGTERM and writes to a file when received + jq '.linux.security_context.masked_paths = ["/proc/asound"]' \ + "$TESTDATA"/container_redis.json > "$TESTDIR/container_config.json" + + ctr_id=$(crictl run "$TESTDIR"/container_config.json "$TESTDATA"/sandbox_config.json) + + # verify that if client passes any masked paths, we append the defaults crictl inspect "$ctr_id" | jq -e '.info.runtimeSpec.linux.maskedPaths | index("/proc/acpi")' } diff --git a/test/image.bats b/test/image.bats index 10ead5e7df5..0c5c5b3e161 100644 --- a/test/image.bats +++ b/test/image.bats @@ -104,7 +104,9 @@ function teardown() { mkdir -p "$TESTDIR/imagestore" CONTAINER_IMAGESTORE="$TESTDIR/imagestore" start_crio - FEDORA="registry.fedoraproject.org/fedora" + # registry.fedoraproject.org is pretty flaky + # Moving to the stable quay.io + FEDORA="quay.io/fedora/fedora" crictl pull $FEDORA imageid=$(crictl images --quiet "$FEDORA") [ "$imageid" != "" ] diff --git a/test/network.bats b/test/network.bats index 2c89725c66a..09a63e1f818 100644 --- a/test/network.bats +++ b/test/network.bats @@ -185,5 +185,42 @@ function check_networking() { # be able to remove the sandbox crictl rmp -f "$POD" - grep -q "Removed invalid netns path $NETNS_PATH$NS from pod sandbox" "$CRIO_LOG" + grep -q "Removed netns path $NETNS_PATH$NS from pod sandbox" "$CRIO_LOG" +} + +@test "Network recovery after reboot with destroyed netns" { + # This test simulates a reboot scenario where network namespaces are destroyed + # but CRI-O needs to clean up pod network resources gracefully. + + start_crio + + pod_id=$(crictl runp "$TESTDATA"/sandbox_config.json) + + # Get the network namespace path + NETNS_PATH=/var/run/netns/ + NS=$(crictl inspectp "$pod_id" | + jq -er '.info.runtimeSpec.linux.namespaces[] | select(.type == "network").path | sub("'$NETNS_PATH'"; "")') + + # Remove the network namespace. + ip netns del "$NS" + + # Create a fake netns file. + touch "$NETNS_PATH$NS" + + restart_crio + + # Try to remove the pod. + crictl rmp -f "$pod_id" 2> /dev/null || true + + grep -q "Successfully cleaned up network for pod" "$CRIO_LOG" + + new_pod_id=$(crictl runp "$TESTDATA"/sandbox_config.json) + + # Verify the new pod is running. + output=$(crictl inspectp "$new_pod_id" | jq -r '.status.state') + [[ "$output" == "SANDBOX_READY" ]] + + # Clean up the new pod + crictl stopp "$new_pod_id" + crictl rmp "$new_pod_id" } diff --git a/test/registries.conf b/test/registries.conf index 0fb3be9f31e..c0fc9f53b5e 100644 --- a/test/registries.conf +++ b/test/registries.conf @@ -1,4 +1,4 @@ -unqualified-search-registries = ['quay.io' ,'registry.access.redhat.com', 'registry.fedoraproject.org', 'docker.io'] +unqualified-search-registries = ['quay.io', 'registry.access.redhat.com', 'docker.io'] [aliases] "image-for-testing" = "registry.crio.test.com/repo" diff --git a/test/testdata/Dockerfile b/test/testdata/Dockerfile index 59c6ce56c69..8ee16e75d92 100644 --- a/test/testdata/Dockerfile +++ b/test/testdata/Dockerfile @@ -1,4 +1,4 @@ -FROM registry.fedoraproject.org/fedora-minimal:38 +FROM quay.io/fedora/fedora-minimal:38 RUN microdnf install -y coreutils \ gcc \ gzip \