Make RuntimeInfo thread safe

[mszadkow]

What happened?

Calling RuntimeInfo concurrently causes data race as observed in this Kueue issue.
As a workaround we synchronise calls to RuntimeInfo.
Seems that the issue comes from syncPodSets access.

What did you expect to happen?

Make RuntimeInfo thread safe

Environment

Kubernetes version:

$ kubectl version

Kubeflow Trainer version:

$ kubectl get pods -n kubeflow-system -l app.kubernetes.io/name=kubeflow-trainer -o jsonpath="{.items[*].spec.containers[*].image}"

Kubeflow Python SDK version:

$ pip show kubeflow

Impacted by this bug?

Give it a 👍 We prioritize the issues with most 👍

[mimowo]

FYI, as a quick fix we wrap the call with a mutex, but we have TODO to remove in the future: kubernetes-sigs/kueue#7132

[andreyvelich]

Thanks for reporting this @mszadkow!
I think, we can fix it.

@kaisoz Regarding this comment: kubernetes-sigs/kueue#7078 (comment), after this PR we will register indexer only when Extension Framework is initialized : https://github.com/kubeflow/trainer/pull/2729/files#diff-f93e4d2eb3f5d1f79c0e6e4ef9f4eec2c316d9b088b99ae25e363a0c670d0dcfR159

/cc @tenzen-y @astefanutti @Electronic-Waste

/remove-label lifecycle/needs-triage
/good-first-issue

[google-oss-prow]

@andreyvelich:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Thanks for reporting this @mszadkow!
I think, we can fix it.

@kaisoz Regarding this comment: kubernetes-sigs/kueue#7078 (comment), after this PR we will register indexer only when Extension Framework is initialized : https://github.com/kubeflow/trainer/pull/2729/files#diff-f93e4d2eb3f5d1f79c0e6e4ef9f4eec2c316d9b088b99ae25e363a0c670d0dcfR159

/cc @tenzen-y @astefanutti @Electronic-Waste

/remove-label lifecycle/needs-triage
/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Deepam02]

hi @andreyvelich
can i work on this?

/assign

[astefanutti]

@Deepam02 thanks, at quick glance it seems the culprit is that global variable:

trainer/pkg/runtime/runtime.go

Line 35 in b918411

syncPodSets = defaultPodSetsSyncer

that should be made a field of the runtime.Info.

[tenzen-y]

IIRC, we planned to migrate all object handling to SSA which means eliminating typed object usages and use ApplyConfiguration everywhere, then we could remove Syncer and typed <-> applyconfiguration converter and so on.

But it is still under construction, which might take a bit of time. So, it would be better to introduce a hot fix as @astefanutti mentioned.