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

change: [M#-8735] - Increase Cloud Manager node.js memory allocation (development jobs) #11084

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Oct 10, 2024

Description 📝

Small PR to increase the memory allocation for Node.js for vite, TS compiler and storybook

Screenshot 2024-10-10 at 12 13 37

Changes 🔄

  • Increase Node memory allocation (approximately 2GB before, to 4GB) for
  • vite
  • tsc
  • storybook

How to test 🧪

Verification steps

  • run yarn start or yarn up
  • run yarn storybook
  • make few changes across the codebase, save a bunch of things
  • confirm no heap memory allocation in the console

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@abailly-akamai abailly-akamai requested a review from a team as a code owner October 10, 2024 16:16
@abailly-akamai abailly-akamai requested review from hana-akamai and cpathipa and removed request for a team October 10, 2024 16:16
@abailly-akamai abailly-akamai self-assigned this Oct 10, 2024
@abailly-akamai abailly-akamai changed the title change: [M#-8735] - Increase Cloud Manager node.js and TS memory limits (development jobs) change: [M#-8735] - Increase Cloud Manager node.js memory allocation (development jobs) Oct 10, 2024
@@ -86,7 +86,7 @@
"zxcvbn": "^4.4.2"
},
"scripts": {
"start": "concurrently --raw \"vite\" \"tsc --watch --preserveWatchOutput\"",
"start": "concurrently --raw \"NODE_OPTIONS='--max-old-space-size=4096' vite\" \"NODE_OPTIONS='--max-old-space-size=4096' tsc --watch --preserveWatchOutput\"",
Copy link
Member

@bnussman-akamai bnussman-akamai Oct 10, 2024

Choose a reason for hiding this comment

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

From what I observed, it is only the tsc process that has the OOM issue. I think we'd likely fix the issue by just apply the fix to the tsc command, but at the same time, I don't think it will hurt having it on the Vite processes too

Suggested change
"start": "concurrently --raw \"NODE_OPTIONS='--max-old-space-size=4096' vite\" \"NODE_OPTIONS='--max-old-space-size=4096' tsc --watch --preserveWatchOutput\"",
"start": "concurrently --raw \"vite\" \"NODE_OPTIONS='--max-old-space-size=4096' tsc --watch --preserveWatchOutput\"",

Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to update the "start" command to have -n vite,tsc so we can better understand which process is crashing but we don't have to

For example:

"start": "concurrently -n vite,tsc \"vite\" \"tsc --watch --preserveWatchOutput\"",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this could be done locally to debug the underlying issues in a separate task - we should have a separate ticket to investigate this - this PR facilitates development for now

Copy link

Coverage Report:
Base Coverage: 86.97%
Current Coverage: 86.97%

@abailly-akamai abailly-akamai merged commit d37a032 into linode:develop Oct 11, 2024
22 of 23 checks passed
Copy link

cypress bot commented Oct 11, 2024

Cloud Manager E2E    Run #6665

Run Properties:  status check passed Passed #6665  •  git commit d37a032cb3: change: [M#-8735] - Increase Cloud Manager `node.js` memory allocation (developm...
Project Cloud Manager E2E
Run status status check passed Passed #6665
Run duration 29m 10s
Commit git commit d37a032cb3: change: [M#-8735] - Increase Cloud Manager `node.js` memory allocation (developm...
Committer Alban Bailly
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 8
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants