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

[jb] check maxHeapSize of IDE server and guide users to set vmoptions in .gitpod.yml #11002

Closed
wants to merge 1 commit into from

Conversation

yaohui-wyh
Copy link
Contributor

@yaohui-wyh yaohui-wyh commented Jun 29, 2022

Description

Quote #10924

An idea is to detect a user change to the actual max heap size in the backend plugin and suggest a user to put in .gitpod.yml to persist. So the flow would be like:

  • a user starts using with default Xmx
  • experience perf issues and follow notifications to upgrade Xmx of the running instance
  • after JB backend restart there will be a warning on .gitpod.yml that Xmx of the running instance was changed, but it should be applied to .gitpod.yml to be persistent

Also check out #10768 for more background info

This PR implements Xmx config checks in JetBrains gitpod-remote plugin:

  • Register a background activity (GitpodStartupActivity) for handling postStart events
  • Add YAML plugin dependency for parsing & navigating PSI elements in .gitpod.yml
  • Show warning notification when the runtime Xmx is different from the configured Xmx
    • Add edit/create-if-missing .gitpod.yml actions, and navigate caret to vmoptions YAML key if exists
    • Add help action for browsing gitpod.yml references
  • Set jvmArgs for the runIde task (only affect local sandbox IDE instance with the developed plugin), which can be modified for local testing purpose

Related Issue(s)

Resolve #10924

How to test

Test locally:

  • Run runIde task from IntelliJ plugin, and modify jvmArgs = listOf("-Xmx4g") to simulate the user's change to the -Xmx of JetBrains IDE server inside a Gitpod workspace. When sandbox IDE instance is started, a warning Notification should popup, indicating current runtime maxHeapSize is different with default/configured vmoptions from .gitpod.yml. Click on "Edit .gitpod.yml" would open the file in the editor

    • edit .gitpod.yml action
  • If there is no .gitpod.yml in the project root, the notification popup would suggest the user to create a .gitpod.yml with generated content (with the current runtime Xmx configured)

    • create .gitpod.yml action
    • .gitpod.yml created

Release Notes

- Detect user changes to the max heap size of IDE server when using JetBrains Gateway, and suggest a user to put in .gitpod.yml to persist.

Documentation

Werft options:

  • /werft with-preview

… in .gitpod.yml

- Register a background activity (GitpodStartupActivity) for handling postStart events
- Add YAML plugin dependency for parsing & navigating PSI elements (AST) in .gitpod.yml
- Show warning notification when the runtime Xmx is different from the configured Xmx
  - Add edit/create-if-missing .gitpod.yml quick actions, and navigate caret to `vmoptions` YAML key if exists
  - Add help action for browsing gitpod.yml references
- Set -Xmx for the `runIde` task (only affect local sandbox IDE instance with the developed plugin), which can be modified for local testing purpose
@akosyakov
Copy link
Member

akosyakov commented Jun 29, 2022

/werft run

👍 started the job as gitpod-build-yh-backend-plugin-fork.0
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-yh-backend-plugin.0 because the annotations in the pull request description changed
(with .werft/ from main)

return YAMLUtil.getQualifiedKeyInFile(psiFile, jetbrains, productName, vmOptions)
}

private fun showEditVMOptionsNotification(project: Project, runtimeXmxMiB: Long, configXmxMiB: Long, productName: String) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need notificaiotns. They are too loud. Can we use inspections instead? https://plugins.jetbrains.com/docs/intellij/code-inspections.html

import org.jetbrains.yaml.psi.YAMLKeyValue
import java.nio.file.Paths

