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

feat(hotreload): create configuration for hot-reload dev server #745

Merged
merged 5 commits into from
Nov 5, 2021

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Nov 3, 2021

Fixes #744

To test:

  1. In first terminal, sh run-local.sh. If you use mvnd or something else like I do and have this aliased as mvn in your PATH, then you may wish to do MVN=/usr/bin/mvn sh run-local.sh instead.
  2. (Optional) In second terminal, cd web-client && yarn yarn:frzinstall && yarn start:dev

This should start a hot-reloading Cryostat backend as a local JVM process in the first terminal, and the typical existing hot-reloading Cryostat web-client in the second terminal. There will be no sample app targets and there will be no Grafana dashboard, but the Cryostat JVM should discover itself via JDP and you should be able to query it for its active recordings, event types, and event templates, as well as start and archive recordings.

Report generation doesn't currently work with this setup, so I'm looking into that. It seems like it's probably a hostname resolution issue.
Turns out the report generation issue is actually a classpath problem - the main process isn't able to load the subprocess class to invoke:

Error: Could not find or load main class io.cryostat.net.reports.SubprocessReportGenerator
Caused by: java.lang.ClassNotFoundException: io.cryostat.net.reports.SubprocessReportGenerator
Nov. 03, 2021 3:31:35 P.M. io.cryostat.core.log.Logger error
SEVERE: Exception thrown
io.cryostat.net.reports.SubprocessReportGenerator$ReportGenerationException: [1] Connection to target JVM failed.
...

This has two parts. First, the new vertx-maven-plugin configuration added doesn't have any easy way that I can see to simply set/append to the classpath when using vertx:run. It may be cleared up by using vertx:start (https://reactiverse.io/vertx-maven-plugin/#vertx:start) instead. Second, the way we invoke the subprocess uses a hardcoded classpath (https://github.com/cryostatio/cryostat/blob/460e2fd78646965cc0759b6431526e5b2c1cac73/src/main/java/io/cryostat/util/JavaProcess.java#L60), which is very unlikely to actually match the filesystem on a development host machine. We would need to make this configurable by an env var or system property so that it can be set to point to the maven build target directory contents.

@andrewazores andrewazores added the feat New feature or request label Nov 3, 2021
@andrewazores
Copy link
Member Author

andrewazores commented Nov 3, 2021

Done tinkering with this now. The report generation classpath issue will require some additional thought and experimentation, but for now this achieves the initial goal I had intended already for other features.

Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

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

If I open a new window in VSCode with the cryostat/ folder while the run-local backend and frontend are running, VSCode will clean and build the project automatically, causing the backend to throw an exception and hang. How can I prevent this from happening?

VSCode output:

1df5dfef Importing Maven project(s) [Done]
5afe19df Repository registry initialization [Done]
21c40e1e Clean workspace... [Done]
71187c59 Building [Done]
83961158 Validate documents [Done]
c66ede8d Building [Done]
7f744c7e Publish Diagnostics [Done]
70889b8a Building [Done]
7ffcc57e Building [Done]

Backend output:

INFO: Removing cached connection for service:jmx:rmi:///jndi/rmi://localhost.localdomain:9091/jmxrmi
Nov. 03, 2021 3:44:50 P.M. io.vertx.core.impl.launcher.commands.Watcher
INFO: Redeploying!
Stopping vert.x application 'bd2dd646-8ec9-496b-a5ed-c862a767af48-redeploy'
Application 'bd2dd646-8ec9-496b-a5ed-c862a767af48-redeploy' terminated with status 0
Starting vert.x application...
bd2dd646-8ec9-496b-a5ed-c862a767af48-redeploy
Nov. 03, 2021 3:44:50 P.M. io.vertx.core.impl.launcher.commands.Watcher
INFO: Redeployment done in 137 ms.
Error: Could not find or load main class io.cryostat.Cryostat
Caused by: java.lang.ClassNotFoundException: io.cryostat.Cryostat

@andrewazores
Copy link
Member Author

Ah, I see. That makes sense but it's unfortunate. I don't know what you could do to prevent that, other than just configure VSCode not to do a clean when it starts, but I don't know how to do that.

You might be able to do something like add a "Run Configuration" which most IDEs have, and either configure it to do the same thing as run-local.sh does or perhaps simply just to invoke sh run-local.sh. This would let you launch the backend devserver from within the IDE rather than doing it on the cli with Maven directly. Hopefully this way the IDE won't interfere and decide to clean the build output while it knows that it's supposed to be running the project.

@jan-law
Copy link
Contributor

jan-law commented Nov 4, 2021

