fix removing pods from podTopologyHints mapping

This commit is contained in:
aheng-ch 2021-04-30 15:20:36 +08:00
parent b4ea4322a6
commit ff7b94fa5a
9 changed files with 47 additions and 64 deletions

View File

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

View File

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

View File

@ -39,9 +39,8 @@ func (m *fakeManager) AddHintProvider(h HintProvider) {
klog.InfoS("AddHintProvider", "hintProvider", h) klog.InfoS("AddHintProvider", "hintProvider", h)
} }
func (m *fakeManager) AddContainer(pod *v1.Pod, containerID string) error { func (m *fakeManager) AddContainer(pod *v1.Pod, container *v1.Container, containerID string) {
klog.InfoS("AddContainer", "pod", klog.KObj(pod), "containerID", containerID) klog.InfoS("AddContainer", "pod", klog.KObj(pod), "containerName", container.Name, "containerID", containerID)
return nil
} }
func (m *fakeManager) RemoveContainer(containerID string) error { func (m *fakeManager) RemoveContainer(containerID string) error {

View File

@ -21,7 +21,6 @@ import (
"testing" "testing"
"k8s.io/api/core/v1" "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/kubernetes/pkg/kubelet/lifecycle" "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) { func TestFakeRemoveContainer(t *testing.T) {
testCases := []struct { testCases := []struct {
name string name string

View File

@ -23,6 +23,7 @@ import (
"k8s.io/api/core/v1" "k8s.io/api/core/v1"
"k8s.io/klog/v2" "k8s.io/klog/v2"
"k8s.io/kubernetes/pkg/kubelet/cm/containermap"
"k8s.io/kubernetes/pkg/kubelet/lifecycle" "k8s.io/kubernetes/pkg/kubelet/lifecycle"
) )
@ -43,7 +44,7 @@ type Scope interface {
// wants to be consoluted with when making topology hints // wants to be consoluted with when making topology hints
AddHintProvider(h HintProvider) AddHintProvider(h HintProvider)
// AddContainer adds pod to Manager for tracking // 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 removes pod from Manager tracking
RemoveContainer(containerID string) error RemoveContainer(containerID string) error
// Store is the interface for storing pod topology hints // Store is the interface for storing pod topology hints
@ -60,8 +61,8 @@ type scope struct {
hintProviders []HintProvider hintProviders []HintProvider
// Topology Manager Policy // Topology Manager Policy
policy Policy policy Policy
// Mapping of PodUID to ContainerID for Adding/Removing Pods from PodTopologyHints mapping // Mapping of (PodUid, ContainerName) to ContainerID for Adding/Removing Pods from PodTopologyHints mapping
podMap map[string]string podMap containermap.ContainerMap
} }
func (s *scope) Name() string { 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 // It would be better to implement this function in topologymanager instead of scope
// but topologymanager do not track mapping anymore // 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() s.mutex.Lock()
defer s.mutex.Unlock() defer s.mutex.Unlock()
s.podMap[containerID] = string(pod.UID) s.podMap.Add(string(pod.UID), container.Name, containerID)
return nil
} }
// It would be better to implement this function in topologymanager instead of scope // 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() defer s.mutex.Unlock()
klog.InfoS("RemoveContainer", "containerID", containerID) klog.InfoS("RemoveContainer", "containerID", containerID)
podUIDString := s.podMap[containerID] // Get the podUID and containerName associated with the containerID to be removed and remove it
delete(s.podMap, containerID) podUIDString, containerName, err := s.podMap.GetContainerRef(containerID)
if _, exists := s.podTopologyHints[podUIDString]; exists { if err != nil {
delete(s.podTopologyHints[podUIDString], containerID) 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 { if len(s.podTopologyHints[podUIDString]) == 0 {
delete(s.podTopologyHints, podUIDString) delete(s.podTopologyHints, podUIDString)
} }

View File

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

View File

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

View File

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

View File

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