node: cm: don't share containerMap instances between managers

[ffromani]

What type of PR is this?

/kind bug
/kind failing-test
/kind regression

What this PR does / why we need it:

Since the GA graduation of memory manager in #128517 we are sharing the initial container map across managers.

The intention of this sharing was not to actually share a data structure, but

1. save the relatively expensive relisting from runtime
2. have all the managers share a consistent view - even though the chance for misalignement tend to be tiny.

The unwanted side effect though is now all the managers race to modify a shared, not thread safe data structure.

The fix is to clone (deepcopy) the computed map when passing it to each manager. This restores the old semantic of the code.

This issue brings the topic of possibly managers go out of sync since each of them maintain a private view of the world. This risk is real, yet very much likely tiny, and this is how the code worked for most of the lifetime, so the plan is to look at this and evaluate possible improvements later on.

Which issue(s) this PR fixes:

Fixes #128638

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

NONE

[ffromani]

/test pull-crio-cgroupv1-node-e2e-resource-managers
/test pull-kubernetes-node-e2e-containerd-serial-ec2
/test pull-kubernetes-node-kubelet-serial-containerd
/test pull-kubernetes-node-kubelet-serial-cpu-manager
/test pull-kubernetes-node-kubelet-serial-memory-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager
/test pull-kubernetes-node-kubelet-serial-containerd

[ffromani]

/cc @SergeyKanzhelev

[haircommander]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 8228c9743572fdfa082c8ca60f8edf45a9f95c48

[ffromani]

New changes are detected. LGTM label has been removed.

wanted to add another test to exercise the new workhorse Clone function

[haircommander]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 5a434c477fb4d8dac0226d0e5266de301a06845b

[ffromani]

/test pull-crio-cgroupv1-node-e2e-resource-managers
/test pull-kubernetes-node-e2e-containerd-serial-ec2
/test pull-kubernetes-node-kubelet-serial-containerd
/test pull-kubernetes-node-kubelet-serial-cpu-manager
/test pull-kubernetes-node-kubelet-serial-memory-manager
/test pull-kubernetes-node-kubelet-serial-topology-manager
/test pull-kubernetes-node-kubelet-serial-containerd

[ffromani]

/assign @klueska
/assign @mrunalp

[ffromani]

/triage accepted
/priority critical-urgent

again pretty evident from the context

[dims]

The addition of the Clone method and its use is very straight forward, so i am comfortable landing this to unblock ourselves from test failures so other things can land just before code freeze.

/approve
/lgtm

[ffromani]

/test pull-kubernetes-integration

failure seems to be unrelated

[SergeyKanzhelev]

@ffromani why any of those managers are writing to this map?

[ffromani]

@ffromani why any of those managers are writing to this map?

because the reconciliation loops, and to keep their copy up to date when container come and go

[SergeyKanzhelev]

@ffromani why any of those managers are writing to this map?

because the reconciliation loops, and to keep their copy up to date when container come and go

oh, I see. This is just an initial list. I'm surprised this is the first time we see it panics

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, ffromani, SergeyKanzhelev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/kubelet/cm/OWNERS [SergeyKanzhelev,dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ffromani]

@ffromani why any of those managers are writing to this map?

because the reconciliation loops, and to keep their copy up to date when container come and go

oh, I see. This is just an initial list. I'm surprised this is the first time we see it panics

previously, each manager used its instance of the map returned by buildContainerMapAndRunningSetFromRuntime. The main(/only) reason was each manager could in theory be enabled independently. Device manager happened to not use the map, so the race was present but dormant.

When we GA'd memory manager we removed the feature gate and the duplicated call to buildContainerMapAndRunningSetFromRuntime (doesn't make sense to call it twice in 30 lines of code) and triggered the bug. The commit message provides more context.