search

kubernetes / test-infra

Test infrastructure for the Kubernetes project.
Apache License 2.0
3.82k stars 2.64k forks source link

consider setting GOMAXPROCS in entrypoint / bootstrap.py #29472

Open BenTheElder opened 1 year ago

BenTheElder commented 1 year ago

What would you like to be added:

We should consider the trade-offs in defaulting GOMAXPROCS in entrypoint. See https://kubernetes.slack.com/archives/CCK68P2Q2/p1683658558671579

Why is this needed:

To deal with https://github.com/golang/go/issues/33803 ahead of go setting it.

Downsides: this can hide that the projects/tools themselves are not handling this, and go is tentatively doing this in the standard runtime anyhow ...

Projects can currently instead use https://github.com/uber-go/automaxprocs themselves until go is fixed. Kubernetes unit tests do this now.

I kinda lean towards "we should only do this if it's fixing widespread issues" and instead waiting on https://github.com/golang/go/issues/33803 for a standardized approach.

But setting it today might help resolve flakiness in jobs that have CPU limits enabled in particular. We have evidence of this with Kubernetes unit tests previously, though those are a particularly resource heavy case.

dims commented 1 year ago

+1 to "we should only do this if it's fixing widespread issues"

+1 to "I'd just go ahead and migrate jobs and worry about this if we actually see issues, it's cheap to migrate a job back to the other cluster, since it's just one field in the config" Quote from @BenTheElder at: https://kubernetes.slack.com/archives/CCK68P2Q2/p1683659191867329?thread_ts=1683658558.671579&cid=CCK68P2Q2

sftim commented 1 year ago

For whole-number CPU limits, is CPU Manager static assignment an option?

I presume the job is either CPU bound or close enough to it that static assignment would be OK. The Pod would have to be Guaranteed, though.

BenTheElder commented 1 year ago

For whole-number CPU limits, is CPU Manager static assignment an option?

Aside from non-whole limits, it is possible to enable this on the GKE clusters at least.

I presume the job is either CPU bound or close enough to it that static assignment would be OK. The Pod would have to be Guaranteed, though.

Prow pods running on k8s infra are guaranteed QOS, enforced by a presubmit.

However we'd have to lessen assignment since some CPU is reserved for system overhead and we're requesting that remaining partial core in some cases. (This is also a serious problem with using a very different machine size on the EKS cluster vs the existing GKE clusters which all have the same machine size), we push our scheduleable bounds pretty hard.

I don't think this problem is actually widespread enough to warrant changes like this, and conversely if it were then we'd want go + prow to work well without requiring highly priviledged node configuration that may not be exposed in all cluster tools.

kumiDa commented 1 year ago

/sig testing

k8s-triage-robot commented 8 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 7 months ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

BenTheElder commented 7 months ago

/remove-lifecycle rotten

This still hasn't changed in the go runtime and is still worth considering. We could probably cut down on a lot of flakes across CI.

BenTheElder commented 5 months ago

NOTE: https://github.com/golang/go/issues/33803 hasn't moved.

cc @pohly @aojea

pohly commented 5 months ago

No strong opinion either way from me here.

aojea commented 5 months ago

magic settings always fire back :/

We could probably cut down on a lot of flakes across CI.

or we don't and create new ones

BenTheElder commented 5 months ago

magic settings always fire back :/

Eh? We also control things like GOPROXY / GOSUM, the user ID, and lots of other ambient values / environment settings

or we don't and create new ones

How do you think this will introduce flakes?

The fundamental problem right now is the go runtime is being over-provisioned versus the CPU assigned to the workload (in this case the e2e tests). It is a well-known mitigation that many projects use to either manually set GOMAXPROCS to match your environment configuration or use the uber library to automatically read cgroups and set this.

We're relying on this fix ourselves in multiple places already including kubernetes/kubernetes unit tests / builds (the makefiles set it with the uber library) and registry.k8s.io (we manually set it to match the cloud run environment)

aojea commented 5 months ago

GOMAXPROCS sets the maximum number of CPUs that can be executing simultaneously and returns the previous setting

magic as changing the physical environment perceived by the runtime :)

How do you think this will introduce flakes?

I don't know, but my experience with this kind of changes at this low level is that always have to expect the unexpected, if we have specific places like hte integration tests or these things that seems a controlled environment, and if something fails you can always trace it back ... enabling something like this globally always can be harder to pinpoint if it causes unexpected problems

BenTheElder commented 5 months ago

magic as changing the physical environment perceived by the runtime :)

It's not "magical", we're telling Go how many cores it should assume for scheduling, that's why the environment variable exists. Users can also override this.

I don't know, but my experience with this kind of changes at this low level is that always have to expect the unexpected, if we have specific places like the integration tests or these things that seems a controlled environment, and if something fails you can always trace it back ... enabling something like this globally always can be harder to pinpoint if it causes unexpected problems

anything using Go is subject to flakiness due to Go assuming it can schedule to M physical cores on the host when we have M < N limits on the container we're running in, this is why we already set it for k/k.

However, we don't set it for other subprojects or for other CI tests that are not using the k/k shell in any way.

So, to clarify, we're saying that prow should, when running CI jobs in Kubernetes pods, ensure that GOMAXPROCS is <= the CPU limits on the test container.

All of the environment details are logged in the job artifacts.

without setting this knob you will not reproduce flakes locally because they happen when go detects the full CPUs of the host but the job has been constrained, which causes thrashing in go's userspace scheduler. See https://github.com/golang/go/issues/33803 and https://github.com/uber-go/automaxprocs

So Go sees "I'm on an 8 core VM" and we run the job with CPU limit = 2 cores. It doesn't detect the 2 core CPU throttling limit and makes a bad assumption about scheduling. This impacts any integration, unit, whatever.

Then you run the test locally and you have 32 cores and no CPU limits set, so you can't reproduce this flakiness. You would have to run in a container AND actually set CPU limits similar to Kubernetes AND be on a similarly sized host.

Whereas if we do set GOMAXPROCS in prow, well the number of cores available will never be identical to your local machine, but it CAN be that go is using the correct number of cores both in local testing and in CI and you should get comparable behavior.

Again: for the jobs you typically debug, we already do this, but for some other random subproject their tests are probably flaky without this being set, and that is worsened by SIG K8s Infra adopting more clusters with even larger hosts but the same or lower CPU limits on the jobs

BenTheElder commented 5 months ago

https://github.com/kubernetes/kubernetes/pull/117016

aojea commented 5 months ago

ok , you convinced me , how do we track the rollout of this change to avoid we don't hit some weird edge case?

BenTheElder commented 5 months ago

If we do this in entrypoint, it will be picked up in the next prow bump and we should watch that and announce in #sig-testing / #release-ci-signal / #testing-ops at minimum.

aojea commented 5 months ago

ok, then it is better to do it early in the release cycle or after the development cycle, so we have better signal

BenTheElder commented 4 months ago

/assign

This is low-priority but something we should do I think. It should not affect k/k CI since the relevant jobs are already running the in-tree auto-max-procs but we'll see.

k8s-triage-robot commented 1 month ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

BenTheElder commented 1 month ago

/remove-lifecycle stale

https://github.com/kubernetes/test-infra/pull/33271/files/8858165b56cbdc6e3e2de477c562661cc46a817c#diff-b6403e48d3defcd3266ffeadf6a8d1be27dc81b22becb7d43007ff62dc123bcd is an interesting thought