fix(labels): map recording labels to unique jvm id

[jan-law]

Fixes #960
Related cryostatio/cryostat-web#442
Depends on cryostatio/cryostat-core#141
Depends on cryostatio/cryostat-web#473

Stores metadata labels according to each target's (hopefully unique enough) generated JVM ID and recording name inside recordingMetadataMap. The recordingMetadataMap key format is Pair< jvmId String, recordingName String>.

The "JVM ID" is a hashcode string generated by the helper method in cryostat-core from the target JVM's runtime properties.

When Cryostat archives a recording from a target JVM, the archived recording's labels will be stored according to the source target. Labels for re-uploaded recordings are mapped to a constant id UNLABELLED_ARCHIVES_ID.

Known issues with this PR:

1. If a target restarts, the jvmId associated with that target's connect URL may be different. Cryostat should associate the archived recording metadata with the new jvmId by replacing the keys in the recordingMetadataMap. Any metadata belonging to active recordings can be removed because active recordings are lost when a target restarts. Since both active and archived recordings are stored according to the same previous jvmId string, I searched Cryostat's archives for each metadata entry's recording name to determine if the recording is an archived recording. Is there a more efficient way to handle archived vs active recording metadata for the same target?
2. The itest in RecordingMetadataIT that checks that recording metadata is handled correctly when a target restarts shows that the metadata for active recordings is removed as expected, but viewing metadata for a recording archived before the target is restarted returns an empty labels map.
3. The cryostat-core version has a SNAPSHOT tag because the a local mvn install of cryostat-core creates a snapshot image. The version will need to be updated once the -core PR gets merged

[jan-law]

@andrewazores I had some questions about handling the cryostat-defined labels template.name and template.type:

1. To use GraphQL for editing labels, I added a doPutMetadata mutator to replace the labels for selected recordings. The query fails to parse if the label keys contain periods. doPutMetadata(metadata: { labels: { template.name:"Continuous", template.type:"TARGET", newLabel:"newValue"}}) results in "Invalid Syntax : token recognition error at: '.n'. If I add double quotes around "template.name", I get "Invalid Syntax : offending token '\"[template.name](http://template.name/)\"'. Is there some way to change this label format or change the way GraphQL parses queries?

2. Requests shouldn't be able to modify the template.name and template.type labels, right? As of now, the mutator will keep the template.name and template.type labels on the recording even if a request contains an empty labels object, indicating that the user wants to remove all labels. I realized the beta handler will remove all the labels including the template labels for an equivalent query.

[andrewazores]

1. Hmmm. No, I don't think there is anything we can do to change how GraphQL parses that.
   "Invalid Syntax : offending token '\"[template.name](http://template.name/)\"' looks pretty weird - how did it end up with a markdown link-like format?
   I think this will have to work around by changing the format of the Input Object passed to that doPutMetadata mutator. Something like this might work:

doPutMetadata(metadata: {
  labels: [
    {
      "key": "template.name",
      "value": "Continuous"
    },
    {
      "key": "template.type",
      "value": "TARGET"
    },
  ]
})

You could also do something like this:

doPutMetadata(metadata: {
  labels: [
    "template.name=Continuous",
    "template.type=TARGET"
  ]
})

and then further parse those label strings to split the LHS and RHS by the = within the mutator, but I think I prefer the first method. It seems a little cleaner and less like ducktyping.

2. This is a good question. I'm inclined to say that the template.name and template.type shouldn't be given special treatment. They are applied by default as a convenience, but if the user really wants to modify them (or any other labels we might apply by default, like the one applied to recordings started by Automated Rule, or others we may add in the future), then they're free to do so. At least, from the API perspective. Perhaps in the web-client we can give some protection to those specific labels so that the user has to confirm that they really want to modify/delete them.

[jan-law]

1. Hmmm. No, I don't think there is anything we can do to change how GraphQL parses that.
   "Invalid Syntax : offending token '\"[template.name](http://template.name/)\"' looks pretty weird - how did it end up with a markdown link-like format?

Whoops, I copied the error into my notes before sending this and the browser copy-paste turned \"template.name\" into \"[template.name](http://template.name/)\". The rest of the error is the same.

I think this will have to work around by changing the format of the Input Object passed to that doPutMetadata mutator. Something like this might work:

doPutMetadata(metadata: {
  labels: [
    {
      "key": "template.name",
      "value": "Continuous"
    },
    {
      "key": "template.type",
      "value": "TARGET"
    },
  ]
})

You could also do something like this:

doPutMetadata(metadata: {
  labels: [
    "template.name=Continuous",
    "template.type=TARGET"
  ]
})

and then further parse those label strings to split the LHS and RHS by the = within the mutator, but I think I prefer the first method. It seems a little cleaner and less like ducktyping.

Agreed, I can use your first option.

