fix removing pods from podTopologyHints mapping

pkg/kubelet/cm/internal_container_lifecycle.go

@@ -45,10 +45,7 @@ func (i *internalContainerLifecycleImpl) PreStartContainer(pod *v1.Pod, containe
 		}
 	}
 	if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.TopologyManager) {
-		err := i.topologyManager.AddContainer(pod, containerID)
-		if err != nil {
-			return err
-		}
+		i.topologyManager.AddContainer(pod, container, containerID)
 	}
 	return nil
 }

pkg/kubelet/cm/topologymanager/BUILD

@@ -17,6 +17,7 @@ go_library(
     importpath = "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/kubelet/cm/containermap:go_default_library",
         "//pkg/kubelet/cm/topologymanager/bitmask:go_default_library",
         "//pkg/kubelet/lifecycle:go_default_library",
         "//pkg/kubelet/util/format:go_default_library",
@@ -59,6 +60,7 @@ go_test(
     ],
     embed = [":go_default_library"],
     deps = [
+        "//pkg/kubelet/cm/containermap:go_default_library",
         "//pkg/kubelet/cm/topologymanager/bitmask:go_default_library",
         "//pkg/kubelet/lifecycle:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",

pkg/kubelet/cm/topologymanager/fake_topology_manager.go

@@ -40,9 +40,8 @@ func (m *fakeManager) AddHintProvider(h HintProvider) {
 	klog.Infof("[fake topologymanager] AddHintProvider HintProvider:  %v", h)
 }
 
-func (m *fakeManager) AddContainer(pod *v1.Pod, containerID string) error {
-	klog.Infof("[fake topologymanager] AddContainer  pod: %v container id:  %v", format.Pod(pod), containerID)
-	return nil
+func (m *fakeManager) AddContainer(pod *v1.Pod, container *v1.Container, containerID string) {
+	klog.Infof("[fake topologymanager] AddContainer  pod: %v container name: %v container id: %v", format.Pod(pod), container.Name, containerID)
 }
 
 func (m *fakeManager) RemoveContainer(containerID string) error {

pkg/kubelet/cm/topologymanager/fake_topology_manager_test.go

@@ -21,7 +21,6 @@ import (
 	"testing"
 
 	"k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
 )
 
@@ -57,36 +56,6 @@ func TestFakeGetAffinity(t *testing.T) {
 	}
 }
 
-func TestFakeAddContainer(t *testing.T) {
-	testCases := []struct {
-		name        string
-		containerID string
-		podUID      types.UID
-	}{
-		{
-			name:        "Case1",
-			containerID: "nginx",
-			podUID:      "0aafa4c4-38e8-11e9-bcb1-a4bf01040474",
-		},
-		{
-			name:        "Case2",
-			containerID: "Busy_Box",
-			podUID:      "b3ee37fc-39a5-11e9-bcb1-a4bf01040474",
-		},
-	}
-	fm := fakeManager{}
-	for _, tc := range testCases {
-		pod := v1.Pod{}
-		pod.UID = tc.podUID
-		err := fm.AddContainer(&pod, tc.containerID)
-		if err != nil {
-			t.Errorf("Expected error to be nil but got: %v", err)
-
-		}
-
-	}
-}
-
 func TestFakeRemoveContainer(t *testing.T) {
 	testCases := []struct {
 		name        string

pkg/kubelet/cm/topologymanager/scope.go

@@ -23,6 +23,7 @@ import (
 
 	"k8s.io/api/core/v1"
 	"k8s.io/klog/v2"
+	"k8s.io/kubernetes/pkg/kubelet/cm/containermap"
 	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
 )
 
@@ -43,7 +44,7 @@ type Scope interface {
 	// wants to be consoluted with when making topology hints
 	AddHintProvider(h HintProvider)
 	// AddContainer adds pod to Manager for tracking
-	AddContainer(pod *v1.Pod, containerID string) error
+	AddContainer(pod *v1.Pod, container *v1.Container, containerID string)
 	// RemoveContainer removes pod from Manager tracking
 	RemoveContainer(containerID string) error
 	// Store is the interface for storing pod topology hints
@@ -60,8 +61,8 @@ type scope struct {
 	hintProviders []HintProvider
 	// Topology Manager Policy
 	policy Policy
-	// Mapping of PodUID to ContainerID for Adding/Removing Pods from PodTopologyHints mapping
-	podMap map[string]string
+	// Mapping of (PodUid, ContainerName) to ContainerID for Adding/Removing Pods from PodTopologyHints mapping
+	podMap containermap.ContainerMap
 }
 
 func (s *scope) Name() string {
@@ -94,12 +95,11 @@ func (s *scope) AddHintProvider(h HintProvider) {
 
 // It would be better to implement this function in topologymanager instead of scope
 // but topologymanager do not track mapping anymore
-func (s *scope) AddContainer(pod *v1.Pod, containerID string) error {
+func (s *scope) AddContainer(pod *v1.Pod, container *v1.Container, containerID string) {
 	s.mutex.Lock()
 	defer s.mutex.Unlock()
 
-	s.podMap[containerID] = string(pod.UID)
-	return nil
+	s.podMap.Add(string(pod.UID), container.Name, containerID)
 }
 
 // It would be better to implement this function in topologymanager instead of scope
@@ -109,10 +109,18 @@ func (s *scope) RemoveContainer(containerID string) error {
 	defer s.mutex.Unlock()
 
 	klog.Infof("[topologymanager] RemoveContainer - Container ID: %v", containerID)
-	podUIDString := s.podMap[containerID]
-	delete(s.podMap, containerID)
-	if _, exists := s.podTopologyHints[podUIDString]; exists {
-		delete(s.podTopologyHints[podUIDString], containerID)
+	// Get the podUID and containerName associated with the containerID to be removed and remove it
+	podUIDString, containerName, err := s.podMap.GetContainerRef(containerID)
+	if err != nil {
+		return nil
+	}
+	s.podMap.RemoveByContainerID(containerID)
+
+	// In cases where a container has been restarted, it's possible that the same podUID and
+	// containerName are already associated with a *different* containerID now. Only remove
+	// the TopologyHints associated with that podUID and containerName if this is not true
+	if _, err := s.podMap.GetContainerID(podUIDString, containerName); err != nil {
+		delete(s.podTopologyHints[podUIDString], containerName)
 		if len(s.podTopologyHints[podUIDString]) == 0 {
 			delete(s.podTopologyHints, podUIDString)
 		}

pkg/kubelet/cm/topologymanager/scope_container.go

@@ -19,6 +19,7 @@ package topologymanager
 import (
 	"k8s.io/api/core/v1"
 	"k8s.io/klog/v2"
+	"k8s.io/kubernetes/pkg/kubelet/cm/containermap"
 	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
 	"k8s.io/kubernetes/pkg/kubelet/util/format"
 )
@@ -37,7 +38,7 @@ func NewContainerScope(policy Policy) Scope {
 			name:             containerTopologyScope,
 			podTopologyHints: podTopologyHints{},
 			policy:           policy,
-			podMap:           make(map[string]string),
+			podMap:           containermap.NewContainerMap(),
 		},
 	}
 }

pkg/kubelet/cm/topologymanager/scope_pod.go

@@ -19,6 +19,7 @@ package topologymanager
 import (
 	"k8s.io/api/core/v1"
 	"k8s.io/klog/v2"
+	"k8s.io/kubernetes/pkg/kubelet/cm/containermap"
 	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
 	"k8s.io/kubernetes/pkg/kubelet/util/format"
 )
@@ -37,7 +38,7 @@ func NewPodScope(policy Policy) Scope {
 			name:             podTopologyScope,
 			podTopologyHints: podTopologyHints{},
 			policy:           policy,
-			podMap:           make(map[string]string),
+			podMap:           containermap.NewContainerMap(),
 		},
 	}
 }

pkg/kubelet/cm/topologymanager/scope_test.go

@@ -19,6 +19,7 @@ package topologymanager
 import (
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/kubernetes/pkg/kubelet/cm/containermap"
 	"reflect"
 	"testing"
 )
@@ -64,14 +65,13 @@ func TestAddContainer(t *testing.T) {
 		},
 	}
 	scope := scope{}
-	scope.podMap = make(map[string]string)
+	scope.podMap = containermap.NewContainerMap()
 	for _, tc := range testCases {
 		pod := v1.Pod{}
 		pod.UID = tc.podUID
-		err := scope.AddContainer(&pod, tc.containerID)
-		if err != nil {
-			t.Errorf("Expected error to be nil but got: %v", err)
-		}
+		container := v1.Container{}
+		container.Name = tc.name
+		scope.AddContainer(&pod, &container, tc.containerID)
 		if val, ok := scope.podMap[tc.containerID]; ok {
 			if reflect.DeepEqual(val, pod.UID) {
 				t.Errorf("Error occurred")
@@ -100,18 +100,27 @@ func TestRemoveContainer(t *testing.T) {
 		},
 	}
 	var len1, len2 int
+	var lenHints1, lenHints2 int
 	scope := scope{}
-	scope.podMap = make(map[string]string)
+	scope.podMap = containermap.NewContainerMap()
+	scope.podTopologyHints = podTopologyHints{}
 	for _, tc := range testCases {
-		scope.podMap[tc.containerID] = string(tc.podUID)
+		scope.podMap.Add(string(tc.podUID), tc.name, tc.containerID)
+		scope.podTopologyHints[string(tc.podUID)] = make(map[string]TopologyHint)
+		scope.podTopologyHints[string(tc.podUID)][tc.name] = TopologyHint{}
 		len1 = len(scope.podMap)
+		lenHints1 = len(scope.podTopologyHints)
 		err := scope.RemoveContainer(tc.containerID)
 		len2 = len(scope.podMap)
+		lenHints2 = len(scope.podTopologyHints)
 		if err != nil {
 			t.Errorf("Expected error to be nil but got: %v", err)
 		}
 		if len1-len2 != 1 {
-			t.Errorf("Remove Pod resulted in error")
+			t.Errorf("Remove Pod from podMap resulted in error")
+		}
+		if lenHints1-lenHints2 != 1 {
+			t.Error("Remove Pod from podTopologyHints resulted in error")
 		}
 	}
 

pkg/kubelet/cm/topologymanager/topology_manager.go

@@ -46,7 +46,7 @@ type Manager interface {
 	// wants to be consulted with when making topology hints
 	AddHintProvider(HintProvider)
 	// AddContainer adds pod to Manager for tracking
-	AddContainer(pod *v1.Pod, containerID string) error
+	AddContainer(pod *v1.Pod, container *v1.Container, containerID string)
 	// RemoveContainer removes pod from Manager tracking
 	RemoveContainer(containerID string) error
 	// Store is the interface for storing pod topology hints
@@ -175,8 +175,8 @@ func (m *manager) AddHintProvider(h HintProvider) {
 	m.scope.AddHintProvider(h)
 }
 
-func (m *manager) AddContainer(pod *v1.Pod, containerID string) error {
-	return m.scope.AddContainer(pod, containerID)
+func (m *manager) AddContainer(pod *v1.Pod, container *v1.Container, containerID string) {
+	m.scope.AddContainer(pod, container, containerID)
 }
 
 func (m *manager) RemoveContainer(containerID string) error {