diff --git a/pkg/kubelet/cm/internal_container_lifecycle.go b/pkg/kubelet/cm/internal_container_lifecycle.go index 278f13eb08e..92b36c2f9af 100644 --- a/pkg/kubelet/cm/internal_container_lifecycle.go +++ b/pkg/kubelet/cm/internal_container_lifecycle.go @@ -50,10 +50,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 } diff --git a/pkg/kubelet/cm/memorymanager/policy_static_test.go b/pkg/kubelet/cm/memorymanager/policy_static_test.go index fd27c581323..ef30457215c 100644 --- a/pkg/kubelet/cm/memorymanager/policy_static_test.go +++ b/pkg/kubelet/cm/memorymanager/policy_static_test.go @@ -83,9 +83,8 @@ func (m *fakeTopologyManagerWithHint) AddHintProvider(h topologymanager.HintProv m.t.Logf("[fake topologymanager] AddHintProvider HintProvider: %v", h) } -func (m *fakeTopologyManagerWithHint) AddContainer(pod *v1.Pod, containerID string) error { - m.t.Logf("[fake topologymanager] AddContainer pod: %v container id: %v", format.Pod(pod), containerID) - return nil +func (m *fakeTopologyManagerWithHint) AddContainer(pod *v1.Pod, container *v1.Container, containerID string) { + m.t.Logf("[fake topologymanager] AddContainer pod: %v container name: %v container id: %v", format.Pod(pod), container.Name, containerID) } func (m *fakeTopologyManagerWithHint) RemoveContainer(containerID string) error { diff --git a/pkg/kubelet/cm/topologymanager/fake_topology_manager.go b/pkg/kubelet/cm/topologymanager/fake_topology_manager.go index 063cac65a27..1e7a831108d 100644 --- a/pkg/kubelet/cm/topologymanager/fake_topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/fake_topology_manager.go @@ -39,9 +39,8 @@ func (m *fakeManager) AddHintProvider(h HintProvider) { klog.InfoS("AddHintProvider", "hintProvider", h) } -func (m *fakeManager) AddContainer(pod *v1.Pod, containerID string) error { - klog.InfoS("AddContainer", "pod", klog.KObj(pod), "containerID", containerID) - return nil +func (m *fakeManager) AddContainer(pod *v1.Pod, container *v1.Container, containerID string) { + klog.InfoS("AddContainer", "pod", klog.KObj(pod), "containerName", container.Name, "containerID", containerID) } func (m *fakeManager) RemoveContainer(containerID string) error { diff --git a/pkg/kubelet/cm/topologymanager/fake_topology_manager_test.go b/pkg/kubelet/cm/topologymanager/fake_topology_manager_test.go index 8297474311f..517b2650712 100644 --- a/pkg/kubelet/cm/topologymanager/fake_topology_manager_test.go +++ b/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 diff --git a/pkg/kubelet/cm/topologymanager/scope.go b/pkg/kubelet/cm/topologymanager/scope.go index af90663368a..c5c6f36be97 100644 --- a/pkg/kubelet/cm/topologymanager/scope.go +++ b/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.InfoS("RemoveContainer", "containerID", 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) } diff --git a/pkg/kubelet/cm/topologymanager/scope_container.go b/pkg/kubelet/cm/topologymanager/scope_container.go index e5d331e00e9..de45209625a 100644 --- a/pkg/kubelet/cm/topologymanager/scope_container.go +++ b/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" ) @@ -36,7 +37,7 @@ func NewContainerScope(policy Policy) Scope { name: containerTopologyScope, podTopologyHints: podTopologyHints{}, policy: policy, - podMap: make(map[string]string), + podMap: containermap.NewContainerMap(), }, } } diff --git a/pkg/kubelet/cm/topologymanager/scope_pod.go b/pkg/kubelet/cm/topologymanager/scope_pod.go index f4645bc4d76..9ccc6414dd9 100644 --- a/pkg/kubelet/cm/topologymanager/scope_pod.go +++ b/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" ) @@ -36,7 +37,7 @@ func NewPodScope(policy Policy) Scope { name: podTopologyScope, podTopologyHints: podTopologyHints{}, policy: policy, - podMap: make(map[string]string), + podMap: containermap.NewContainerMap(), }, } } diff --git a/pkg/kubelet/cm/topologymanager/scope_test.go b/pkg/kubelet/cm/topologymanager/scope_test.go index 4d6315adb72..bb0dccf0a90 100644 --- a/pkg/kubelet/cm/topologymanager/scope_test.go +++ b/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") } } diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index f1e435260de..4f327e6efc0 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/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 {