From 7ca5aa7e1abf054223290930c8b633e3a2d049dc Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 29 Apr 2025 11:45:45 -0400 Subject: [PATCH 1/4] sandbox: use created/stopped instead of infra container for readiness As we hit cases of deadlock when a sandbox is stuck stopping Signed-off-by: Peter Hunt --- internal/lib/container_server.go | 5 +++ internal/lib/sandbox/sandbox.go | 66 ++++++++++++++++------------ internal/lib/sandbox/sandbox_test.go | 2 + server/container_portforward.go | 2 +- server/sandbox_list_test.go | 3 +- server/sandbox_status.go | 5 +-- server/server.go | 6 +++ 7 files changed, 55 insertions(+), 34 deletions(-) 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/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_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()) } From ce6f724d73ed492e114d7df35b71610ff6113916 Mon Sep 17 00:00:00 2001 From: Kubernetes Release Robot Date: Sun, 1 Jun 2025 00:38:36 +0000 Subject: [PATCH 2/4] version: bump to 1.33.1 Signed-off-by: Kubernetes Release Robot --- internal/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/version/version.go b/internal/version/version.go index 808db5b5e2a..1ea7710c442 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.1" // ReleaseMinorVersions are the currently supported minor versions. var ReleaseMinorVersions = []string{"1.33", "1.32", "1.31", "1.30"} From 219b88a1744258acd2485dc77be0f51ec1c72737 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Mon, 26 May 2025 11:35:32 -0400 Subject: [PATCH 3/4] Improve iptables error handling when there's no iptables binary If `iptables --version` failed, iptables.New() would log a warning and assume that the problem was that you had an implausibly ancient version of iptables installed. Change it to instead assume that the problem is that you don't have iptables installed at all (and only log at debug). Signed-off-by: Dan Winship --- internal/iptables/iptables.go | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) 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 } From 4759426b1a3c22a29d7199bf95257c5841455f73 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 27 May 2025 07:23:40 -0400 Subject: [PATCH 4/4] Redo metaHostportManager construction, fix bug Add an internal constructor so that the unit tests share the same logic as the real constructor, and fix it to handle nil sub-managers correctly. Signed-off-by: Dan Winship --- internal/hostport/meta_hostport_manager.go | 54 ++++--- .../hostport/meta_hostport_manager_test.go | 142 +++++------------- 2 files changed, 78 insertions(+), 118 deletions(-) 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 {