class GitpodStartupActivity : StartupActivity.Background {
Copy link
Member

Choose a reason for hiding this comment

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

We should perform validation whenever content of .gitpod.yml is changing even if dirty. Not only once. Should not it be rather local inspection?

OpenFileAction.openFile(gitpodYaml, project)
val vmOptionsKeyValue = getGitpodYamlVMOptionsPsiElement(project, gitpodYaml, productName)
// navigate caret to "vmoptions" if exist
vmOptionsKeyValue?.navigate(true)
Copy link
Member

Choose a reason for hiding this comment

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

Cannot we automatically update it, instead of asking a user to edit?

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I think warning should not be notifications, but dynamically update inspections. And there should be automatic quick fixes.

object GitpodConfig {

const val gitpodYamlFile = ".gitpod.yml"
const val defaultXmxOption = "-Xmx2048m"
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to provide as env var from go status endpoint where we set it.

val notificationGroup = NotificationGroupManager.getInstance().getNotificationGroup("Gitpod Notifications")
val title = "Gitpod memory settings"
val message = """
|Current maxHeapSize <code>-Xmx${runtimeXmxMiB}m</code> is not matched to configured <code>-Xmx${configXmxMiB}m</code>.<br/>
Copy link
Member

Choose a reason for hiding this comment

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

There should be a warning inspection:
IDE's max heap size (-Xmx) is {runtimeValue}M, but not configured in .gitpod.yml - if it is missing in .gitpod.yml
IDE's max heap size (-Xmx) is {runtimeValue}M, but {configuredXmx}M in .gitpod.yml - if it is defined in .gitpod.yml

There should be quick fixes:

  • Add -Xmx{runtimeValue}M to .gitpod.yml (both cases) - It should automatically create .gitpod.yml if necessary and sections or update vmoptions string
  • Apply default -Xmx{defaultValue} (if not in .gitpod.yml) - it should apply default value using VMOptions api
  • Apply -Xmx{defaultValue} configured in .gitpod.yml (if not in .gitpod.yml) - it should apply configured value using VMOptions api

@yaohui-wyh
Copy link
Contributor Author

Hi @akosyakov, thanks for the quick reply. About the notification part:

I do understand the Notification is a bit noisy and interrupting, so I was trying to make it a post-start activity once only. Inspection on .gitpod.yml might not fit into the memory settings scenario with:

  • Suppose a user experience perf issues and upgrade Xmx of the running instance (e.g. from the control center window of JBClient).
  • After JB backend restart the IDE would reopen the recent files of the project and the user continued on their work, and is unlikely to open (maybe also unaware of) the .gitpod.yml until they stop the workspace.
  • Due to the immutable nature of vmoptions, when they open the same project in a new Gitpod workspace they will encounter the perf issue again.
  • Inspections on .gitpod.yml cannot help users familiarize themselves with how to persist the vmoptions settings.

JetBrains IDEs also use Warning Notification & Dialog for low memory warning & settings:

image

image

@yaohui-wyh
Copy link
Contributor Author

yaohui-wyh commented Jun 29, 2022

In short, I totally understand your points and agree with the necessity of Inspection & quickfix. IMHO, Inspection & quickfix features are more about adding Code IntelliSense to gitpod.yml schema (which I think is also nice to have). However, I was trying to solve the problem raised by the user story of #10924 (i.e. trying to help users to "gitpodify" a persistent IDE's memory setting with project-wide .gitpod.yml), which I might just understand it literally 😿

@akosyakov
Copy link
Member

Can we run subset automatically on startup? Then a user should have a yellow warning sign in problems view at the bottom. I think having good diagnostics and automatic quick fixes which goes in both directions should be a starting point. If we see that it does not help we can be more noisy. Even if we go with the notification I would prefer automatically update .gitpod.yml instead of asking user to read something.

@yaohui-wyh
Copy link
Contributor Author

think having good diagnostics and automatic quick fixes which goes in both directions should be a starting point. If we see that it does not help we can be more noisy. ...

Agree with this. I will try adding local inspection and quick fix intention on .gitpod.yml

@yaohui-wyh yaohui-wyh marked this pull request as draft June 29, 2022 15:44
@yaohui-wyh
Copy link
Contributor Author

yaohui-wyh commented Jul 1, 2022

Thanks @akosyakov for the review, I will open a new PR that implements inspections & quickfixes. I will keep this one opened as Draft for a while, in case some discussions need context 🙏

@stale
Copy link

stale bot commented Jul 12, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jul 12, 2022
@yaohui-wyh yaohui-wyh closed this Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress meta: stale This issue/PR is stale and will be closed soon release-note size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[jb] add a warning if runtime Xmx is different from configured
3 participants