fix(serialization): recording serialization properly converts recording state types #314

andrewazores · 2023-12-04T18:43:39Z

Fixes #313

andrewazores · 2023-12-04T19:24:27Z

 [INFO] Scanning for projects...
[INFO] 
[INFO] ------------------------< io.cryostat:cryostat >------------------------
[INFO] Building cryostat 2.5.0-SNAPSHOT
[INFO]   from pom.xml
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] Downloading from github: https://maven.pkg.github.com/cryostatio/cryostat-core/io/cryostat/cryostat-core/2.26.0-SNAPSHOT/maven-metadata.xml
[INFO] Downloading from github: https://maven.pkg.github.com/cryostatio/cryostat-core/io/cryostat/cryostat-core/2.26.0-SNAPSHOT/cryostat-core-2.26.0-SNAPSHOT.pom
Warning:  The POM for io.cryostat:cryostat-core:jar:2.26.0-SNAPSHOT is missing, no dependency information available
[INFO] Downloading from github: https://maven.pkg.github.com/cryostatio/cryostat-core/io/cryostat/cryostat-core/2.26.0-SNAPSHOT/cryostat-core-2.26.0-SNAPSHOT.jar
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.642 s
[INFO] Finished at: 2023-12-04T18:45:26Z
[INFO] ------------------------------------------------------------------------
Error:  Failed to execute goal on project cryostat: Could not resolve dependencies for project io.cryostat:cryostat:jar:2.5.0-SNAPSHOT: Could not find artifact io.cryostat:cryostat-core:jar:2.26.0-SNAPSHOT in github (https://maven.pkg.github.com/cryostatio/cryostat-core) -> [Help 1]
Error:  
Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.
Error:  Re-run Maven using the -X switch to enable full debug logging.
Error:  
Error:  For more information about the errors and possible solutions, please read the following articles:
Error:  [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/DependencyResolutionException
Error: Process completed with exit code 1.

CI failure looks like the builder is trying to use 2.26.0-SNAPSHOT for -core because that's the version in the pom.xml in this PR (since that's what is on main), but that is not a published version since we only publish actual releases, not snapshots.

I think the CI builder needs to build -core to test it, then upload that artifact. The "build Cryostat" task should then download that artifact again and re-install it into the local .m2 so that the Cryostat image build can find it and use it, so that the Cryostat build and tests use a -core artifact that is derived from the PR being tested.

mwangggg · 2023-12-04T19:54:21Z

cryostat-core/src/main/java/org/openjdk/jmc/rjmx/services/jfr/internal/RecordingDescriptorV2.java

Lines 91 to 104 in e77984d

    
           private static RecordingState decideState(String state) { 
        
           	if ("NEW".equals(state)) { 
        
           		return RecordingState.CREATED; 
        
           	} else if ("RUNNING".equals(state) || "DELAYED".equals(state) || "STARTING".equals(state)) { 
        
           		return RecordingState.RUNNING; 
        
           	} else if ("STOPPED".equals(state)) { 
        
           		return RecordingState.STOPPED; 
        
           	} else if ("STOPPING".equals(state)) { 
        
           		return RecordingState.STOPPING; 
        
           	} else { 
        
           		// FIXME: CLOSED would fit better but there is no such enum value at the moment 
        
           		return RecordingState.STOPPED; 
        
           	} 
        
           }

I was taking a look at some other places where the recording states are mentioned and I was just wondering where these states come from? I know they are composite data from the recordings, but they aren't entirely the jdk.jfr.recording or the RecordingStates sets?

mwangggg · 2023-12-04T20:27:09Z

I think the CI builder needs to build -core to test it, then upload that artifact. The "build Cryostat" task should then download that artifact again and re-install it into the local .m2 so that the Cryostat image build can find it and use it, so that the Cryostat build and tests use a -core artifact that is derived from the PR being tested.

This is what I intended when I first implemented the Cryostat build testing, but I think there's an error caching or retrieving the cached files. I'll take a look at it right now.

andrewazores · 2023-12-04T20:28:08Z

Basically I painted us into a corner a long time ago. We probably want to move in the direction of just using the jdk.jfr classes to model our data as much as possible, but a lot of our stack is built on top of JMC and its org.openjdk.jmc.rjmx.services.jfr classes, which are roughly equivalent but do have some differences.

One nice thing about the JMC stuff is that it provides nicer type signatures in many cases that make it clearer/less error-prone what you're doing - though sometimes this is a little boilerplate-y.

On the downside, those classes and the various Unit types don't always serialize well, and there is at least this one case where the state of a recording is modelled differently between the two packages. I don't know if that recording state difference is because the JMC side of it is more geared toward JMC's UI display, or if perhaps it's an older state model that looks more like the old V1 proprietary JFR implementation rather than the V2 OpenJDK JFR implementation. Regardless of the reason for the difference, we would probably be better off migrating away from using JMC's abstractions except in cases where we really want/need it, like for Automated Analysis. But that will be a pretty big undertaking to refactor all of that and test for any regressions (in serialization especially), and it's just as cleanup/tech-debt chore rather than feature work, so it hasn't been very high priority.

mwangggg

LGTM

andrewazores · 2023-12-04T20:49:35Z

https://github.com/cryostatio/cryostat-core/blob/main/src/main/java/org/openjdk/jmc/rjmx/services/jfr/IRecordingDescriptor.java

cryostat-core/src/main/java/org/openjdk/jmc/rjmx/services/jfr/internal/RecordingDescriptorV1.java

Line 101 in e77984d

    
           private RecordingState decideState(boolean isStarted, boolean isStopped, boolean isRunning) {

cryostat-core/src/main/java/org/openjdk/jmc/rjmx/services/jfr/internal/RecordingDescriptorV2.java

Line 91 in e77984d

private static RecordingState decideState(String state) {

Looks to me like the JMC RecordingState model is indeed modelled after the old V1 JFR implementation, and they just mapped the OpenJDK JFR V2 states onto the existing model, which we've inherited from JMC.

aptmac · 2023-12-04T20:53:49Z

Looks to me like the JMC RecordingState model is indeed modelled after the old V1 JFR implementation, and they just mapped the OpenJDK JFR V2 states onto the existing model, which we've inherited from JMC.

IRecordingDescriptor and RecordingDescriptorV2 haven't been updated since JMC was open-sourced. Perhaps it's time to update the RecordingState enum and touch-up RecordingDescriptorV2 at the same time.

In this case it would also be nice if RecordingServiceV2.decideState() publicly accessible..

Edit: I've opened: https://bugs.openjdk.org/browse/JMC-8153

…ng state types

github-actions · 2023-12-05T19:17:45Z

ARCH	IMAGE
amd64	ghcr.io/cryostatio/cryostat-core:pr-314-06b246d57a7e8fb4f84e222da093a32b927f4e27-linux-amd64
arm64	ghcr.io/cryostatio/cryostat-core:pr-314-06b246d57a7e8fb4f84e222da093a32b927f4e27-linux-arm64

To run smoketest:

# amd64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-314-06b246d57a7e8fb4f84e222da093a32b927f4e27-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-core:pr-314-06b246d57a7e8fb4f84e222da093a32b927f4e27-linux-arm64 sh smoketest.sh

andrewazores · 2023-12-05T19:18:02Z

/build_test

andrewazores · 2023-12-05T19:20:47Z

https://github.com/cryostatio/cryostat-core/actions/runs/7105513277

github-actions · 2023-12-05T19:31:14Z

Cryostat Test: All tests pass ✅
https://github.com/cryostatio/cryostat-core/actions/runs/7105513277

…ng state types (#314) (cherry picked from commit 96a7f8c)

aali309 · 2023-12-05T19:38:20Z

Did you see this @andrewazores

Error: cannot create directory ‘/home/runner/work/cryostat-core/cryostat-core/target/[ERROR] Failed to execute goal org.codehaus.mojo:build-helper-maven-plugin:3.5.0:regex-property (image-tag-to-lower) on project cryostat: Execution image-tag-to-lower of goal org.codehaus.mojo:build-helper-maven-plugin:3.5.0:regex-property failed: Plugin org.codehaus.mojo:build-helper-maven-plugin:3.5.0 or one of its dependencies could not be resolved: Cannot access central (https:’: File name too long
Error: d-integration-tests.bash: line 95: /home/runner/work/cryostat-core/cryostat-core/target/[ERROR] Failed to execute goal org.codehaus.mojo:build-helper-maven-plugin:3.5.0:regex-property (image-tag-to-lower) on project cryostat: Execution image-tag-to-lower of goal org.codehaus.mojo:build-helper-maven-plugin:3.5.0:regex-property failed: Plugin org.codehaus.mojo:build-helper-maven-plugin:3.5.0 or one of its dependencies could not be resolved: Cannot access central (https://repo.maven.apache.org/maven2) in offline mode and the artifact org.apache-extras.beanshell:bsh:jar:2.0b6 has not been downloaded from it before. -> [Help 1]
Error:  
Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.
Error:  Re-run Maven using the -X switch to enable full debug logging.
Error:  
Error:  For more information about the errors and possible solutions, please read the following articles:
Error:  [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginResolutionException-2023-12-05T19:19+00:00.client.log: File name too long
Error: d-integration-tests.bash: line 96: /home/runner/work/cryostat-core/cryostat-core/target/[ERROR] Failed to execute goal org.codehaus.mojo:build-helper-maven-plugin:3.5.0:regex-property (image-tag-to-lower) on project cryostat: Execution image-tag-to-lower of goal org.codehaus.mojo:build-helper-maven-plugin:3.5.0:regex-property failed: Plugin org.codehaus.mojo:build-helper-maven-plugin:3.5.0 or one of its dependencies could not be resolved: Cannot access central (https://repo.maven.apache.org/maven2) in offline mode and the artifact org.apache-extras.beanshell:bsh:jar:2.0b6 has not been downloaded from it before. -> [Help 1]
Error:  
Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.
Error:  Re-run Maven using the -X switch to enable full debug logging.
Error:  
Error:  For more information about the errors and possible solutions, please read the following articles:
Error:  [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginResolutionException-2023-12-05T19:19+00:00.server.log: File name too long
Error: d-integration-tests.bash: line 97: /home/runner/work/cryostat-core/cryostat-core/target/[ERROR] Failed to execute goal org.codehaus.mojo:build-helper-maven-plugin:3.5.0:regex-property (image-tag-to-lower) on project cryostat: Execution image-tag-to-lower of goal org.codehaus.mojo:build-helper-maven-plugin:3.5.0:regex-property failed: Plugin org.codehaus.mojo:build-helper-maven-plugin:3.5.0 or one of its dependencies could not be resolved: Cannot access central (https://repo.maven.apache.org/maven2) in offline mode and the artifact org.apache-extras.beanshell:bsh:jar:2.0b6 has not been downloaded from it before. -> [Help 1]
Error:  
Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.
Error:  Re-run Maven using the -X switch to enable full debug logging.
Error:  
Error:  For more information about the errors and possible solutions, please read the following articles:
Error:  [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginResolutionException-2023-12-05T19:19+00:00.client.log: File name too long```

andrewazores · 2023-12-05T19:39:28Z

No, I didn't notice that. Where is it from?

aali309 · 2023-12-05T19:41:20Z

No, I didn't notice that. Where is it from?

Cryostat Test: All tests pass ✅ https://github.com/cryostatio/cryostat-core/actions/runs/7105513277

Here..

andrewazores · 2023-12-05T19:46:01Z

Ah, I see. Odd that the workflow step succeeded despite those errors. Despite the errors it looks like it still did run the tests successfully, too...

aali309 · 2023-12-05T19:48:24Z

Yes, was wondering the same how did we get to receive the All tests pass comment.

andrewazores added bug Something isn't working backport safe-to-test labels Dec 4, 2023

andrewazores requested review from aali309 and mwangggg December 4, 2023 18:49

mwangggg mentioned this pull request Dec 4, 2023

[Bug] CI is not correctly caching the PR's -core version #315

Closed

mwangggg previously approved these changes Dec 4, 2023

View reviewed changes

andrewazores dismissed mwangggg’s stale review via 66e7df8 December 4, 2023 20:53

andrewazores force-pushed the recording-state-mismatch branch from 66e7df8 to d424c7e Compare December 4, 2023 21:55

andrewazores added 2 commits December 5, 2023 14:08

fix(serialization): recording serialization properly converts recordi…

082f4f4

…ng state types

minor fixup to align with JMC interpretations, add comment

06b246d

andrewazores force-pushed the recording-state-mismatch branch from d424c7e to 06b246d Compare December 5, 2023 19:08

andrewazores merged commit 96a7f8c into cryostatio:main Dec 5, 2023
14 checks passed

andrewazores deleted the recording-state-mismatch branch December 5, 2023 19:32

mergify bot pushed a commit that referenced this pull request Dec 5, 2023

fix(serialization): recording serialization properly converts recordi…

ec18fa3

…ng state types (#314) (cherry picked from commit 96a7f8c)

mergify bot mentioned this pull request Dec 5, 2023

fix(serialization): recording serialization properly converts recording state types (backport #314) #318

Closed

andrewazores removed the backport label Dec 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(serialization): recording serialization properly converts recording state types #314

fix(serialization): recording serialization properly converts recording state types #314

andrewazores commented Dec 4, 2023 •

edited

Loading

andrewazores commented Dec 4, 2023

mwangggg commented Dec 4, 2023

mwangggg commented Dec 4, 2023

andrewazores commented Dec 4, 2023

mwangggg left a comment

andrewazores commented Dec 4, 2023

aptmac commented Dec 4, 2023 •

edited

Loading

github-actions bot commented Dec 5, 2023

andrewazores commented Dec 5, 2023

andrewazores commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

aali309 commented Dec 5, 2023

andrewazores commented Dec 5, 2023

aali309 commented Dec 5, 2023

andrewazores commented Dec 5, 2023

aali309 commented Dec 5, 2023

fix(serialization): recording serialization properly converts recording state types #314

fix(serialization): recording serialization properly converts recording state types #314

Conversation

andrewazores commented Dec 4, 2023 • edited Loading

andrewazores commented Dec 4, 2023

mwangggg commented Dec 4, 2023

mwangggg commented Dec 4, 2023

andrewazores commented Dec 4, 2023

mwangggg left a comment

Choose a reason for hiding this comment

andrewazores commented Dec 4, 2023

aptmac commented Dec 4, 2023 • edited Loading

github-actions bot commented Dec 5, 2023

andrewazores commented Dec 5, 2023

andrewazores commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

aali309 commented Dec 5, 2023

andrewazores commented Dec 5, 2023

aali309 commented Dec 5, 2023

andrewazores commented Dec 5, 2023

aali309 commented Dec 5, 2023

andrewazores commented Dec 4, 2023 •

edited

Loading

aptmac commented Dec 4, 2023 •

edited

Loading