From fe617750c420976638fef46869ce4b7a2a755f2b Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 13:46:29 -0400 Subject: [PATCH 01/21] refactoring --- .../cryostat/agent/FlightRecorderHelper.java | 46 ++++++++----------- .../agent/triggers/TriggerEvaluator.java | 27 ++++++++--- 2 files changed, 39 insertions(+), 34 deletions(-) diff --git a/src/main/java/io/cryostat/agent/FlightRecorderHelper.java b/src/main/java/io/cryostat/agent/FlightRecorderHelper.java index b12a4566..a8b6f7cf 100644 --- a/src/main/java/io/cryostat/agent/FlightRecorderHelper.java +++ b/src/main/java/io/cryostat/agent/FlightRecorderHelper.java @@ -15,56 +15,46 @@ */ package io.cryostat.agent; -import java.lang.management.ManagementFactory; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import jdk.jfr.Configuration; import jdk.jfr.FlightRecorder; import jdk.jfr.Recording; -import jdk.management.jfr.ConfigurationInfo; -import jdk.management.jfr.FlightRecorderMXBean; import org.slf4j.Logger; import org.slf4j.LoggerFactory; public class FlightRecorderHelper { - private final FlightRecorderMXBean bean = - ManagementFactory.getPlatformMXBean(FlightRecorderMXBean.class); private final Logger log = LoggerFactory.getLogger(getClass()); - // FIXME this is repeated logic shared with Harvester startRecording - public void startRecording(String templateNameOrLabel) { - getTemplate(templateNameOrLabel) - .ifPresentOrElse( - c -> { - long recordingId = bean.newRecording(); - bean.setPredefinedConfiguration(recordingId, c.getName()); - String recoringName = - String.format("cryostat-smart-trigger-%d", recordingId); - bean.setRecordingOptions( - recordingId, Map.of("name", recoringName, "disk", "true")); - bean.startRecording(recordingId); - log.info( - "Started recording \"{}\" using template \"{}\"", - recoringName, - templateNameOrLabel); - }, - () -> - log.error( - "Cannot start recording with template named or labelled {}", - templateNameOrLabel)); + public Optional createRecording(String templateNameOrLabel) { + Optional opt = getTemplate(templateNameOrLabel); + if (opt.isEmpty()) { + log.error( + "Cannot start recording with template named or labelled {}", + templateNameOrLabel); + return Optional.empty(); + } + Configuration configuration = opt.get(); + Recording recording = new Recording(configuration.getSettings()); + recording.setToDisk(true); + return Optional.of(recording); } - public Optional getTemplate(String nameOrLabel) { - return bean.getConfigurations().stream() + public Optional getTemplate(String nameOrLabel) { + Objects.requireNonNull(nameOrLabel); + return Configuration.getConfigurations().stream() .filter(c -> c.getName().equals(nameOrLabel) || c.getLabel().equals(nameOrLabel)) .findFirst(); } public boolean isValidTemplate(String nameOrLabel) { + Objects.requireNonNull(nameOrLabel); return getTemplate(nameOrLabel).isPresent(); } diff --git a/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java b/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java index c2aad7ba..75f841c8 100644 --- a/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java +++ b/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java @@ -52,11 +52,11 @@ public class TriggerEvaluator { public TriggerEvaluator( ScheduledExecutorService scheduler, TriggerParser parser, - FlightRecorderHelper flightRecorderModule, + FlightRecorderHelper flightRecorderHelper, long evaluationPeriodMs) { this.scheduler = scheduler; this.parser = parser; - this.flightRecorderHelper = flightRecorderModule; + this.flightRecorderHelper = flightRecorderHelper; this.evaluationPeriodMs = evaluationPeriodMs; } @@ -109,8 +109,7 @@ private void evaluate() { // met once if (t.isSimple() && evaluateTriggerConstraint(t, t.getTargetDuration())) { log.trace("Trigger {} satisfied, starting recording...", t); - flightRecorderHelper.startRecording(t.getRecordingTemplateName()); - t.setState(TriggerState.COMPLETE); + startRecording(t); } else if (!t.isSimple()) { if (evaluateTriggerConstraint(t, Duration.ZERO)) { // Condition was met, set the state accordingly @@ -128,8 +127,7 @@ private void evaluate() { // Condition was met at last check but duration hasn't passed if (evaluateTriggerConstraint(t, Duration.ofMillis(difference))) { log.trace("Trigger {} satisfied, completing...", t); - t.setState(TriggerState.COMPLETE); - flightRecorderHelper.startRecording(t.getRecordingTemplateName()); + startRecording(t); } else if (evaluateTriggerConstraint(t, Duration.ZERO)) { log.trace("Trigger {} satisfied, waiting for duration...", t); } else { @@ -153,6 +151,23 @@ private void evaluate() { } } + private void startRecording(SmartTrigger t) { + flightRecorderHelper + .createRecording(t.getRecordingTemplateName()) + .ifPresent( + recording -> { + String recordingName = + String.format("cryostat-smart-trigger-%d", recording.getId()); + recording.setName(recordingName); + recording.start(); + t.setState(TriggerState.COMPLETE); + log.info( + "Started recording \"{}\" using template \"{}\"", + recordingName, + t.getRecordingTemplateName()); + }); + } + private boolean evaluateTriggerConstraint(SmartTrigger trigger, Duration targetDuration) { try { Map scriptVars = new HashMap<>(new MBeanInfo().getSimplifiedMetrics()); From 006a5883dcad8bf7429f0ab981f7aeb4e14d05d4 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 14:03:36 -0400 Subject: [PATCH 02/21] create package --- src/main/java/io/cryostat/agent/Agent.java | 1 + .../io/cryostat/agent/CryostatClient.java | 1 + .../java/io/cryostat/agent/MainModule.java | 3 ++- .../java/io/cryostat/agent/Registration.java | 11 +++++----- .../agent/{ => harvest}/Harvester.java | 22 ++++++++++++------- .../agent/remote/RecordingsContext.java | 2 +- .../agent/triggers/TriggerParser.java | 2 +- .../agent/{ => util}/StringUtils.java | 2 +- 8 files changed, 27 insertions(+), 17 deletions(-) rename src/main/java/io/cryostat/agent/{ => harvest}/Harvester.java (96%) rename src/main/java/io/cryostat/agent/{ => util}/StringUtils.java (95%) diff --git a/src/main/java/io/cryostat/agent/Agent.java b/src/main/java/io/cryostat/agent/Agent.java index 91d390d9..d8714b74 100644 --- a/src/main/java/io/cryostat/agent/Agent.java +++ b/src/main/java/io/cryostat/agent/Agent.java @@ -28,6 +28,7 @@ import javax.inject.Named; import javax.inject.Singleton; +import io.cryostat.agent.harvest.Harvester; import io.cryostat.agent.triggers.TriggerEvaluator; import dagger.Component; diff --git a/src/main/java/io/cryostat/agent/CryostatClient.java b/src/main/java/io/cryostat/agent/CryostatClient.java index fa91957c..7b2999ed 100644 --- a/src/main/java/io/cryostat/agent/CryostatClient.java +++ b/src/main/java/io/cryostat/agent/CryostatClient.java @@ -37,6 +37,7 @@ import java.util.function.Function; import io.cryostat.agent.WebServer.Credentials; +import io.cryostat.agent.harvest.Harvester; import io.cryostat.agent.model.DiscoveryNode; import io.cryostat.agent.model.PluginInfo; import io.cryostat.agent.model.RegistrationInfo; diff --git a/src/main/java/io/cryostat/agent/MainModule.java b/src/main/java/io/cryostat/agent/MainModule.java index 097f1d0f..878fbee8 100644 --- a/src/main/java/io/cryostat/agent/MainModule.java +++ b/src/main/java/io/cryostat/agent/MainModule.java @@ -34,7 +34,8 @@ import javax.net.ssl.TrustManager; import javax.net.ssl.X509TrustManager; -import io.cryostat.agent.Harvester.RecordingSettings; +import io.cryostat.agent.harvest.Harvester; +import io.cryostat.agent.harvest.Harvester.RecordingSettings; import io.cryostat.agent.remote.RemoteContext; import io.cryostat.agent.remote.RemoteModule; import io.cryostat.agent.triggers.TriggerEvaluator; diff --git a/src/main/java/io/cryostat/agent/Registration.java b/src/main/java/io/cryostat/agent/Registration.java index 8d7ca43a..2f792c33 100644 --- a/src/main/java/io/cryostat/agent/Registration.java +++ b/src/main/java/io/cryostat/agent/Registration.java @@ -36,12 +36,13 @@ import io.cryostat.agent.model.DiscoveryNode; import io.cryostat.agent.model.PluginInfo; +import io.cryostat.agent.util.StringUtils; import org.apache.http.client.utils.URIBuilder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -class Registration { +public class Registration { private static final String NODE_TYPE = "JVM"; @@ -186,7 +187,7 @@ void start() { log.info("{} started", getClass().getName()); } - void addRegistrationListener(Consumer listener) { + public void addRegistrationListener(Consumer listener) { this.listeners.add(listener); } @@ -358,9 +359,9 @@ public void notify(RegistrationEvent.State state) { executor.submit(() -> this.listeners.forEach(listener -> listener.accept(evt))); } - static class RegistrationEvent { + public static class RegistrationEvent { - enum State { + public enum State { UNREGISTERED, REGISTERED, PUBLISHED, @@ -368,7 +369,7 @@ enum State { REFRESHED, } - final State state; + public final State state; RegistrationEvent(State state) { this.state = state; diff --git a/src/main/java/io/cryostat/agent/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java similarity index 96% rename from src/main/java/io/cryostat/agent/Harvester.java rename to src/main/java/io/cryostat/agent/harvest/Harvester.java index 1a1a50f1..d0f8ed3c 100644 --- a/src/main/java/io/cryostat/agent/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package io.cryostat.agent; +package io.cryostat.agent.harvest; import java.io.IOException; import java.nio.file.Files; @@ -31,6 +31,11 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.function.UnaryOperator; +import io.cryostat.agent.CryostatClient; +import io.cryostat.agent.Registration; +import io.cryostat.agent.util.StringUtils; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import jdk.jfr.Configuration; import jdk.jfr.FlightRecorder; import jdk.jfr.FlightRecorderListener; @@ -39,7 +44,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -class Harvester implements FlightRecorderListener { +public class Harvester implements FlightRecorderListener { private final Logger log = LoggerFactory.getLogger(getClass()); @@ -57,7 +62,8 @@ class Harvester implements FlightRecorderListener { private Future task; private boolean running; - Harvester( + @SuppressFBWarnings("EI_EXPOSE_REP2") + public Harvester( ScheduledExecutorService executor, ScheduledExecutorService workerPool, long period, @@ -219,7 +225,7 @@ public void recordingStateChanged(Recording recording) { } } - Future exitUpload() { + public Future exitUpload() { return CompletableFuture.supplyAsync( () -> { running = false; @@ -321,15 +327,15 @@ private Future uploadDumpedFile() throws IOException { return client.upload(PushType.EMERGENCY, template, maxFiles, exitPath); } - enum PushType { + public enum PushType { SCHEDULED, ON_STOP, EMERGENCY, } - static class RecordingSettings implements UnaryOperator { - long maxSize; - long maxAge; + public static class RecordingSettings implements UnaryOperator { + public long maxSize; + public long maxAge; @Override public Recording apply(Recording r) { diff --git a/src/main/java/io/cryostat/agent/remote/RecordingsContext.java b/src/main/java/io/cryostat/agent/remote/RecordingsContext.java index cc4502cc..db3399f8 100644 --- a/src/main/java/io/cryostat/agent/remote/RecordingsContext.java +++ b/src/main/java/io/cryostat/agent/remote/RecordingsContext.java @@ -44,7 +44,7 @@ import org.openjdk.jmc.rjmx.services.jfr.IRecordingDescriptor; import org.openjdk.jmc.rjmx.services.jfr.IRecordingDescriptor.RecordingState; -import io.cryostat.agent.StringUtils; +import io.cryostat.agent.util.StringUtils; import io.cryostat.core.FlightRecorderException; import io.cryostat.core.net.JFRConnection; import io.cryostat.core.net.JFRConnectionToolkit; diff --git a/src/main/java/io/cryostat/agent/triggers/TriggerParser.java b/src/main/java/io/cryostat/agent/triggers/TriggerParser.java index 4e099fee..391a45e6 100644 --- a/src/main/java/io/cryostat/agent/triggers/TriggerParser.java +++ b/src/main/java/io/cryostat/agent/triggers/TriggerParser.java @@ -22,7 +22,7 @@ import java.util.regex.Pattern; import io.cryostat.agent.FlightRecorderHelper; -import io.cryostat.agent.StringUtils; +import io.cryostat.agent.util.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; diff --git a/src/main/java/io/cryostat/agent/StringUtils.java b/src/main/java/io/cryostat/agent/util/StringUtils.java similarity index 95% rename from src/main/java/io/cryostat/agent/StringUtils.java rename to src/main/java/io/cryostat/agent/util/StringUtils.java index 6d756df7..a8f76f8f 100644 --- a/src/main/java/io/cryostat/agent/StringUtils.java +++ b/src/main/java/io/cryostat/agent/util/StringUtils.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package io.cryostat.agent; +package io.cryostat.agent.util; public class StringUtils { private StringUtils() {} From 6cbae791caa89d23b7e0dcc6a9d278fe352824fe Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 14:06:25 -0400 Subject: [PATCH 03/21] split out harvest module --- .../java/io/cryostat/agent/MainModule.java | 41 +---------- .../cryostat/agent/harvest/HarvestModule.java | 70 +++++++++++++++++++ 2 files changed, 72 insertions(+), 39 deletions(-) create mode 100644 src/main/java/io/cryostat/agent/harvest/HarvestModule.java diff --git a/src/main/java/io/cryostat/agent/MainModule.java b/src/main/java/io/cryostat/agent/MainModule.java index 878fbee8..45f8f414 100644 --- a/src/main/java/io/cryostat/agent/MainModule.java +++ b/src/main/java/io/cryostat/agent/MainModule.java @@ -34,8 +34,7 @@ import javax.net.ssl.TrustManager; import javax.net.ssl.X509TrustManager; -import io.cryostat.agent.harvest.Harvester; -import io.cryostat.agent.harvest.Harvester.RecordingSettings; +import io.cryostat.agent.harvest.HarvestModule; import io.cryostat.agent.remote.RemoteContext; import io.cryostat.agent.remote.RemoteModule; import io.cryostat.agent.triggers.TriggerEvaluator; @@ -65,6 +64,7 @@ includes = { ConfigModule.class, RemoteModule.class, + HarvestModule.class, }) public abstract class MainModule { @@ -237,43 +237,6 @@ public static Registration provideRegistration( registrationCheckMs); } - @Provides - @Singleton - public static Harvester provideHarvester( - ScheduledExecutorService workerPool, - @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_PERIOD_MS) long period, - @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_TEMPLATE) String template, - @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_MAX_FILES) int maxFiles, - @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_EXIT_MAX_AGE_MS) long exitMaxAge, - @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_EXIT_MAX_SIZE_B) long exitMaxSize, - @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_MAX_AGE_MS) long maxAge, - @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_MAX_SIZE_B) long maxSize, - CryostatClient client, - Registration registration) { - RecordingSettings exitSettings = new RecordingSettings(); - exitSettings.maxAge = exitMaxAge; - exitSettings.maxSize = exitMaxSize; - RecordingSettings periodicSettings = new RecordingSettings(); - periodicSettings.maxAge = maxAge > 0 ? maxAge : (long) (period * 1.5); - periodicSettings.maxSize = maxSize; - return new Harvester( - Executors.newSingleThreadScheduledExecutor( - r -> { - Thread t = new Thread(r); - t.setName("cryostat-agent-harvester"); - t.setDaemon(true); - return t; - }), - workerPool, - period, - template, - maxFiles, - exitSettings, - periodicSettings, - client, - registration); - } - @Provides @Singleton @Named(TRIGGER_SCHEDULER) diff --git a/src/main/java/io/cryostat/agent/harvest/HarvestModule.java b/src/main/java/io/cryostat/agent/harvest/HarvestModule.java new file mode 100644 index 00000000..a88b8744 --- /dev/null +++ b/src/main/java/io/cryostat/agent/harvest/HarvestModule.java @@ -0,0 +1,70 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.agent.harvest; + +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; + +import javax.inject.Named; +import javax.inject.Singleton; + +import io.cryostat.agent.ConfigModule; +import io.cryostat.agent.CryostatClient; +import io.cryostat.agent.Registration; +import io.cryostat.agent.harvest.Harvester.RecordingSettings; + +import dagger.Module; +import dagger.Provides; + +@Module +public abstract class HarvestModule { + @Provides + @Singleton + public static Harvester provideHarvester( + ScheduledExecutorService workerPool, + @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_PERIOD_MS) long period, + @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_TEMPLATE) String template, + @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_MAX_FILES) int maxFiles, + @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_EXIT_MAX_AGE_MS) long exitMaxAge, + @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_EXIT_MAX_SIZE_B) long exitMaxSize, + @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_MAX_AGE_MS) long maxAge, + @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_MAX_SIZE_B) long maxSize, + CryostatClient client, + Registration registration) { + RecordingSettings exitSettings = new RecordingSettings(); + exitSettings.maxAge = exitMaxAge; + exitSettings.maxSize = exitMaxSize; + RecordingSettings periodicSettings = new RecordingSettings(); + periodicSettings.maxAge = maxAge > 0 ? maxAge : (long) (period * 1.5); + periodicSettings.maxSize = maxSize; + return new Harvester( + Executors.newSingleThreadScheduledExecutor( + r -> { + Thread t = new Thread(r); + t.setName("cryostat-agent-harvester"); + t.setDaemon(true); + return t; + }), + workerPool, + period, + template, + maxFiles, + exitSettings, + periodicSettings, + client, + registration); + } +} From 97ee6b77c83f0485ae2f4093675822a4ef98893a Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 14:45:32 -0400 Subject: [PATCH 04/21] preparing Harvester to handle multiple recordings --- .../cryostat/agent/FlightRecorderHelper.java | 4 + .../io/cryostat/agent/harvest/Harvester.java | 152 +++++++++++------- 2 files changed, 98 insertions(+), 58 deletions(-) diff --git a/src/main/java/io/cryostat/agent/FlightRecorderHelper.java b/src/main/java/io/cryostat/agent/FlightRecorderHelper.java index a8b6f7cf..2e256e30 100644 --- a/src/main/java/io/cryostat/agent/FlightRecorderHelper.java +++ b/src/main/java/io/cryostat/agent/FlightRecorderHelper.java @@ -59,6 +59,10 @@ public boolean isValidTemplate(String nameOrLabel) { } public List getRecordings() { + if (!FlightRecorder.isAvailable()) { + log.error("FlightRecorder is unavailable"); + return List.of(); + } return FlightRecorder.getFlightRecorder().getRecordings().stream() .map(RecordingInfo::new) .collect(Collectors.toList()); diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index d0f8ed3c..766caeca 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -21,14 +21,16 @@ import java.nio.file.StandardOpenOption; import java.text.ParseException; import java.time.Duration; +import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicLong; import java.util.function.UnaryOperator; import io.cryostat.agent.CryostatClient; @@ -56,8 +58,9 @@ public class Harvester implements FlightRecorderListener { private final RecordingSettings exitSettings; private final RecordingSettings periodicSettings; private final CryostatClient client; - private final AtomicLong recordingId = new AtomicLong(-1L); - private volatile Path exitPath; + private final Set recordings = ConcurrentHashMap.newKeySet(); + private Optional sownRecording = Optional.empty(); + private final Map exitPaths = new ConcurrentHashMap<>(); private FlightRecorder flightRecorder; private Future task; private boolean running; @@ -112,7 +115,8 @@ public void start() { return; } if (StringUtils.isBlank(template)) { - log.info("Template not specified"); + log.error("Template not specified"); + return; } if (maxFiles <= 0) { log.info( @@ -164,7 +168,7 @@ public void start() { return; } startRecording(true); - running = true; + startPeriodic(); }); } @@ -188,41 +192,52 @@ public void stop() { @Override public void recordingStateChanged(Recording recording) { log.info("{}({}) {}", recording.getName(), recording.getId(), recording.getState().name()); - if (this.recordingId.get() == recording.getId()) { - switch (recording.getState()) { - case NEW: - break; - case DELAYED: - break; - case RUNNING: - break; - case STOPPED: - recording.close(); // we should get notified for the CLOSED state next - break; - case CLOSED: - executor.submit( - () -> { - if (running) { - try { - uploadDumpedFile().get(); - } catch (ExecutionException - | InterruptedException - | IOException e) { - log.warn("Could not upload exit dump file", e); - } finally { - startRecording(false); - } - } - }); - break; - default: - log.warn( - "Unknown state {} for recording with ID {}", - recording.getState(), - recording.getId()); - break; - } - } + getTrackedRecordingById(recording.getId()) + .ifPresent( + trackedRecording -> { + switch (recording.getState()) { + case NEW: + break; + case DELAYED: + break; + case RUNNING: + break; + case STOPPED: + recording + .close(); // we should get notified for the CLOSED state + // next + break; + case CLOSED: + executor.submit( + () -> { + if (running) { + try { + Path exitPath = + exitPaths.remove(trackedRecording); + recordings.remove(trackedRecording); + uploadDumpedFile(exitPath).get(); + Files.deleteIfExists(exitPath); + log.info("Deleted temp file {}", exitPath); + } catch (ExecutionException + | InterruptedException + | IOException e) { + log.warn( + "Could not upload exit dump file", + e); + } finally { + startRecording(false); + } + } + }); + break; + default: + log.warn( + "Unknown state {} for recording with ID {}", + recording.getState(), + recording.getId()); + break; + } + }); } public Future exitUpload() { @@ -246,33 +261,41 @@ public Future exitUpload() { executor); } + public void handleNewRecording(Recording recording) { + try { + recording.setToDisk(true); + recording.setMaxAge(Duration.ofMillis(period)); + recording.setDumpOnExit(true); + Path path = Files.createTempFile(null, null); + Files.write(path, new byte[0], StandardOpenOption.TRUNCATE_EXISTING); + recording.setDestination(path); + log.info("{}({}) will dump to {}", recording.getName(), recording.getId(), path); + this.recordings.add(recording); + this.exitPaths.put(recording, path); + } catch (IOException ioe) { + log.error("Unable to handle recording", ioe); + recording.close(); + } + } + private void startRecording(boolean restart) { executor.submit( () -> { if (restart) { safeCloseCurrentRecording(); - } else if (getById(this.recordingId.get()).isPresent()) { + } else if (sownRecording.isPresent()) { return; } Recording recording = null; try { Configuration config = Configuration.getConfiguration(template); recording = new Recording(config); - recording.setName("cryostat-agent"); - recording.setToDisk(true); - recording.setMaxAge(Duration.ofMillis(period)); - recording.setDumpOnExit(true); - this.exitPath = Files.createTempFile(null, null); - Files.write(exitPath, new byte[0], StandardOpenOption.TRUNCATE_EXISTING); - recording.setDestination(this.exitPath); + recording.setName("cryostat-agent-harvester"); + handleNewRecording(recording); + this.sownRecording = Optional.of(recording); recording.start(); - this.recordingId.set(recording.getId()); - startPeriodic(); } catch (ParseException | IOException e) { log.error("Unable to start recording", e); - if (recording != null) { - recording.close(); - } } }); } @@ -281,20 +304,22 @@ private void startPeriodic() { if (this.task != null) { this.task.cancel(true); } + this.running = true; this.task = workerPool.scheduleAtFixedRate( this::uploadOngoing, period, period, TimeUnit.MILLISECONDS); } private void safeCloseCurrentRecording() { - getById(recordingId.get()).ifPresent(Recording::close); + sownRecording.ifPresent(Recording::close); + sownRecording = Optional.empty(); } - private Optional getById(long id) { + private Optional getTrackedRecordingById(long id) { if (id < 0) { return Optional.empty(); } - for (Recording recording : this.flightRecorder.getRecordings()) { + for (Recording recording : this.recordings) { if (id == recording.getId()) { return Optional.of(recording); } @@ -313,9 +338,20 @@ private Future uploadOngoing(PushType pushType, RecordingSettings settings new IllegalStateException("No source recording data")); } try { + Path exitPath = Files.createTempFile(null, null); Files.write(exitPath, new byte[0], StandardOpenOption.TRUNCATE_EXISTING); recording.dump(exitPath); - return client.upload(pushType, template, maxFiles, exitPath); + log.info("Dumping {}({}) to {}", recording.getName(), recording.getId(), exitPath); + return client.upload(pushType, template, maxFiles, exitPath) + .thenRun( + () -> { + try { + Files.deleteIfExists(exitPath); + log.info("Deleted temp file {}", exitPath); + } catch (IOException ioe) { + log.warn("Failed to clean up snapshot dump file", ioe); + } + }); } catch (IOException e) { return CompletableFuture.failedFuture(e); } finally { @@ -323,7 +359,7 @@ private Future uploadOngoing(PushType pushType, RecordingSettings settings } } - private Future uploadDumpedFile() throws IOException { + private Future uploadDumpedFile(Path exitPath) throws IOException { return client.upload(PushType.EMERGENCY, template, maxFiles, exitPath); } From dd4405c80d7660db28295abd550b3f69d8e72ca0 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 14:47:10 -0400 Subject: [PATCH 05/21] create TriggerModule --- .../java/io/cryostat/agent/MainModule.java | 29 +--------- .../agent/triggers/TriggerModule.java | 58 +++++++++++++++++++ 2 files changed, 60 insertions(+), 27 deletions(-) create mode 100644 src/main/java/io/cryostat/agent/triggers/TriggerModule.java diff --git a/src/main/java/io/cryostat/agent/MainModule.java b/src/main/java/io/cryostat/agent/MainModule.java index 45f8f414..12218056 100644 --- a/src/main/java/io/cryostat/agent/MainModule.java +++ b/src/main/java/io/cryostat/agent/MainModule.java @@ -37,8 +37,7 @@ import io.cryostat.agent.harvest.HarvestModule; import io.cryostat.agent.remote.RemoteContext; import io.cryostat.agent.remote.RemoteModule; -import io.cryostat.agent.triggers.TriggerEvaluator; -import io.cryostat.agent.triggers.TriggerParser; +import io.cryostat.agent.triggers.TriggerModule; import io.cryostat.core.net.JFRConnection; import io.cryostat.core.net.JFRConnectionToolkit; import io.cryostat.core.sys.Environment; @@ -65,6 +64,7 @@ ConfigModule.class, RemoteModule.class, HarvestModule.class, + TriggerModule.class, }) public abstract class MainModule { @@ -72,7 +72,6 @@ public abstract class MainModule { private static final int NUM_WORKER_THREADS = 3; private static final String JVM_ID = "JVM_ID"; private static final String TEMPLATES_PATH = "TEMPLATES_PATH"; - private static final String TRIGGER_SCHEDULER = "TRIGGER_SCHEDULER"; @Provides @Singleton @@ -237,36 +236,12 @@ public static Registration provideRegistration( registrationCheckMs); } - @Provides - @Singleton - @Named(TRIGGER_SCHEDULER) - public static ScheduledExecutorService provideTriggerScheduler() { - return Executors.newScheduledThreadPool(0); - } - @Provides @Singleton public static FlightRecorderHelper provideFlightRecorderHelper() { return new FlightRecorderHelper(); } - @Provides - @Singleton - public static TriggerParser provideTriggerParser(FlightRecorderHelper helper) { - return new TriggerParser(helper); - } - - @Provides - @Singleton - public static TriggerEvaluator provideTriggerEvaluatorFactory( - @Named(TRIGGER_SCHEDULER) ScheduledExecutorService scheduler, - TriggerParser parser, - FlightRecorderHelper helper, - @Named(ConfigModule.CRYOSTAT_AGENT_SMART_TRIGGER_EVALUATION_PERIOD_MS) - long evaluationPeriodMs) { - return new TriggerEvaluator(scheduler, parser, helper, evaluationPeriodMs); - } - @Provides @Singleton public static FileSystem provideFileSystem() { diff --git a/src/main/java/io/cryostat/agent/triggers/TriggerModule.java b/src/main/java/io/cryostat/agent/triggers/TriggerModule.java new file mode 100644 index 00000000..6755b8c5 --- /dev/null +++ b/src/main/java/io/cryostat/agent/triggers/TriggerModule.java @@ -0,0 +1,58 @@ +/* + * Copyright The Cryostat Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.cryostat.agent.triggers; + +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; + +import javax.inject.Named; +import javax.inject.Singleton; + +import io.cryostat.agent.ConfigModule; +import io.cryostat.agent.FlightRecorderHelper; + +import dagger.Module; +import dagger.Provides; + +@Module +public abstract class TriggerModule { + + private static final String TRIGGER_SCHEDULER = "TRIGGER_SCHEDULER"; + + @Provides + @Singleton + @Named(TRIGGER_SCHEDULER) + public static ScheduledExecutorService provideTriggerScheduler() { + return Executors.newScheduledThreadPool(0); + } + + @Provides + @Singleton + public static TriggerParser provideTriggerParser(FlightRecorderHelper helper) { + return new TriggerParser(helper); + } + + @Provides + @Singleton + public static TriggerEvaluator provideTriggerEvaluatorFactory( + @Named(TRIGGER_SCHEDULER) ScheduledExecutorService scheduler, + TriggerParser parser, + FlightRecorderHelper helper, + @Named(ConfigModule.CRYOSTAT_AGENT_SMART_TRIGGER_EVALUATION_PERIOD_MS) + long evaluationPeriodMs) { + return new TriggerEvaluator(scheduler, parser, helper, evaluationPeriodMs); + } +} From 07a356167518cc4af2d5f48dc2946333da257ed4 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:02:28 -0400 Subject: [PATCH 06/21] track smart-triggered recordings in harvester logic --- .../io/cryostat/agent/harvest/Harvester.java | 26 ++++++++++--------- .../agent/triggers/TriggerEvaluator.java | 5 ++++ .../agent/triggers/TriggerModule.java | 4 ++- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 766caeca..44e84d75 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -110,6 +110,7 @@ public void start() { if (running) { return; } + this.running = true; if (period <= 0) { log.info("Harvester disabled, period {} < 0", period); return; @@ -168,7 +169,12 @@ public void start() { return; } startRecording(true); - startPeriodic(); + if (this.task != null) { + this.task.cancel(true); + } + this.task = + workerPool.scheduleAtFixedRate( + this::uploadOngoing, period, period, TimeUnit.MILLISECONDS); }); } @@ -210,6 +216,12 @@ public void recordingStateChanged(Recording recording) { case CLOSED: executor.submit( () -> { + if (sownRecording + .map(Recording::getId) + .map(id -> id == recording.getId()) + .orElse(false)) { + safeCloseCurrentRecording(); + } if (running) { try { Path exitPath = @@ -244,7 +256,7 @@ public Future exitUpload() { return CompletableFuture.supplyAsync( () -> { running = false; - if (flightRecorder == null || period <= 0) { + if (flightRecorder == null) { return null; } try { @@ -300,16 +312,6 @@ private void startRecording(boolean restart) { }); } - private void startPeriodic() { - if (this.task != null) { - this.task.cancel(true); - } - this.running = true; - this.task = - workerPool.scheduleAtFixedRate( - this::uploadOngoing, period, period, TimeUnit.MILLISECONDS); - } - private void safeCloseCurrentRecording() { sownRecording.ifPresent(Recording::close); sownRecording = Optional.empty(); diff --git a/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java b/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java index 75f841c8..3c4b4400 100644 --- a/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java +++ b/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java @@ -27,6 +27,7 @@ import java.util.concurrent.TimeUnit; import io.cryostat.agent.FlightRecorderHelper; +import io.cryostat.agent.harvest.Harvester; import io.cryostat.agent.model.MBeanInfo; import io.cryostat.agent.triggers.SmartTrigger.TriggerState; @@ -44,6 +45,7 @@ public class TriggerEvaluator { private final ScheduledExecutorService scheduler; private final TriggerParser parser; private final FlightRecorderHelper flightRecorderHelper; + private final Harvester harvester; private final long evaluationPeriodMs; private final ConcurrentLinkedQueue triggers = new ConcurrentLinkedQueue<>(); private Future task; @@ -53,10 +55,12 @@ public TriggerEvaluator( ScheduledExecutorService scheduler, TriggerParser parser, FlightRecorderHelper flightRecorderHelper, + Harvester harvester, long evaluationPeriodMs) { this.scheduler = scheduler; this.parser = parser; this.flightRecorderHelper = flightRecorderHelper; + this.harvester = harvester; this.evaluationPeriodMs = evaluationPeriodMs; } @@ -159,6 +163,7 @@ private void startRecording(SmartTrigger t) { String recordingName = String.format("cryostat-smart-trigger-%d", recording.getId()); recording.setName(recordingName); + harvester.handleNewRecording(recording); recording.start(); t.setState(TriggerState.COMPLETE); log.info( diff --git a/src/main/java/io/cryostat/agent/triggers/TriggerModule.java b/src/main/java/io/cryostat/agent/triggers/TriggerModule.java index 6755b8c5..94896a2e 100644 --- a/src/main/java/io/cryostat/agent/triggers/TriggerModule.java +++ b/src/main/java/io/cryostat/agent/triggers/TriggerModule.java @@ -23,6 +23,7 @@ import io.cryostat.agent.ConfigModule; import io.cryostat.agent.FlightRecorderHelper; +import io.cryostat.agent.harvest.Harvester; import dagger.Module; import dagger.Provides; @@ -51,8 +52,9 @@ public static TriggerEvaluator provideTriggerEvaluatorFactory( @Named(TRIGGER_SCHEDULER) ScheduledExecutorService scheduler, TriggerParser parser, FlightRecorderHelper helper, + Harvester harvester, @Named(ConfigModule.CRYOSTAT_AGENT_SMART_TRIGGER_EVALUATION_PERIOD_MS) long evaluationPeriodMs) { - return new TriggerEvaluator(scheduler, parser, helper, evaluationPeriodMs); + return new TriggerEvaluator(scheduler, parser, helper, harvester, evaluationPeriodMs); } } From a84552559d7fd1d105e1eeaa77cb0053cc227ecf Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:18:55 -0400 Subject: [PATCH 07/21] harvester can start collecting smart triggers without its own sown recording --- README.md | 27 ++++++++++++++++--- .../io/cryostat/agent/harvest/Harvester.java | 15 +++++------ .../META-INF/microprofile-config.properties | 2 +- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 3ce06896..da1180a9 100644 --- a/README.md +++ b/README.md @@ -31,9 +31,18 @@ JAVA_OPTIONS="-Dcom.sun.management.jmxremote.port=9091 -Dcom.sun.management.jmxr ``` This assumes that the agent JAR has been included in the application image within `/deployments/app/`. +## Harvester + +The various `cryostat.agent.harvester.*` properties may be used to configure `cryostat-agent` to start a new Flight +Recording using a given event template on Agent initialization, and to periodically collect this recorded data and push +it to the Agent's associated Cryostat server. The Agent will also attempt to push the tail end of this recording on JVM +shutdown so that the cause of an unexpected JVM shutdown might be captured for later analysis. + ## SMART TRIGGERS -`cryostat-agent` supports smart triggers that listen to the values of the MBean Counters and can start recordings based on a set of constraints specified by the user. +`cryostat-agent` supports smart triggers that listen to the values of the MBean Counters and can start recordings based +on a set of constraints specified by the user. + The general form of a smart trigger expression is as follows: ``` @@ -46,7 +55,8 @@ An example for listening to CPU Usage and starting a recording using the Profili [ProcessCpuLoad>0.2]~profile ``` -An example for watching for the Thread Count to exceed 20 for longer than 10 seconds and starting a recording using the Continuous template: +An example for watching for the Thread Count to exceed 20 for longer than 10 seconds and starting a recording using the +Continuous template: ``` [ThreadCount>20&&TargetDuration>duration("10s")]~Continuous @@ -64,7 +74,16 @@ Multiple smart trigger definitions may be specified and separated by commas, for [ProcessCpuLoad>0.2]~profile,[ThreadCount>30]~Continuous ``` -**NOTE**: Smart Triggers are evaluated on a polling basis. The poll period is configurable (see list below). This means that your conditions are subject to sampling biases. +**NOTE**: Smart Triggers are evaluated on a polling basis. The poll period is configurable (see list below). This means +that your conditions are subject to sampling biases. + +### Harvester Integration + +Any Flight Recordings created by Smart Trigger will also be tracked by the Harvester system. This data will be captured +in a JFR Snapshot and pushed to the server on the Harvester's usual schedule. By defining Smart Triggers and a +Harvester period without a Harvester template, you can achieve a setup where dynamically-started Flight Recordings +begin when trigger conditions are met, and their data is then periodically captured until the recording is manually +stopped or the host JVM shuts down. ## CONFIGURATION @@ -91,7 +110,7 @@ and how it advertises itself to a Cryostat server instance. Required properties - [ ] `cryostat.agent.exit.signals` [`[String]`]: a comma-separated list of signals that the agent should handle. When any of these signals is caught the agent initiates an orderly shutdown, deregistering from the Cryostat server and potentially uploading the latest harvested JFR data. Default `INT,TERM`. - [ ] `cryostat.agent.exit.deregistration.timeout-ms` [`long`]: the duration in milliseconds to wait for a response from the Cryostat server when attempting to deregister at shutdown time . Default `3000`. - [ ] `cryostat.agent.harvester.period-ms` [`long`]: the length of time between JFR collections and pushes by the harvester. This also controls the maximum age of data stored in the buffer for the harvester's managed Flight Recording. Every `period-ms` the harvester will upload a JFR binary file to the `cryostat.agent.baseuri` archives. Default `-1`, which indicates no harvesting will be performed. -- [ ] `cryostat.agent.harvester.template` [`String`]: the name of the `.jfc` event template configuration to use for the harvester's managed Flight Recording. Default `default`, the continuous monitoring event template. +- [ ] `cryostat.agent.harvester.template` [`String`]: the name of the `.jfc` event template configuration to use for the harvester's managed Flight Recording. Defaults to the empty string, so that no recording is started. - [ ] `cryostat.agent.harvester.max-files` [`String`]: the maximum number of pushed files that Cryostat will keep over the network from the agent. This is supplied to the harvester's push requests which instructs Cryostat to prune, in a FIFO manner, the oldest JFR files within the attached JVM target's storage, while the number of stored recordings is greater than this configuration's maximum file limit. Default `2147483647` (`Integer.MAX_VALUE`). - [ ] `cryostat.agent.harvester.upload.timeout-ms` [`long`]: the duration in milliseconds to wait for HTTP upload requests to the Cryostat server to complete and respond. Default `30000`. - [ ] `cryostat.agent.harvester.exit.max-age-ms` [`long`]: the JFR `maxage` setting, specified in milliseconds, to apply to recording data uploaded to the Cryostat server when the JVM this Agent instance is attached to exits. This ensures that tail-end data is captured between the last periodic push and the application exit. Exit uploads only occur when the application receives `SIGINT`/`SIGTERM` from the operating system or container platform. diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 44e84d75..a18107b1 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -117,7 +117,6 @@ public void start() { } if (StringUtils.isBlank(template)) { log.error("Template not specified"); - return; } if (maxFiles <= 0) { log.info( @@ -132,10 +131,7 @@ public void start() { try { FlightRecorder.addListener(this); this.flightRecorder = FlightRecorder.getFlightRecorder(); - log.info( - "JFR Harvester started using template \"{}\" with period {}", - template, - Duration.ofMillis(period)); + log.info("JFR Harvester started with period {}", Duration.ofMillis(period)); if (exitSettings.maxAge > 0) { log.info( "On-stop uploads will contain approximately the most recent" @@ -229,7 +225,7 @@ public void recordingStateChanged(Recording recording) { recordings.remove(trackedRecording); uploadDumpedFile(exitPath).get(); Files.deleteIfExists(exitPath); - log.info("Deleted temp file {}", exitPath); + log.trace("Deleted temp file {}", exitPath); } catch (ExecutionException | InterruptedException | IOException e) { @@ -281,7 +277,7 @@ public void handleNewRecording(Recording recording) { Path path = Files.createTempFile(null, null); Files.write(path, new byte[0], StandardOpenOption.TRUNCATE_EXISTING); recording.setDestination(path); - log.info("{}({}) will dump to {}", recording.getName(), recording.getId(), path); + log.trace("{}({}) will dump to {}", recording.getName(), recording.getId(), path); this.recordings.add(recording); this.exitPaths.put(recording, path); } catch (IOException ioe) { @@ -306,6 +302,7 @@ private void startRecording(boolean restart) { handleNewRecording(recording); this.sownRecording = Optional.of(recording); recording.start(); + log.info("JFR Harvester started recording using template \"{}\"", template); } catch (ParseException | IOException e) { log.error("Unable to start recording", e); } @@ -343,13 +340,13 @@ private Future uploadOngoing(PushType pushType, RecordingSettings settings Path exitPath = Files.createTempFile(null, null); Files.write(exitPath, new byte[0], StandardOpenOption.TRUNCATE_EXISTING); recording.dump(exitPath); - log.info("Dumping {}({}) to {}", recording.getName(), recording.getId(), exitPath); + log.trace("Dumping {}({}) to {}", recording.getName(), recording.getId(), exitPath); return client.upload(pushType, template, maxFiles, exitPath) .thenRun( () -> { try { Files.deleteIfExists(exitPath); - log.info("Deleted temp file {}", exitPath); + log.trace("Deleted temp file {}", exitPath); } catch (IOException ioe) { log.warn("Failed to clean up snapshot dump file", ioe); } diff --git a/src/main/resources/META-INF/microprofile-config.properties b/src/main/resources/META-INF/microprofile-config.properties index 73b3a1be..e2b80184 100644 --- a/src/main/resources/META-INF/microprofile-config.properties +++ b/src/main/resources/META-INF/microprofile-config.properties @@ -20,7 +20,7 @@ cryostat.agent.registration.check-ms=60000 cryostat.agent.exit.deregistration.timeout-ms=3000 cryostat.agent.harvester.period-ms=-1 -cryostat.agent.harvester.template=default +cryostat.agent.harvester.template= cryostat.agent.harvester.max-files=2147483647 cryostat.agent.harvester.upload.timeout-ms=30000 cryostat.agent.harvester.exit.max-age-ms=0 From 4420aac1d9f45549dcfa01180649ade62717c41c Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:20:36 -0400 Subject: [PATCH 08/21] fixup! harvester can start collecting smart triggers without its own sown recording --- src/main/java/io/cryostat/agent/ConfigModule.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/cryostat/agent/ConfigModule.java b/src/main/java/io/cryostat/agent/ConfigModule.java index 212d8b00..c0f0dac8 100644 --- a/src/main/java/io/cryostat/agent/ConfigModule.java +++ b/src/main/java/io/cryostat/agent/ConfigModule.java @@ -232,7 +232,7 @@ public static long provideCryostatAgentHarvesterPeriod(SmallRyeConfig config) { @Singleton @Named(CRYOSTAT_AGENT_HARVESTER_TEMPLATE) public static String provideCryostatAgentHarvesterTemplate(SmallRyeConfig config) { - return config.getValue(CRYOSTAT_AGENT_HARVESTER_TEMPLATE, String.class); + return config.getOptionalValue(CRYOSTAT_AGENT_HARVESTER_TEMPLATE, String.class).orElse(""); } @Provides From a83027fd33b49710adcd07caaeea51c514d85334 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:22:01 -0400 Subject: [PATCH 09/21] fixup! fixup! harvester can start collecting smart triggers without its own sown recording --- src/main/java/io/cryostat/agent/harvest/Harvester.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index a18107b1..54577e05 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -289,7 +289,9 @@ public void handleNewRecording(Recording recording) { private void startRecording(boolean restart) { executor.submit( () -> { - if (restart) { + if (StringUtils.isBlank(template)) { + return; + } else if (restart) { safeCloseCurrentRecording(); } else if (sownRecording.isPresent()) { return; From bba0db3fe5cd84698960e89eb1ad0687e11c6518 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:24:56 -0400 Subject: [PATCH 10/21] fixup! fixup! fixup! harvester can start collecting smart triggers without its own sown recording --- README.md | 2 +- .../io/cryostat/agent/harvest/Harvester.java | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index da1180a9..e57d266a 100644 --- a/README.md +++ b/README.md @@ -109,7 +109,7 @@ and how it advertises itself to a Cryostat server instance. Required properties - [ ] `cryostat.agent.registration.retry-ms` [`long`]: the duration in milliseconds between attempts to register with the Cryostat server. Default `5000`. - [ ] `cryostat.agent.exit.signals` [`[String]`]: a comma-separated list of signals that the agent should handle. When any of these signals is caught the agent initiates an orderly shutdown, deregistering from the Cryostat server and potentially uploading the latest harvested JFR data. Default `INT,TERM`. - [ ] `cryostat.agent.exit.deregistration.timeout-ms` [`long`]: the duration in milliseconds to wait for a response from the Cryostat server when attempting to deregister at shutdown time . Default `3000`. -- [ ] `cryostat.agent.harvester.period-ms` [`long`]: the length of time between JFR collections and pushes by the harvester. This also controls the maximum age of data stored in the buffer for the harvester's managed Flight Recording. Every `period-ms` the harvester will upload a JFR binary file to the `cryostat.agent.baseuri` archives. Default `-1`, which indicates no harvesting will be performed. +- [ ] `cryostat.agent.harvester.period-ms` [`long`]: the length of time between JFR collections and pushes by the harvester. This also controls the maximum age of data stored in the buffer for the harvester's managed Flight Recording. Every `period-ms` the harvester will upload a JFR binary file to the `cryostat.agent.baseuri` archives. Default `-1`, which indicates no scheduled harvest uploading will be performed. - [ ] `cryostat.agent.harvester.template` [`String`]: the name of the `.jfc` event template configuration to use for the harvester's managed Flight Recording. Defaults to the empty string, so that no recording is started. - [ ] `cryostat.agent.harvester.max-files` [`String`]: the maximum number of pushed files that Cryostat will keep over the network from the agent. This is supplied to the harvester's push requests which instructs Cryostat to prune, in a FIFO manner, the oldest JFR files within the attached JVM target's storage, while the number of stored recordings is greater than this configuration's maximum file limit. Default `2147483647` (`Integer.MAX_VALUE`). - [ ] `cryostat.agent.harvester.upload.timeout-ms` [`long`]: the duration in milliseconds to wait for HTTP upload requests to the Cryostat server to complete and respond. Default `30000`. diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 54577e05..9f6e1000 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -111,12 +111,8 @@ public void start() { return; } this.running = true; - if (period <= 0) { - log.info("Harvester disabled, period {} < 0", period); - return; - } if (StringUtils.isBlank(template)) { - log.error("Template not specified"); + log.info("Template not specified"); } if (maxFiles <= 0) { log.info( @@ -168,9 +164,13 @@ public void start() { if (this.task != null) { this.task.cancel(true); } - this.task = - workerPool.scheduleAtFixedRate( - this::uploadOngoing, period, period, TimeUnit.MILLISECONDS); + if (period <= 0) { + log.info("Harvester periodic uploads disabled, period {} < 0", period); + } else { + this.task = + workerPool.scheduleAtFixedRate( + this::uploadOngoing, period, period, TimeUnit.MILLISECONDS); + } }); } From 3e1fe90a16185124464f639a8f9019a163246697 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:27:16 -0400 Subject: [PATCH 11/21] fixup! fixup! fixup! fixup! harvester can start collecting smart triggers without its own sown recording --- src/main/java/io/cryostat/agent/harvest/Harvester.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 9f6e1000..e709a81a 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -127,7 +127,6 @@ public void start() { try { FlightRecorder.addListener(this); this.flightRecorder = FlightRecorder.getFlightRecorder(); - log.info("JFR Harvester started with period {}", Duration.ofMillis(period)); if (exitSettings.maxAge > 0) { log.info( "On-stop uploads will contain approximately the most recent" @@ -164,12 +163,15 @@ public void start() { if (this.task != null) { this.task.cancel(true); } - if (period <= 0) { - log.info("Harvester periodic uploads disabled, period {} < 0", period); - } else { + if (period > 0) { + log.info("JFR Harvester started with period {}", Duration.ofMillis(period)); this.task = workerPool.scheduleAtFixedRate( this::uploadOngoing, period, period, TimeUnit.MILLISECONDS); + } else { + log.info( + "JFR Harvester started, periodic uploads disabled (period {} < 0)", + period); } }); } From 471cd25d353896ee11767d223cd4aea451681080 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:33:13 -0400 Subject: [PATCH 12/21] apply periodic upload age/size settings to handled recordings --- src/main/java/io/cryostat/agent/harvest/Harvester.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index e709a81a..52a2e52b 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -274,8 +274,8 @@ public Future exitUpload() { public void handleNewRecording(Recording recording) { try { recording.setToDisk(true); - recording.setMaxAge(Duration.ofMillis(period)); recording.setDumpOnExit(true); + recording = this.periodicSettings.apply(recording); Path path = Files.createTempFile(null, null); Files.write(path, new byte[0], StandardOpenOption.TRUNCATE_EXISTING); recording.setDestination(path); From 799121aac4b787f4c6e25f02ea2812058ebd1122 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:36:18 -0400 Subject: [PATCH 13/21] only close and restart sown recording --- .../java/io/cryostat/agent/harvest/Harvester.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 52a2e52b..68bb1b69 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -199,6 +199,11 @@ public void recordingStateChanged(Recording recording) { getTrackedRecordingById(recording.getId()) .ifPresent( trackedRecording -> { + boolean isSownRecording = + sownRecording + .map(Recording::getId) + .map(id -> id == recording.getId()) + .orElse(false); switch (recording.getState()) { case NEW: break; @@ -207,17 +212,15 @@ public void recordingStateChanged(Recording recording) { case RUNNING: break; case STOPPED: - recording - .close(); // we should get notified for the CLOSED state + if (isSownRecording) { + recording.close(); + } // next break; case CLOSED: executor.submit( () -> { - if (sownRecording - .map(Recording::getId) - .map(id -> id == recording.getId()) - .orElse(false)) { + if (isSownRecording) { safeCloseCurrentRecording(); } if (running) { From 578573336b96ec015868e3c9921e715a37a7f27e Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:43:35 -0400 Subject: [PATCH 14/21] ensure recording dumps to exit path --- src/main/java/io/cryostat/agent/harvest/Harvester.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 68bb1b69..1cb9cac4 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -228,6 +228,7 @@ public void recordingStateChanged(Recording recording) { Path exitPath = exitPaths.remove(trackedRecording); recordings.remove(trackedRecording); + trackedRecording.dump(exitPath); uploadDumpedFile(exitPath).get(); Files.deleteIfExists(exitPath); log.trace("Deleted temp file {}", exitPath); From 3ed5daf407ae20840b9a366965d30bbcebba7a0d Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 15:46:18 -0400 Subject: [PATCH 15/21] fixup! ensure recording dumps to exit path --- src/main/java/io/cryostat/agent/harvest/Harvester.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 1cb9cac4..3d3b8bbe 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -212,6 +212,11 @@ public void recordingStateChanged(Recording recording) { case RUNNING: break; case STOPPED: + try { + trackedRecording.dump(exitPaths.get(trackedRecording)); + } catch (IOException e) { + log.error("Failed to dump recording to file", e); + } if (isSownRecording) { recording.close(); } @@ -228,7 +233,6 @@ public void recordingStateChanged(Recording recording) { Path exitPath = exitPaths.remove(trackedRecording); recordings.remove(trackedRecording); - trackedRecording.dump(exitPath); uploadDumpedFile(exitPath).get(); Files.deleteIfExists(exitPath); log.trace("Deleted temp file {}", exitPath); From 2b4f77e0001d4b65212e8adc390b2a8ac8e526e2 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 16:05:55 -0400 Subject: [PATCH 16/21] refactoring, ensure correct template type is reflected in upload labels --- .../io/cryostat/agent/CryostatClient.java | 20 ++--- .../cryostat/agent/FlightRecorderHelper.java | 23 ++++- .../cryostat/agent/harvest/HarvestModule.java | 3 + .../io/cryostat/agent/harvest/Harvester.java | 88 ++++++++++++------- .../agent/triggers/TriggerEvaluator.java | 11 +-- 5 files changed, 94 insertions(+), 51 deletions(-) diff --git a/src/main/java/io/cryostat/agent/CryostatClient.java b/src/main/java/io/cryostat/agent/CryostatClient.java index 7b2999ed..475c4639 100644 --- a/src/main/java/io/cryostat/agent/CryostatClient.java +++ b/src/main/java/io/cryostat/agent/CryostatClient.java @@ -27,9 +27,11 @@ import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.Collection; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; @@ -361,21 +363,17 @@ public CompletableFuture update( } public CompletableFuture upload( - Harvester.PushType pushType, String template, int maxFiles, Path recording) + Harvester.PushType pushType, Optional template, int maxFiles, Path recording) throws IOException { Instant start = Instant.now(); String timestamp = start.truncatedTo(ChronoUnit.SECONDS).toString().replaceAll("[-:]", ""); - String fileName = String.format("%s_%s_%s.jfr", appName, template, timestamp); + String fileName = + template.map(t -> String.format("%s_%s_%s.jfr", appName, t, timestamp)) + .orElseGet(() -> String.format("%s_%s.jfr", appName, timestamp)); Map labels = - Map.of( - "jvmId", - jvmId, - "template.name", - template, - "template.type", - "TARGET", - "pushType", - pushType.name()); + new HashMap<>(Map.of("jvmId", jvmId, "pushType", pushType.name())); + template.ifPresent( + t -> labels.putAll(Map.of("template.name", t, "template.type", "TARGET"))); HttpPost req = new HttpPost(baseUri.resolve("/api/beta/recordings/" + jvmId)); diff --git a/src/main/java/io/cryostat/agent/FlightRecorderHelper.java b/src/main/java/io/cryostat/agent/FlightRecorderHelper.java index 2e256e30..9814bc9c 100644 --- a/src/main/java/io/cryostat/agent/FlightRecorderHelper.java +++ b/src/main/java/io/cryostat/agent/FlightRecorderHelper.java @@ -32,7 +32,7 @@ public class FlightRecorderHelper { private final Logger log = LoggerFactory.getLogger(getClass()); - public Optional createRecording(String templateNameOrLabel) { + public Optional createRecording(String templateNameOrLabel) { Optional opt = getTemplate(templateNameOrLabel); if (opt.isEmpty()) { log.error( @@ -43,7 +43,7 @@ public Optional createRecording(String templateNameOrLabel) { Configuration configuration = opt.get(); Recording recording = new Recording(configuration.getSettings()); recording.setToDisk(true); - return Optional.of(recording); + return Optional.of(new TemplatedRecording(configuration, recording)); } public Optional getTemplate(String nameOrLabel) { @@ -68,6 +68,25 @@ public List getRecordings() { .collect(Collectors.toList()); } + @SuppressFBWarnings(value = {"EI_EXPOSE_REP", "EI_EXPOSE_REP2"}) + public static class TemplatedRecording { + private final Configuration configuration; + private final Recording recording; + + public TemplatedRecording(Configuration configuration, Recording recording) { + this.configuration = configuration; + this.recording = recording; + } + + public Configuration getConfiguration() { + return configuration; + } + + public Recording getRecording() { + return recording; + } + } + @SuppressFBWarnings(value = "URF_UNREAD_FIELD") public static class RecordingInfo { diff --git a/src/main/java/io/cryostat/agent/harvest/HarvestModule.java b/src/main/java/io/cryostat/agent/harvest/HarvestModule.java index a88b8744..9234bdb2 100644 --- a/src/main/java/io/cryostat/agent/harvest/HarvestModule.java +++ b/src/main/java/io/cryostat/agent/harvest/HarvestModule.java @@ -23,6 +23,7 @@ import io.cryostat.agent.ConfigModule; import io.cryostat.agent.CryostatClient; +import io.cryostat.agent.FlightRecorderHelper; import io.cryostat.agent.Registration; import io.cryostat.agent.harvest.Harvester.RecordingSettings; @@ -43,6 +44,7 @@ public static Harvester provideHarvester( @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_MAX_AGE_MS) long maxAge, @Named(ConfigModule.CRYOSTAT_AGENT_HARVESTER_MAX_SIZE_B) long maxSize, CryostatClient client, + FlightRecorderHelper flightRecorderHelper, Registration registration) { RecordingSettings exitSettings = new RecordingSettings(); exitSettings.maxAge = exitMaxAge; @@ -65,6 +67,7 @@ public static Harvester provideHarvester( exitSettings, periodicSettings, client, + flightRecorderHelper, registration); } } diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 3d3b8bbe..5cf6839f 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -19,7 +19,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardOpenOption; -import java.text.ParseException; import java.time.Duration; import java.util.Map; import java.util.Optional; @@ -34,6 +33,8 @@ import java.util.function.UnaryOperator; import io.cryostat.agent.CryostatClient; +import io.cryostat.agent.FlightRecorderHelper; +import io.cryostat.agent.FlightRecorderHelper.TemplatedRecording; import io.cryostat.agent.Registration; import io.cryostat.agent.util.StringUtils; @@ -58,9 +59,10 @@ public class Harvester implements FlightRecorderListener { private final RecordingSettings exitSettings; private final RecordingSettings periodicSettings; private final CryostatClient client; - private final Set recordings = ConcurrentHashMap.newKeySet(); - private Optional sownRecording = Optional.empty(); - private final Map exitPaths = new ConcurrentHashMap<>(); + private final FlightRecorderHelper flightRecorderHelper; + private final Set recordings = ConcurrentHashMap.newKeySet(); + private Optional sownRecording = Optional.empty(); + private final Map exitPaths = new ConcurrentHashMap<>(); private FlightRecorder flightRecorder; private Future task; private boolean running; @@ -75,6 +77,7 @@ public Harvester( RecordingSettings exitSettings, RecordingSettings periodicSettings, CryostatClient client, + FlightRecorderHelper flightRecorderHelper, Registration registration) { this.executor = executor; this.workerPool = workerPool; @@ -84,6 +87,7 @@ public Harvester( this.exitSettings = exitSettings; this.periodicSettings = periodicSettings; this.client = client; + this.flightRecorderHelper = flightRecorderHelper; registration.addRegistrationListener( evt -> { @@ -198,9 +202,10 @@ public void recordingStateChanged(Recording recording) { log.info("{}({}) {}", recording.getName(), recording.getId(), recording.getState().name()); getTrackedRecordingById(recording.getId()) .ifPresent( - trackedRecording -> { + tr -> { boolean isSownRecording = sownRecording + .map(TemplatedRecording::getRecording) .map(Recording::getId) .map(id -> id == recording.getId()) .orElse(false); @@ -213,7 +218,7 @@ public void recordingStateChanged(Recording recording) { break; case STOPPED: try { - trackedRecording.dump(exitPaths.get(trackedRecording)); + tr.getRecording().dump(exitPaths.get(tr)); } catch (IOException e) { log.error("Failed to dump recording to file", e); } @@ -230,10 +235,9 @@ public void recordingStateChanged(Recording recording) { } if (running) { try { - Path exitPath = - exitPaths.remove(trackedRecording); - recordings.remove(trackedRecording); - uploadDumpedFile(exitPath).get(); + Path exitPath = exitPaths.remove(tr); + recordings.remove(tr); + uploadRecording(tr).get(); Files.deleteIfExists(exitPath); log.trace("Deleted temp file {}", exitPath); } catch (ExecutionException @@ -279,8 +283,9 @@ public Future exitUpload() { executor); } - public void handleNewRecording(Recording recording) { + public void handleNewRecording(TemplatedRecording tr) { try { + Recording recording = tr.getRecording(); recording.setToDisk(true); recording.setDumpOnExit(true); recording = this.periodicSettings.apply(recording); @@ -288,11 +293,11 @@ public void handleNewRecording(Recording recording) { Files.write(path, new byte[0], StandardOpenOption.TRUNCATE_EXISTING); recording.setDestination(path); log.trace("{}({}) will dump to {}", recording.getName(), recording.getId(), path); - this.recordings.add(recording); - this.exitPaths.put(recording, path); + this.recordings.add(tr); + this.exitPaths.put(tr, path); } catch (IOException ioe) { log.error("Unable to handle recording", ioe); - recording.close(); + tr.getRecording().close(); } } @@ -306,32 +311,35 @@ private void startRecording(boolean restart) { } else if (sownRecording.isPresent()) { return; } - Recording recording = null; - try { - Configuration config = Configuration.getConfiguration(template); - recording = new Recording(config); - recording.setName("cryostat-agent-harvester"); - handleNewRecording(recording); - this.sownRecording = Optional.of(recording); - recording.start(); - log.info("JFR Harvester started recording using template \"{}\"", template); - } catch (ParseException | IOException e) { - log.error("Unable to start recording", e); - } + flightRecorderHelper + .createRecording(template) + .ifPresent( + recording -> { + recording + .getRecording() + .setName("cryostat-agent-harvester"); + handleNewRecording(recording); + this.sownRecording = Optional.of(recording); + recording.getRecording().start(); + log.info( + "JFR Harvester started recording using template" + + " \"{}\"", + template); + }); }); } private void safeCloseCurrentRecording() { - sownRecording.ifPresent(Recording::close); + sownRecording.map(TemplatedRecording::getRecording).ifPresent(Recording::close); sownRecording = Optional.empty(); } - private Optional getTrackedRecordingById(long id) { + private Optional getTrackedRecordingById(long id) { if (id < 0) { return Optional.empty(); } - for (Recording recording : this.recordings) { - if (id == recording.getId()) { + for (TemplatedRecording recording : this.recordings) { + if (id == recording.getRecording().getId()) { return Optional.of(recording); } } @@ -353,7 +361,13 @@ private Future uploadOngoing(PushType pushType, RecordingSettings settings Files.write(exitPath, new byte[0], StandardOpenOption.TRUNCATE_EXISTING); recording.dump(exitPath); log.trace("Dumping {}({}) to {}", recording.getName(), recording.getId(), exitPath); - return client.upload(pushType, template, maxFiles, exitPath) + return client.upload( + pushType, + sownRecording + .map(TemplatedRecording::getConfiguration) + .map(Configuration::getName), + maxFiles, + exitPath) .thenRun( () -> { try { @@ -370,8 +384,16 @@ private Future uploadOngoing(PushType pushType, RecordingSettings settings } } - private Future uploadDumpedFile(Path exitPath) throws IOException { - return client.upload(PushType.EMERGENCY, template, maxFiles, exitPath); + private Future uploadRecording(TemplatedRecording tr) throws IOException { + if (!exitPaths.containsKey(tr)) { + return CompletableFuture.failedFuture(new IllegalStateException()); + } + Path exitPath = exitPaths.get(tr); + return client.upload( + PushType.EMERGENCY, + Optional.of(tr.getConfiguration().getName()), + maxFiles, + exitPath); } public enum PushType { diff --git a/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java b/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java index 3c4b4400..bec064d9 100644 --- a/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java +++ b/src/main/java/io/cryostat/agent/triggers/TriggerEvaluator.java @@ -159,12 +159,13 @@ private void startRecording(SmartTrigger t) { flightRecorderHelper .createRecording(t.getRecordingTemplateName()) .ifPresent( - recording -> { + tr -> { String recordingName = - String.format("cryostat-smart-trigger-%d", recording.getId()); - recording.setName(recordingName); - harvester.handleNewRecording(recording); - recording.start(); + String.format( + "cryostat-smart-trigger-%d", tr.getRecording().getId()); + tr.getRecording().setName(recordingName); + harvester.handleNewRecording(tr); + tr.getRecording().start(); t.setState(TriggerState.COMPLETE); log.info( "Started recording \"{}\" using template \"{}\"", From 21f05da353e60f2e42a0722d7e2ad6d7827d9fa7 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 16:07:29 -0400 Subject: [PATCH 17/21] fixup! refactoring, ensure correct template type is reflected in upload labels --- src/main/java/io/cryostat/agent/harvest/Harvester.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 5cf6839f..e4c04eef 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -385,9 +385,6 @@ private Future uploadOngoing(PushType pushType, RecordingSettings settings } private Future uploadRecording(TemplatedRecording tr) throws IOException { - if (!exitPaths.containsKey(tr)) { - return CompletableFuture.failedFuture(new IllegalStateException()); - } Path exitPath = exitPaths.get(tr); return client.upload( PushType.EMERGENCY, From e3c7d7aa2603198cb8e9a7b567b3995d6bc7685c Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 16:11:09 -0400 Subject: [PATCH 18/21] fixup! fixup! ensure recording dumps to exit path --- .../io/cryostat/agent/harvest/Harvester.java | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index e4c04eef..280e6ec1 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -219,33 +219,28 @@ public void recordingStateChanged(Recording recording) { case STOPPED: try { tr.getRecording().dump(exitPaths.get(tr)); + uploadRecording(tr).get(); } catch (IOException e) { log.error("Failed to dump recording to file", e); + } catch (InterruptedException | ExecutionException e) { + log.warn("Could not upload exit dump file", e); } if (isSownRecording) { - recording.close(); + safeCloseCurrentRecording(); } // next break; case CLOSED: executor.submit( () -> { - if (isSownRecording) { - safeCloseCurrentRecording(); - } if (running) { try { Path exitPath = exitPaths.remove(tr); recordings.remove(tr); - uploadRecording(tr).get(); Files.deleteIfExists(exitPath); log.trace("Deleted temp file {}", exitPath); - } catch (ExecutionException - | InterruptedException - | IOException e) { - log.warn( - "Could not upload exit dump file", - e); + } catch (IOException e) { + log.warn("Could not delete temp file", e); } finally { startRecording(false); } From 391e3b804e7210b6faaacb8eab558e6464b79514 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 16:12:20 -0400 Subject: [PATCH 19/21] cleanup --- .../io/cryostat/agent/harvest/Harvester.java | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 280e6ec1..b3e23007 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -233,17 +233,15 @@ public void recordingStateChanged(Recording recording) { case CLOSED: executor.submit( () -> { - if (running) { - try { - Path exitPath = exitPaths.remove(tr); - recordings.remove(tr); - Files.deleteIfExists(exitPath); - log.trace("Deleted temp file {}", exitPath); - } catch (IOException e) { - log.warn("Could not delete temp file", e); - } finally { - startRecording(false); - } + try { + recordings.remove(tr); + Path exitPath = exitPaths.remove(tr); + Files.deleteIfExists(exitPath); + log.trace("Deleted temp file {}", exitPath); + } catch (IOException e) { + log.warn("Could not delete temp file", e); + } finally { + startRecording(false); } }); break; From 1cd5b029145bcfd1268b2be41a62a7479244827a Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 16:18:58 -0400 Subject: [PATCH 20/21] ensure correct upload file name --- src/main/java/io/cryostat/agent/CryostatClient.java | 3 +-- src/main/java/io/cryostat/agent/harvest/Harvester.java | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/cryostat/agent/CryostatClient.java b/src/main/java/io/cryostat/agent/CryostatClient.java index 475c4639..b6e28512 100644 --- a/src/main/java/io/cryostat/agent/CryostatClient.java +++ b/src/main/java/io/cryostat/agent/CryostatClient.java @@ -368,8 +368,7 @@ public CompletableFuture upload( Instant start = Instant.now(); String timestamp = start.truncatedTo(ChronoUnit.SECONDS).toString().replaceAll("[-:]", ""); String fileName = - template.map(t -> String.format("%s_%s_%s.jfr", appName, t, timestamp)) - .orElseGet(() -> String.format("%s_%s.jfr", appName, timestamp)); + String.format("%s_%s_%s.jfr", appName, template.orElse("unknown"), timestamp); Map labels = new HashMap<>(Map.of("jvmId", jvmId, "pushType", pushType.name())); template.ifPresent( diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index b3e23007..8fd4eeac 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -381,7 +381,7 @@ private Future uploadRecording(TemplatedRecording tr) throws IOException { Path exitPath = exitPaths.get(tr); return client.upload( PushType.EMERGENCY, - Optional.of(tr.getConfiguration().getName()), + Optional.of(tr.getConfiguration().getName().toLowerCase()), maxFiles, exitPath); } From ba5d8b3be0f88ad544b1b561e06bb63ed5f7f09e Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 3 Oct 2023 16:30:23 -0400 Subject: [PATCH 21/21] fix up upload recording template metadata --- .../io/cryostat/agent/CryostatClient.java | 37 ++++++++++++++++--- .../io/cryostat/agent/harvest/Harvester.java | 15 +------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/main/java/io/cryostat/agent/CryostatClient.java b/src/main/java/io/cryostat/agent/CryostatClient.java index b6e28512..a6d87193 100644 --- a/src/main/java/io/cryostat/agent/CryostatClient.java +++ b/src/main/java/io/cryostat/agent/CryostatClient.java @@ -27,7 +27,6 @@ import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.Collection; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -38,6 +37,7 @@ import java.util.concurrent.Executor; import java.util.function.Function; +import io.cryostat.agent.FlightRecorderHelper.TemplatedRecording; import io.cryostat.agent.WebServer.Credentials; import io.cryostat.agent.harvest.Harvester; import io.cryostat.agent.model.DiscoveryNode; @@ -49,6 +49,8 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; +import jdk.jfr.Configuration; +import jdk.jfr.Recording; import org.apache.commons.io.FileUtils; import org.apache.commons.io.input.CountingInputStream; import org.apache.http.HttpHeaders; @@ -363,16 +365,39 @@ public CompletableFuture update( } public CompletableFuture upload( - Harvester.PushType pushType, Optional template, int maxFiles, Path recording) + Harvester.PushType pushType, + Optional opt, + int maxFiles, + Path recording) throws IOException { Instant start = Instant.now(); String timestamp = start.truncatedTo(ChronoUnit.SECONDS).toString().replaceAll("[-:]", ""); + String template = + opt.map(TemplatedRecording::getConfiguration) + .map(Configuration::getName) + .map(String::toLowerCase) + .map(String::trim) + .orElse("unknown"); String fileName = - String.format("%s_%s_%s.jfr", appName, template.orElse("unknown"), timestamp); + String.format( + "%s_%s_%s.jfr", + appName + + opt.map(TemplatedRecording::getRecording) + .map(Recording::getName) + .map(n -> "-" + n) + .orElse(""), + template, + timestamp); Map labels = - new HashMap<>(Map.of("jvmId", jvmId, "pushType", pushType.name())); - template.ifPresent( - t -> labels.putAll(Map.of("template.name", t, "template.type", "TARGET"))); + Map.of( + "jvmId", + jvmId, + "pushType", + pushType.name(), + "template.name", + template, + "template.type", + "TARGET"); HttpPost req = new HttpPost(baseUri.resolve("/api/beta/recordings/" + jvmId)); diff --git a/src/main/java/io/cryostat/agent/harvest/Harvester.java b/src/main/java/io/cryostat/agent/harvest/Harvester.java index 8fd4eeac..558a7635 100644 --- a/src/main/java/io/cryostat/agent/harvest/Harvester.java +++ b/src/main/java/io/cryostat/agent/harvest/Harvester.java @@ -39,7 +39,6 @@ import io.cryostat.agent.util.StringUtils; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; -import jdk.jfr.Configuration; import jdk.jfr.FlightRecorder; import jdk.jfr.FlightRecorderListener; import jdk.jfr.Recording; @@ -354,13 +353,7 @@ private Future uploadOngoing(PushType pushType, RecordingSettings settings Files.write(exitPath, new byte[0], StandardOpenOption.TRUNCATE_EXISTING); recording.dump(exitPath); log.trace("Dumping {}({}) to {}", recording.getName(), recording.getId(), exitPath); - return client.upload( - pushType, - sownRecording - .map(TemplatedRecording::getConfiguration) - .map(Configuration::getName), - maxFiles, - exitPath) + return client.upload(pushType, sownRecording, maxFiles, exitPath) .thenRun( () -> { try { @@ -379,11 +372,7 @@ private Future uploadOngoing(PushType pushType, RecordingSettings settings private Future uploadRecording(TemplatedRecording tr) throws IOException { Path exitPath = exitPaths.get(tr); - return client.upload( - PushType.EMERGENCY, - Optional.of(tr.getConfiguration().getName().toLowerCase()), - maxFiles, - exitPath); + return client.upload(PushType.EMERGENCY, Optional.of(tr), maxFiles, exitPath); } public enum PushType {