2. This is a good question. I'm inclined to say that the template.name and template.type shouldn't be given special treatment. They are applied by default as a convenience, but if the user really wants to modify them (or any other labels we might apply by default, like the one applied to recordings started by Automated Rule, or others we may add in the future), then they're free to do so. At least, from the API perspective. Perhaps in the web-client we can give some protection to those specific labels so that the user has to confirm that they really want to modify/delete them.

Thanks, sounds good.

[jan-law]

For #878 I've pasted some docs for the recording metadata mutator I've added in this PR. Not sure where it should go or how it should be formatted.

RecordingMetadataMutator

Replace recording metadata for active or archived recordings. Overwrites any existing metadata for that recording. If multiple recordings are specified with the GraphQL query, the metadata for all selected recordings will be replaced.

Query

doPutMetadata(metadata: { labels: []})

Arguments

labels - An array consisting of key-value label objects. The label objects should follow the {key: "myLabelKey", value: "myValue"} format.

Examples

query {
    targetNodes(filter: { name: "service:jmx:rmi:///jndi/rmi://cryostat:9091/jmxrmi" }) {
        recordings {
            active(filter: { name: "myActiveRecording" }) {
                doPutMetadata(metadata: { labels: [{key:"app",value:"cryostat"}, {key:"template.name",value:"Profiling"},{key:"template.type",value:"TARGET"}] }) {
                    name
                    metadata {
                        labels
                    }
                }
            }
        }
    }
}

{
    "data": {
        "targetNodes": [
            {
                "recordings": {
                    "active": [
                        {
                            "doPutMetadata": {
                                "metadata": {
                                    "labels": {
                                        "app": "cryostat",
                                        "template.name": "Continuous",
                                        "template.type": "TARGET"
                                    }
                                },
                                "name": "myActiveRecording"
                            }
                        }
                    ]
                }
            }
        ]
    }
}

query {
    targetNodes(filter: { name: "service:jmx:rmi:///jndi/rmi://cryostat:9091/jmxrmi" }) {
        recordings {
            archived(filter: { name: "myArchivedRecording" }) {
                doPutMetadata(metadata: { labels: [{key:"app",value:"cryostat"}, {key:"template.name",value:"Continuous"},{key:"template.type",value:"TARGET"}] }) {
                    name
                    metadata {
                        labels
                    }
                }
            }
        }
    }
}

{
    "data": {
        "targetNodes": [
            {
                "recordings": {
                    "archived": [
                        {
                            "doPutMetadata": {
                                "metadata": {
                                    "labels": {
                                        "app": "cryostat",
                                        "template.name": "Continuous",
                                        "template.type": "TARGET"
                                    }
                                },
                                "name": "myArchivedRecording"
                            }
                        }
                    ],
                }
            }
        ]
    }
}

[andrewazores]

For #878 I've pasted some docs for the recording metadata mutator I've added in this PR. Not sure where it should go or how it should be formatted.

I think using GraphQL.md is fine. We can update that to follow a more similar format to HTTP_API.md.

[maxcao13]

About this PR, ActiveRecording metadata should be removed when cryostat is killed right, because right now in this PR it does not. I know @jan-law added a method called removeLostTargetMetadata when LOST notifications come up but I don't believe it works when cryostat is shutdown. Does cryostat have some shutdown handler?

EDIT: Thinking about it even more, since ActiveRecordings are lost, the metadata along with them should be removed too...
I think only making ArchivedRecording metadata be StoredRecordingMetadata will probably fix this since there's probably no point in storing ActiveRecording metadata.

[andrewazores]

There is shutdown handling in the main Cryostat.java class, but it isn't really set up now as an extended mechanism that you can react to the pending shutdown from other classes.

[maxcao13]

Is there any way for the ITs to react to Notifications being emitted? The current implementation uses these Notifications to change metadata around when targets are lost and found. But from what I've noticed testing in RecordingMetadataIT, things are not going how they are supposed to and I am suspecting it's because of this.

In more detail if it helps and what my problem is right now: testStaleMetadataDeletedAndArchivedMetadataPreservedWhenTargetRestarted
When we create a recording with the name TestRecording, and then kill and restart the pod, but then create a new recording with the same name, it will show up with the old metadata.

1. When the we kill the target, and restart it, we start a new recording which calls the handler for TargetRecordingPostHandler which gets the jvmId from the jvmIdMap and calls computeIfAbsent so either we gets id from the map directly or compute a new one.
2. In this case, the test does not compute a new jvmId for the restarted target which is fine... because there's a way to solve this problem -> removeLostTargetMetadata which, when targets are lost, removes non-archived Metadata in the RecordingMetadataMap. But now the real problem is that the IT does not delete it and we get the old metadata and I believe it is because it cannot detect the TargetJvmLost notifications. The functionality is completely fine when I test manually.

[github-actions]

This PR/issue depends on:

• feat(jmx): compute unique jvm id cryostat-core#141
• fix(labels): edit labels for distinct recordings with identical file names cryostat-web#473
  By Dependent Issues (🤖). Happy coding!