diff --git a/pkg/kubelet/cm/internal_container_lifecycle.go b/pkg/kubelet/cm/internal_container_lifecycle.go index 690718e4e68ac..060b03e1e1d8b 100644 --- a/pkg/kubelet/cm/internal_container_lifecycle.go +++ b/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 } diff --git a/pkg/kubelet/cm/topologymanager/BUILD b/pkg/kubelet/cm/topologymanager/BUILD index 3405dbc488cba..0d5130fc13970 100644 --- a/pkg/kubelet/cm/topologymanager/BUILD +++ b/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", diff --git a/pkg/kubelet/cm/topologymanager/fake_topology_manager.go b/pkg/kubelet/cm/topologymanager/fake_topology_manager.go index a21e50c555a8b..d622cd5af6a36 100644 --- a/pkg/kubelet/cm/topologymanager/fake_topology_manager.go +++ b/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 { diff --git a/pkg/kubelet/cm/topologymanager/fake_topology_manager_test.go b/pkg/kubelet/cm/topologymanager/fake_topology_manager_test.go index 8297474311fb4..517b265071232 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 a7948c0b00cfb..98c1c0b4dcb47 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.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) } diff --git a/pkg/kubelet/cm/topologymanager/scope_container.go b/pkg/kubelet/cm/topologymanager/scope_container.go index 0a221b0259b6b..f6a829c15d281 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" "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(), }, } } diff --git a/pkg/kubelet/cm/topologymanager/scope_pod.go b/pkg/kubelet/cm/topologymanager/scope_pod.go index 11e66ac47c066..878caf1a9ebbd 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" "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(), }, } } diff --git a/pkg/kubelet/cm/topologymanager/scope_test.go b/pkg/kubelet/cm/topologymanager/scope_test.go index 4d6315adb72c3..bb0dccf0a90ed 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 00ebf490bd357..70a73080b77a8 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 {