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

Set GOMAXPROCS automatically #41151

Closed

Conversation

sftim
Copy link
Contributor

@sftim sftim commented May 15, 2023

Fix frequently-broken production deploys by forcing GOMAXPROCS to a sensible value. Follows #41142

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 15, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from sftim. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 sig/docs Categorizes an issue or PR as relevant to SIG Docs. label May 15, 2023
@sftim
Copy link
Contributor Author

sftim commented May 15, 2023

nproc returns 32 for a deploy preview.

@sftim sftim force-pushed the 20230515_set_go_max_procs_automatically branch from c28047d to a134001 Compare May 15, 2023 13:52
@netlify
Copy link

netlify bot commented May 15, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit c28047d
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/646236fe970ef6000887783e
😎 Deploy Preview https://deploy-preview-41151--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sftim
Copy link
Contributor Author

sftim commented May 15, 2023

OK, so this hasn't helped like I'd like it to. I think nproc is reporting an unhelpfully high value and this is causing Hugo to try to use too much concurrency.

I suspect there is also a concurrency bug of some kind in Hugo, but I'm not sure.

/close

@k8s-ci-robot
Copy link
Contributor

@sftim: Closed this PR.

In response to this:

OK, so this hasn't helped like I'd like it to. I think nproc is reporting an unhelpfully high value and this is causing Hugo to try to use too much concurrency.

I suspect there is also a concurrency bug of some kind in Hugo, but I'm not sure.

/close

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.

@tengqm
Copy link
Contributor

tengqm commented May 15, 2023

Checked this: moby/moby#43587

This is actually annoying:

This is because the CPU constraints are using the kernels' CFS ("completely fair scheduler"), which, depending on load, gives each process a certain amount of CPU time to use.

The actual number of CPUs doesn't change, and when looked from inside the container, isn't reflected in the information provided by the kernel (which always shows all CPUs to be present; /proc is not "namespace aware" in that respect).

@sftim
Copy link
Contributor Author

sftim commented May 15, 2023

It's a bit annoying that having 1 CPU and believing that there are 32 leads to either:

  • quick builds
  • apparent deadlocks

I think there might still be a bug in Go, Hugo, or one of their dependencies, but I haven't the appetite to track it down.

@tengqm
Copy link
Contributor

tengqm commented May 16, 2023

Another related issue is about the section-index partial timeout.
I checked the layouts/docs/list.html where the section-index partial is invoked.
It turned out that we are reusing the themes/docsy/layouts/partials/section-index.html, but we have our own layouts/docs/list.html, which overrides the themes/docsy/layouts/docs/list.html.
In short, we are invoking the upstream version "section-index" from our derived version of "list.html".

Besides this, the Docsy theme we are using is pretty old. We missed 0.6.0, 0.5.0, ... updates from Docsy. These inconsistencies all complicate the issue we have about section-index timeout.

Call for action:

  • Upgrade Docsy to its latest stable release: 0.6.0 at least;
  • Check if we really want our own customization of the 'list.html' file and drop the customized version if possible.

@tengqm tengqm mentioned this pull request May 16, 2023
@sftim sftim deleted the 20230515_set_go_max_procs_automatically branch September 23, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants