You are viewing a plain text version of this content. The canonical link for it is here.
Posted to custos@airavata.apache.org by "DImuthuUpe (via GitHub)" <gi...@apache.org> on 2025/11/28 22:54:44 UTC
[PR] Adding support to clean up experiment working directory on cluster [airavata]
DImuthuUpe opened a new pull request, #582:
URL: https://github.com/apache/airavata/pull/582
Added a clean up flag to experiment. This will enable cleaning up the working directory on cluster based on predifined conditions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: custos-unsubscribe@airavata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to clean up experiment working directory on cluster [airavata]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #582:
URL: https://github.com/apache/airavata/pull/582#discussion_r2572637710
##########
airavata-api/src/main/java/org/apache/airavata/helix/impl/task/completing/CompletingTask.java:
##########
@@ -39,6 +41,23 @@ public TaskResult onRun(TaskHelper helper, TaskContext taskContext) {
logger.info("Process " + getProcessId() + " successfully completed");
saveAndPublishProcessStatus(ProcessState.COMPLETED);
cleanup();
+
+ try {
+ if (getExperimentModel().getCleanUpStrategy() == ExperimentCleanupStrategy.ALWAYS) {
+ AgentAdaptor adaptor = helper
+ .getAdaptorSupport()
+ .fetchAdaptor(
+ getTaskContext().getGatewayId(),
+ getTaskContext().getComputeResourceId(),
+ getTaskContext().getJobSubmissionProtocol(),
+ getTaskContext().getComputeResourceCredentialToken(),
+ getTaskContext().getComputeResourceLoginUserName());
+ logger.info("Cleaning up the working directory {}", taskContext.getWorkingDir());
+ adaptor.deleteDirectory(getTaskContext().getWorkingDir());
+ }
+ } catch (Exception e) {
+ logger.error("Failed clean up experiment " + getExperimentId(), e);
Review Comment:
The error message states "Failed clean up experiment" but should say "Failed to clean up experiment" for proper grammar.
```suggestion
logger.error("Failed to clean up experiment " + getExperimentId(), e);
```
##########
airavata-api/src/main/java/org/apache/airavata/helix/agent/ssh/SshAgentAdaptor.java:
##########
@@ -182,6 +182,40 @@ public void createDirectory(String path, boolean recursive) throws AgentExceptio
}
}
+ @Override
+ public void deleteDirectory(String path) throws AgentException {
+ String command = "rm -rf " + path;
+ ChannelExec channelExec = null;
+ try {
+ channelExec = (ChannelExec) session.openChannel("exec");
+ StandardOutReader stdOutReader = new StandardOutReader();
+
+ channelExec.setCommand(command);
+ InputStream out = channelExec.getInputStream();
+ InputStream err = channelExec.getErrStream();
+ channelExec.connect();
+
+ stdOutReader.readStdOutFromStream(out);
+ stdOutReader.readStdErrFromStream(err);
+
+ if (stdOutReader.getStdError() != null && stdOutReader.getStdError().contains("mkdir:")) {
Review Comment:
The error check is looking for "mkdir:" in the standard error output, but this is a delete operation using `rm -rf`. This appears to be copy-pasted from the `createDirectory` method above. The error check should either be removed or updated to check for appropriate `rm` error patterns.
```suggestion
if (stdOutReader.getStdError() != null && stdOutReader.getStdError().contains("rm:")) {
```
##########
airavata-api/src/main/java/org/apache/airavata/agents/api/StorageResourceAdaptor.java:
##########
@@ -41,6 +41,8 @@ public void downloadFile(String remoteFile, OutputStream localOutStream, FileMet
public void createDirectory(String path, boolean recursive) throws AgentException;
+ public void deleteDirectory(String path) throws AgentException;
Review Comment:
This method overrides [AgentAdaptor.deleteDirectory](1); it is advisable to add an Override annotation.
##########
airavata-api/src/main/java/org/apache/airavata/helix/adaptor/SSHJAgentAdaptor.java:
##########
@@ -277,6 +277,30 @@ public void createDirectory(String path, boolean recursive) throws AgentExceptio
}
}
+ @Override
+ public void deleteDirectory(String path) throws AgentException {
+ SFTPClientWrapper sftpClient = null;
+ try {
+ sftpClient = sshjClient.newSFTPClientWrapper();
+ sftpClient.rmdir(path);
Review Comment:
The `SSHJAgentAdaptor.deleteDirectory` uses `sftpClient.rmdir(path)` which only removes empty directories. This will fail if the working directory contains files or subdirectories. The implementation should recursively delete the directory contents, similar to how `SshAgentAdaptor` uses `rm -rf`. Consider using a recursive deletion approach or executing a remote command like `rm -rf`.
##########
airavata-api/src/main/java/org/apache/airavata/helix/agent/ssh/SshAgentAdaptor.java:
##########
@@ -182,6 +182,40 @@ public void createDirectory(String path, boolean recursive) throws AgentExceptio
}
}
+ @Override
+ public void deleteDirectory(String path) throws AgentException {
+ String command = "rm -rf " + path;
Review Comment:
Missing space between "rm -rf" and the path parameter. While the shell will execute this correctly, it's a style inconsistency. Should be `"rm -rf " + path` for proper spacing.
```suggestion
String command = "rm -rf " + path;
```
##########
.devcontainer/database_scripts/init/07-cleanup-strategy-migration.sql:
##########
@@ -0,0 +1,4 @@
+USE experiment_catalog;
+
+-- Add cleanAfterStaged flag to APPLICATION_INTERFACE
Review Comment:
The comment describes adding a `cleanAfterStaged` flag, but the actual column being added is `CLEANUP_STRATEGY`. The comment should be updated to accurately reflect what's being added.
```suggestion
-- Add CLEANUP_STRATEGY column to EXPERIMENT table
```
##########
airavata-api/src/main/java/org/apache/airavata/helix/impl/task/TaskContext.java:
##########
@@ -176,6 +178,10 @@ public void setProcessModel(ProcessModel processModel) {
this.processModel = processModel;
}
+ public void setExperimentModel(ExperimentModel experimentModel) {
+ this.experimentModel = experimentModel;
+ }
+
Review Comment:
The `TaskContext` class has a setter for `experimentModel` but no corresponding getter method. This prevents other classes from accessing the experiment model through the task context. A `getExperimentModel()` method should be added to complete the API.
```suggestion
public ExperimentModel getExperimentModel() {
return experimentModel;
}
```
##########
airavata-api/src/main/java/org/apache/airavata/helix/agent/ssh/SshAgentAdaptor.java:
##########
@@ -182,6 +182,40 @@ public void createDirectory(String path, boolean recursive) throws AgentExceptio
}
}
+ @Override
+ public void deleteDirectory(String path) throws AgentException {
+ String command = "rm -rf " + path;
Review Comment:
The `rm -rf` command is executed without proper path sanitization or validation. If the path contains special characters or is maliciously crafted, it could lead to unintended deletions. Consider validating the path or using proper shell escaping before executing the command.
##########
airavata-api/src/main/java/org/apache/airavata/helix/impl/task/completing/CompletingTask.java:
##########
@@ -39,6 +41,23 @@ public TaskResult onRun(TaskHelper helper, TaskContext taskContext) {
logger.info("Process " + getProcessId() + " successfully completed");
saveAndPublishProcessStatus(ProcessState.COMPLETED);
cleanup();
+
+ try {
+ if (getExperimentModel().getCleanUpStrategy() == ExperimentCleanupStrategy.ALWAYS) {
+ AgentAdaptor adaptor = helper
+ .getAdaptorSupport()
+ .fetchAdaptor(
+ getTaskContext().getGatewayId(),
+ getTaskContext().getComputeResourceId(),
+ getTaskContext().getJobSubmissionProtocol(),
+ getTaskContext().getComputeResourceCredentialToken(),
+ getTaskContext().getComputeResourceLoginUserName());
+ logger.info("Cleaning up the working directory {}", taskContext.getWorkingDir());
+ adaptor.deleteDirectory(getTaskContext().getWorkingDir());
+ }
Review Comment:
The enum defines `ONLY_COMPLETED` and `ONLY_FAILED` strategies, but only `ALWAYS` is implemented in the `CompletingTask`. The `ONLY_COMPLETED` strategy is not checked/handled here (though it would apply to this task), and there's no implementation for `ONLY_FAILED` which should likely be added to the failure handling path (e.g., in `AiravataTask.onFail()`). This incomplete implementation means the feature won't work as documented.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@airavata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org