Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make parallel build memory threshold configurable #96882

Merged

Conversation

saschagrunert
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

The amount of memory required to build binaries in parallel is right now
set to 40GiB. We now make this variable to be able to build artifacts in
parallel even with a lower amount of memory.

This enables SIG Release to speed-up the build time drastically in
Google Cloud Build (GCB).

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

cc @kubernetes/release-engineering
/sig release

Does this PR introduce a user-facing change?:

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/release Categorizes an issue or PR as relevant to SIG Release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 26, 2020
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2020
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@saschagrunert
Copy link
Member Author

/test pull-kubernetes-e2e-kind

@puerco
Copy link
Member

puerco commented Nov 27, 2020

/lgtm

Copy link
Contributor

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@saschagrunert
Copy link
Member Author

@kubernetes/milestone-maintainers can we get this over the finishing line?

@ehashman
Copy link
Member

@saschagrunert that's not the issue, you're missing an approver:

kubernetes/hack/OWNERS

Lines 18 to 31 in 6d43e2b

- bentheelder
- cblecker
- deads2k
- dims # for local-up-cluster related files
- fejta
- lavalamp
- liggitt
- pwittrock
- SataQiu
- soltysh # for sig-cli related stuff
- spiffxp
- sttts
- vishh
- mikedanese

go chase one of these fine folks down :)

@cblecker
Copy link
Member

/hold

So this variable is designed to be used as a deciding factor where if you have more than this amount of memory, you can build in parallel.. and below it, we don't because you experience errors during the build. I'm not clear on how making this configurable changes this.

I'd be okay with is making it configurable in general, but can we expand the comment to provide more context on what this variable does and why?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2020
Comment on lines 308 to 309
# As of January 2018, RAM usage is exceeding 30G
# Setting to 40 to provide some headroom
readonly KUBE_PARALLEL_BUILD_MEMORY=40
# Defaulting to 40G to provide some headroom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't make sense anymore if this isn't hard coded

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note that it can be overwritten at our own risk, whereas it still defaults to 40GB.

The amount of memory required to build binaries in parallel is right now
set to 40GiB. We now make this variable to be able to build artifacts in
parallel even with a lower amount of memory.

This enables SIG Release to speed-up the build time drastically in
Google Cloud Build (GCB).

Signed-off-by: Sascha Grunert <sgrunert@suse.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2021
@saschagrunert
Copy link
Member Author

@cblecker do you think you can give this another look? :)

@saschagrunert
Copy link
Member Author

PING @fejta @deads2k @lavalamp may I ask you for an approval?

@BenTheElder
Copy link
Member

If we're able to lower this, perhaps the default is higher than it needs to be?
I'd expect this to change over time with go releases, kubernetes changes ...

@saschagrunert
Copy link
Member Author

If we're able to lower this, perhaps the default is higher than it needs to be?
I'd expect this to change over time with go releases, kubernetes changes ...

Yeah probably, this was just a test coming from our GCB instance. I was just a bit conservative in changing the default because I did not really know which impact it would have on existing build systems using older go versions. WDYT?

@justaugustus
Copy link
Member

/area release-eng
/triage accepted

@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 16, 2021
@BenTheElder
Copy link
Member

Yeah probably, this was just a test coming from our GCB instance. I was just a bit conservative in changing the default because I did not really know which impact it would have on existing build systems using older go versions. WDYT?

Your GCB instance is using newer than k/k? Or is the concern about backporting to older branches?

Ideally we should have a reasonable default on the current branch, which will evolve with time?

@saschagrunert
Copy link
Member Author

Yeah probably, this was just a test coming from our GCB instance. I was just a bit conservative in changing the default because I did not really know which impact it would have on existing build systems using older go versions. WDYT?

Your GCB instance is using newer than k/k? Or is the concern about backporting to older branches?

Yeah it was mainly about the backports.

Ideally we should have a reasonable default on the current branch, which will evolve with time?

I think we could play around with the value together with CI signal as follow-up.

@saschagrunert
Copy link
Member Author

saschagrunert commented Mar 2, 2021

@BenTheElder do you think we can move forward with this PR and benchmark the actual required threshold after we made it configurable?

Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, hasheddan, saschagrunert, spiffxp, xmudrii

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2021
@BenTheElder
Copy link
Member

Sorry E_TOO_MANY_PRS. Looks like Aaron got it 😅

@spiffxp
Copy link
Member

spiffxp commented Mar 2, 2021

/test pull-kubernetes-unit
prow's status-reconciler should have triggered this but didn't, not sure why

@k8s-ci-robot k8s-ci-robot merged commit 28feb8e into kubernetes:master Mar 3, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 3, 2021
@saschagrunert saschagrunert deleted the parallel-build-configurable branch March 3, 2021 08:34
k8s-ci-robot added a commit that referenced this pull request Apr 27, 2021
…82-upstream-release-1.20

Automated cherry pick of #96882: Make parallel build memory threshold configurable
k8s-ci-robot added a commit that referenced this pull request Apr 27, 2021
…82-upstream-release-1.18

Automated cherry pick of #96882: Make parallel build memory threshold configurable
k8s-ci-robot added a commit that referenced this pull request Apr 27, 2021
…82-upstream-release-1.19

Automated cherry pick of #96882: Make parallel build memory threshold configurable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet