`upload-recording` command should assume datasource URL

[andrewazores]

Currently, the upload-recording command expects to be provided three arguments: targetId, recordingName, and datasourceUrl. The datasourceUrl should not be provided by the client. Instead, container-jfr should simply assume that the URL to upload the recording to is the configured datasource URL in its environment variables (ie the same one reported by the WebServer on GET /api/v1/grafana_datasource_url).

[vic-ma]

I'm trying to run the current version of the command.

I was able to get Grafana + jfr-datasource working with a local JFR file, but I can't seem to get this command to work to upload a recording from Container JFR. It fails to connect:

> upload-recording localhost foo http://localhost:8080

"upload-recording" "[localhost, foo, http://localhost:8080]"
[TRACE] Locking connection localhost:9091
[TRACE] Unlocking connection localhost:9091
[TRACE] Locking connection localhost:9091
[TRACE] Unlocking connection localhost:9091
upload-recording localhost foo http://localhost:8080 operation failed due to Connect \
to localhost:8080 [localhost/127.0.0.1, localhost/0:0:0:0:0:0:0:1] failed: \
Connection refused (Connection refused)
    at org.apache.http.impl.conn.DefaultHttpClientConnectionOperator.connect(DefaultHttpClientConnectionOperator.java:156)
...

I believe I've set the proper environment variable:

[vma@victor-work container-jfr]$ env | grep CONTAINER
CONTAINER_JFR_ALLOW_UNTRUSTED_SSL=1

[andrewazores]

That doesn't look like an SSL issue necessarily, rather that the jfr-datasource process/service isn't listening at the URL you specified. How are you running jfr-datasource and Grafana? The easiest way is probably to set up CodeReady Containers and the ContainerJFR Operator, which will automate setting up Grafana and the datasource, and a ContainerJFR instance configured to use them. This is a much slower workflow for testing your changes however, since it means any changes you make to ContainerJFR will need to be built into a new OCI image, published somewhere (typically quay.io), and then redeployed into the CRC cluster.

[vic-ma]

I ran Grafana with sudo systemctl start grafana-server, and for jfr-datasource I just ran the jar. I'll try doing the method you outlined.

[andrewazores]

When you ran the jfr-datasource JAR, you mean you just did java -jar jfr-datasource.jar on the built JAR on your local disk? That should work, and run it as a local JVM process on your machine. If you ran ContainerJFR using something like mvn prepare-package exec:java then that should put them in the same network subnet and I'd expect the upload connection to probably work, but if you used run.sh or something else with podman then it likely wouldn't (because to the ContainerJFR instance in a container, there is no other process listening on the port you specified).

[vic-ma]

Oh, yes I ran the jar with java -jar ./server/target/server-1.0.0-SNAPSHOT-runner.jar but I ran Container JFR with the run script, so I guess that was the problem. I'll try doing the exec:java command.

[vic-ma]

Using mvn prepare-package exec:java, I can't get the dump command to work. The ping command does work. Also the dump command works when I connect to CJFR using $ sh run.sh, also using websocat.

Here is the output when I try to do the dump command on a cjfr instance I started with mvn prepare-package exec:java; I also did mvn clean package before that:

[vma@victor-work ~]$ websocat ws://localhost:8181/api/v1/command
{command:ping}
{"id":null,"commandName":"ping","status":0,"payload":null}
{command:dump,args:[localhost,foo,1,"template=Continuous"]}
{"id":null,"commandName":"dump","status":-2,"payload": \
"WrappedConnectionException: Failed to retrieve RMIServer stub: \
javax.naming.ServiceUnavailableException [Root exception is \
java.rmi.ConnectException: Connection refused to host: localhost; \
nested exception is: \n\tjava.net.ConnectException: \
Connection refused (Connection refused)]"}

Is there something I can try here? Or maybe I should just try the cjfr Operator method?

[andrewazores]

If you're running it as a local JVM process with exec:java then your ContainerJFR process probably isn't set up to listen for incoming JMX connections by default, so you need to do that as well. Check the README, look for the MAVEN_OPTS env var you should set to enable this functionality.

It works without any configuration when you use run.sh because those required arguments are already set in the container when it is built. This happens in the pom.xml by the Jib plugin when you do mvn package.

[vic-ma]

Okay, thanks, it's working now.

[vic-ma]

When I run upload-command I now get an IOException saying the client has been closed (newlines added for readability):

[vma@victor-work container-jfr]$ websocat ws://localhost:8181/api/v1/command

{command:dump,args:[localhost,foo,1,"template=Continuous"]}

{"id":null,"commandName":"dump","status":0,"payload":null}

{command:upload-recording,args:[localhost,foo,"http://localhost:8080"]}

{"id":null,"commandName":"upload-recording","status":-2,"payload":"IOException: \
The client has been closed."}

jfr-datasource outputs this message:

2020-06-03 16:20:14,342 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] \
(vert.x-eventloop-thread-4) HTTP Request to / failed, error id: \
75e60610-1354-4cd9-b4c2-5a666179dd27-1: \
 io.vertx.core.VertxException: Connection was closed

As a sanity check, I tried uploading the local JFR file again, and that still works.

[andrewazores]

Is the API endpoint for the POST request just http://localhost:8080? I thought there was some URL path there. Although that might not be the root cause - that exception sounds like an odd one to be thrown if that's the problem. If you verify that you have the correct URL for the datasource upload, then perhaps there's a new bug breaking the upload functionality that hasn't been spotted before. If so then I would suspect the recent connectionless API changes, since those are the only thing that has changed anything HTTP-related lately, but I'm not sure how that would have an affect on outgoing connections from ContainerJFR to the datasource.

[andrewazores]

https://github.com/rh-jmc-team/jfr-datasource/blob/master/README.md

I think it should be {command:upload-recording,args:[localhost,foo,"http://localhost:8080/load"]}

[vic-ma]

Looks like it gives the same error. I also tried /upload.

[vic-ma]

I just tried with the commit before connectionless and it seems to work:

{"id":null,"commandName":"upload-recording","status":0,"payload": \
{"body":"Uploaded: foo\nSet: foo\n","status":{"protoVersion": \
{"protocol":"HTTP","major":1,"minor":1},"statusCode":200,"reasonPhrase":"OK"}}}

2020-06-03 16:48:46,941 INFO  [com.red.jfr.eve.RecordingService] (vert.x-eventloop-thread-2) Uploaded: foo
2020-06-03 16:48:46,943 INFO  [com.red.jfr.eve.RecordingService] (vert.x-eventloop-thread-2) Loading file: /home/vma/jfr-datasource/file-uploads/foo

[andrewazores]

Nice, I was just typing out a comment that that is probably the problem area. I haven't tested the command out the way you are manually now in quite some time, and I may have missed the Grafana upload functionality when I tried testing things in an updated image in an Operator deployment.

Okay, so there's a new bug to file then. Feel free to tackle this one as well if you'd like, otherwise I'll take a look at it.

[andrewazores]

I don't think I changed anything about the HttpClient and its POST request in the connectionless API changes, so it's more likely something to do with the TargetConnectionManager interaction with the command. We're trying to open a connection to the remote target JVM and then stream data from it to ContainerJFR, and then from there to the datasource. It sounds like the connection to the target is being closed prematurely given the exception you hit earlier.

[andrewazores]

Just read it over and yea, I'm pretty confident the issue is with how doPost in the UploadRecordingCommand is implemented.

[andrewazores]

Also, this highlights that uploading to the jfr-datasource should be included in our integration tests, so that would be good to include on top of the rest of #164. That will take a bit of extra setup since those tests are currently only configured to run the container-jfr image alone.

[vic-ma]

Do you want me to file the issue or you? I think fixing it may be out of my depth for now though haha. Do you think I should continue working on upload-recording and just try using Operator? Or wait for the bug to be fixed and work on a different issue until then?

[vic-ma]

It would probably be good to learn the Operator anyway I guess?

[andrewazores]

If it's broken due to the connectionless API changes then it will also be broken in the Operator once that's updated with a more recent ContainerJFR image version, so it would need to be fixed upstream here either way.

I can help walk you through fixing this upload bug - it isn't actually that complex, and there is another example of a connection being consumed in the appropriate way (I think) elsewhere in the codebase already that can be used as a template for how to go about this fix properly.

[vic-ma]

Okay, sounds good!

[vic-ma]

I was just wondering, how come we use Environment as an instance, versus just having all its methods be static?

[andrewazores]

If you dependency-inject an instance into the constructor, your unit tests can also just call that same constructor while supplying a mock. Not so clean and easy with static methods.

[andrewazores]

Just to help illustrate the point - Environment exists simply as a wrapper around System.getenv(), basically. If we look at a piece of code that uses Environment, let's say the new HealthGetHandler, we could easily swap out its env calls for System.getenv() with few real modifications. But then how would HealthGetHandlerTest unit test how the handler behaves when under different external environment states, when the handler directly calls into a real System static method? The test would end up reading the actual environment variables inherited by the JVM process under test, so it wouldn't be possible to change what the values are at runtime, and worse, the values could (and would) differ between different machines running the tests.

There are testing libraries that are able to mock static methods, like PowerMockito, but I prefer to lean on dependency injection and have consistent dependencies on instances rather than have some instance dependencies and some static dependencies, plus an extra library to deal with testing just the static ones.

[vic-ma]

I see, thanks for the explanation!

[vic-ma]

I've got the change working, but just without DI for Environment, which I'm trying to figure out how to do now.

My understanding from reading the Dagger docs is that I should be able to just add Environment env into the constructor's parameter list, since it already has an @Inject annotation. But when I compile this, I get a bunch of errors. So I'm thinking there's something else I need to do, like maybe an import or some Dagger-related configuration to allow Environment to be injected in this class. But I also see that the constructor already has a FileSystem parameter, whose provider exists in the same file as the Environment provider (SystemModule.java); so I would think that any additional configuration for this class needed to inject Environment would already have been done?

Though the errors are about mismatched parameters and arguments, so maybe there is some place that declares the parameter list for the UploadRecordingCommand constructor, where I need to add in Environment env?

[andrewazores]

I'm not sure what the problem is exactly, it sounds like you're doing the right thing. There shouldn't be any additional steps required than simply adding it to the constructor params. I just tested it by making this tiny diff:

diff --git a/src/main/java/com/redhat/rhjmc/containerjfr/commands/internal/UploadRecordingCommand.java b/src/main/java/com/redhat/rhjmc/containerjfr/commands/internal/UploadRecordingCommand.java
index 1e45af9..096c829 100644
--- a/src/main/java/com/redhat/rhjmc/containerjfr/commands/internal/UploadRecordingCommand.java
+++ b/src/main/java/com/redhat/rhjmc/containerjfr/commands/internal/UploadRecordingCommand.java
@@ -65,6 +65,7 @@ import org.openjdk.jmc.rjmx.services.jfr.IRecordingDescriptor;
 import com.redhat.rhjmc.containerjfr.MainModule;
 import com.redhat.rhjmc.containerjfr.commands.SerializableCommand;
 import com.redhat.rhjmc.containerjfr.core.net.JFRConnection;
+import com.redhat.rhjmc.containerjfr.core.sys.Environment;
 import com.redhat.rhjmc.containerjfr.core.sys.FileSystem;
 import com.redhat.rhjmc.containerjfr.core.tui.ClientWriter;
 import com.redhat.rhjmc.containerjfr.net.TargetConnectionManager;
@@ -83,6 +84,7 @@ class UploadRecordingCommand extends AbstractConnectedCommand implements Seriali
             ClientWriter cw,
             TargetConnectionManager targetConnectionManager,
             FileSystem fs,
+            Environment env,
             @Named(MainModule.RECORDINGS_PATH) Path recordingsPath,
             Provider<CloseableHttpClient> httpClientProvider) {
         super(targetConnectionManager);
diff --git a/src/test/java/com/redhat/rhjmc/containerjfr/commands/internal/UploadRecordingCommandTest.java b/src/test/java/com/redhat/rhjmc/containerjfr/commands/internal/UploadRecordingCommandTest.java
index 0ebba10..8830203 100644
--- a/src/test/java/com/redhat/rhjmc/containerjfr/commands/internal/UploadRecordingCommandTest.java
+++ b/src/test/java/com/redhat/rhjmc/containerjfr/commands/internal/UploadRecordingCommandTest.java
@@ -77,6 +77,7 @@ import com.redhat.rhjmc.containerjfr.commands.SerializableCommand.MapOutput;
 import com.redhat.rhjmc.containerjfr.commands.SerializableCommand.Output;
 import com.redhat.rhjmc.containerjfr.commands.internal.UploadRecordingCommand.RecordingNotFoundException;
 import com.redhat.rhjmc.containerjfr.core.net.JFRConnection;
+import com.redhat.rhjmc.containerjfr.core.sys.Environment;
 import com.redhat.rhjmc.containerjfr.core.sys.FileSystem;
 import com.redhat.rhjmc.containerjfr.core.tui.ClientWriter;
 import com.redhat.rhjmc.containerjfr.net.TargetConnectionManager;
@@ -92,6 +93,7 @@ class UploadRecordingCommandTest implements ValidatesTargetId, ValidatesRecordin
     @Mock ClientWriter cw;
     @Mock TargetConnectionManager targetConnectionManager;
     @Mock FileSystem fs;
+    @Mock Environment env;
     @Mock Path path;
     @Mock CloseableHttpClient httpClient;
     @Mock JFRConnection conn;
@@ -109,7 +111,8 @@ class UploadRecordingCommandTest implements ValidatesTargetId, ValidatesRecordin
     @BeforeEach
     void setup() {
         this.command =
-                new UploadRecordingCommand(cw, targetConnectionManager, fs, path, () -> httpClient);
+                new UploadRecordingCommand(
+                        cw, targetConnectionManager, fs, env, path, () -> httpClient);
     }

     @Test

and my build works.

Can you open a draft PR with your changes, even if the build is broken in the current state, so that I can take a look at it?

[vic-ma]

If UploadRecordingCommandTest implements ValidatesRecordingName, then that should handle all the testing for the validate() method, regarding whether the recording name is correct or not, right? I see that we also have shouldValidateRecordingNames() and shouldNotValidateInvalidRecordingNames() inside UploadRecordingCommandTest itself; do those do something different?

[andrewazores]

Yea, the "mix-in" ValidatesRecordingName and other similar interfaces (those are abuses of the Java 8 type system for functional interfaces and default methods, don't use those tricks in actual implementations - I just did it as a hack for testing convenience) handle various validations for similar commands. It seems like those two other methods you linked are duplicating the same testing, but they were there first and were just never removed when those interfaces were added.

[vic-ma]

(those are abuses of the Java 8 type system for functional interfaces and default methods, don't use those tricks in actual implementations - I just did it as a hack for testing convenience)

Ah I was wondering why I couldn't find anything about that technique online hah. It certainly does feel awfully convenient.

It seems like those two other methods you linked are duplicating the same testing, but they were there first and were just never removed when those interfaces were added.

In that case do you want me to remove them in this PR?

[andrewazores]

Yea, may as well remove them. It's cleanup in a closely related area, even if it isn't directly related to the PR. I'm fine with doing a bit of cleanup here and there in PRs like that. Forcing a new PR to be made just for the cleanup steps is a barrier and will just deter people from doing cleanup, and that's A Bad Thing (TM). Anything very significant, particularly refactoring more than is really required to implement a bugfix or new feature in the original issue, should probably be in a new PR, but where exactly the line lies is pretty subjective.

[andrewazores]

@vic-ma I think this can be closed now, no?

[vic-ma]

Ah yes.