Thanks, I found a way to start the devserver by creating the file .vscode/tasks.json and enabling Allow Automatic Tasks in Folder (source):

{
    "version": "2.0.0",
    "tasks": [
      {
        "label": "Launch devserver",
        "type": "shell",
        "command": "sh devserver.sh",
        "group": "none",
        "presentation": {
          "reveal": "always",
          "panel": "new"
        },
        "runOptions": {
          "runOn": "folderOpen"
        }
      }
    ]
  }

Update: you can also change the runOptions to "runOn": "default". Then you can start and stop the devserver with ctrl-shift-p Tasks: Run Task and Task: Terminate Task

@andrewazores
Copy link
Member Author

Neat. Although, the devserver will try to bind on the same ports as run.sh would - so if you have your IDE set up to automatically start the devserver then you'll need a way to stop and restart that separately so that you can still use run.sh/smoketest.sh at some point. Either that or we just adjust the devserver port numbers so that it can run side-by-side with the Podman container.

@hareetd
Copy link
Contributor

hareetd commented Nov 4, 2021

image

To confirm, is this what I should be seeing after running sh devserver.sh and making some changes?

Also, since devserver.sh now does a minimal build upon compilation, this rules out any use of the optional, hot-reloading frontend as originally suggested in step 2. of the PR body, right?

@andrewazores
Copy link
Member Author

Hmm. That AccessDeniedException definitely isn't expected. The compilation error with all the missing types isn't, either.

My hunch with the AccessDeniedException is SELinux, maybe. I'll dig into that further. The missing types have me stumped however.

Also, since devserver.sh now does a minimal build upon compilation, this rules out any use of the optional, hot-reloading frontend as originally suggested in step 2. of the PR body, right?

No, you can still do step 2 and run a separate frontend devserver. The minimal build just prevents Maven from building the web-client submodule to generate its assets, but the backend devserver doesn't know where those are located anyway (this is very closely related to the classpath problem I explained above) so even if you do build those assets, the backend devserver wouldn't serve them to you when you visit http://localhost:8080 . Now that it does a minimal build it skips over building those non-served assets so the backend devserver startup time is faster. If you want to have a web-client UI to use with the backend devserver then, at least for now, you will need to do step 2 and launch a separate frontend devserver as well, building and serving the frontend assets on its own.

@andrewazores
Copy link
Member Author

Doesn't seem to me like SELinux is behind the permissions issue. @hareetd , what does ls -l show for the owner/permissions on your archive, conf, certs, clientlib, templates, and truststore?

I'll push a commit in a few minutes that sets all of those up to use temporary directories instead of ones within the repo, too. That might be a good idea in general.

@hareetd
Copy link
Contributor

hareetd commented Nov 5, 2021

image

Looks like credentials has a different owner than expected. The user account I'm currently logged into (hdhillon) is one with root privileges that I created myself, and not the account that IT provided.

It seems the hdhillon group has no file permissions for credentials which explains why my user hdhillon is denied access. I don't know which user 165720 is though but I guess it has to be the original IT account?

@andrewazores
Copy link
Member Author

I have something similar on my computer, and I don't have any other user account on here. I think that other UID is probably coming from when we run the run.sh/smoketest.sh setup with Podman. When the Cryostat process is running inside there it creates the contents of conf and archive, which matches the ones that have different permissions.

You could try chowning those directories to set the owner back to your account, but it might just go back to being owned by the other UID the next time you use run.sh.

The last commit I pushed that sets the directories to new ones within /tmp should avoid this problem for you, since the devserver will be using different directories than the ones we use as mountpoints for run.sh.

Try that out and let me know if you still run into the missing types issue as well - that one I'm really confused about.

@hareetd
Copy link
Contributor

hareetd commented Nov 5, 2021

image

Okay, so the missing types are no longer an issue (I'm thinking they were caused by Cryostat.java not fully executing due to the missing file permissions from before) but now I get an NPE when launching the hot-reloading frontend. However, this is the only issue; recompilation upon making changes to the backend is working as expected.

@andrewazores
Copy link
Member Author

Yea, that NPE is normal/expected right now. The git version info is stored in the file target/assets/app/resources/io/cryostat/version during the maven build, and that normally gets included in the built image and is available on the classpath for loading so that it can be reported in GET /health. Since this devserver doesn't have a working assets/classpath setup at the moment, this feature is also broken. If you do GET /health you'll get 'unknown' for the version.

Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

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

The new changes work as expected for me

@andrewazores andrewazores merged commit bb7840b into cryostatio:main Nov 5, 2021
@andrewazores andrewazores deleted the hot-reload branch November 5, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run-as-local-process and hot-reload development server
3 participants