Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow for the deletion of workflow reports #8156

Merged
merged 11 commits into from
Nov 6, 2024
Merged

Allow for the deletion of workflow reports #8156

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
31babbe
Allow for the deletion of workflow reports
frcroth Oct 30, 2024
5ad996b
Update Changelog
frcroth Oct 30, 2024
fd64a57
Merge branch 'master' into delete-workflow-reports
frcroth Oct 30, 2024
ba82913
Add note on deletion
frcroth Nov 4, 2024
c26f05d
not tested: only show delete workflow report entry for superusers
dieknolle3333 Nov 4, 2024
856f9c6
improve syntax
dieknolle3333 Nov 4, 2024
9f65900
lint
dieknolle3333 Nov 4, 2024
0eaf018
Merge branch 'master' into delete-workflow-reports
frcroth Nov 4, 2024
54037c7
Apply suggestions from code review
frcroth Nov 4, 2024
443b71c
Ensure org for setting hash to null in jobs table
frcroth Nov 4, 2024
114ff78
Merge branch 'master' into delete-workflow-reports
frcroth Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Added a button to the search popover in the skeleton and segment tab to select all matching non-group results. [#8123](https://github.com/scalableminds/webknossos/pull/8123)
- Unified wording in UI and code: “Magnification”/“mag” is now used in place of “Resolution“ most of the time, compare [https://docs.webknossos.org/webknossos/terminology.html](terminology document). [#8111](https://github.com/scalableminds/webknossos/pull/8111)
- Added support for adding remote OME-Zarr NGFF version 0.5 datasets. [#8122](https://github.com/scalableminds/webknossos/pull/8122)
- Workflow reports may be deleted by superusers. [#8156](https://github.com/scalableminds/webknossos/pull/8156)

### Changed
- Some mesh-related actions were disabled in proofreading-mode when using meshfiles that were created for a mapping rather than an oversegmentation. [#8091](https://github.com/scalableminds/webknossos/pull/8091)
Expand Down
13 changes: 13 additions & 0 deletions app/controllers/VoxelyticsController.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package controllers
import com.scalableminds.util.time.Instant
import com.scalableminds.util.tools.{Fox, FoxImplicits}
import models.organization.OrganizationDAO
import models.user.UserService
import models.voxelytics._
import play.api.libs.json._
import play.api.mvc._
Expand All @@ -19,6 +20,7 @@ class VoxelyticsController @Inject()(
organizationDAO: OrganizationDAO,
voxelyticsDAO: VoxelyticsDAO,
voxelyticsService: VoxelyticsService,
userService: UserService,
lokiClient: LokiClient,
wkConf: WkConf,
sil: Silhouette[WkEnv])(implicit ec: ExecutionContext, bodyParsers: PlayBodyParsers)
Expand Down Expand Up @@ -158,6 +160,17 @@ class VoxelyticsController @Inject()(
} yield JsonOk(result)
}

def deleteWorkflow(workflowHash: String): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
_ <- bool2Fox(wkConf.Features.voxelyticsEnabled) ?~> "voxelytics.disabled"
_ <- userService.assertIsSuperUser(request.identity)
_ <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
_ = logger.info(s"Deleting workflow with hash $workflowHash in organization ${request.identity._organization}")
_ <- voxelyticsDAO.deleteWorkflow(workflowHash, request.identity._organization)
} yield Ok
}
Comment on lines +163 to +172
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add validation for running workflows and improve error handling

The implementation needs several improvements for robustness:

  1. As discussed in the PR objectives, we should prevent deletion of running workflows or at least warn users
  2. The deletion operation should be wrapped in error handling
  3. Consider using a transaction to ensure atomicity of the deletion operation

Here's a suggested implementation:

 def deleteWorkflow(workflowHash: String): Action[AnyContent] =
   sil.SecuredAction.async { implicit request =>
     for {
       _ <- bool2Fox(wkConf.Features.voxelyticsEnabled) ?~> "voxelytics.disabled"
       _ <- userService.assertIsSuperUser(request.identity)
-      _ <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
+      workflow <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
+      isRunning <- voxelyticsDAO.isWorkflowRunning(workflowHash)
+      _ <- bool2Fox(!isRunning) ?~> "voxelytics.cannotDeleteRunningWorkflow" ~> CONFLICT
       _ = logger.info(s"Deleting workflow with hash $workflowHash in organization ${request.identity._organization}")
-      _ <- voxelyticsDAO.deleteWorkflow(workflowHash, request.identity._organization)
+      result <- Fox.runAndHandleErrors {
+        voxelyticsDAO.inTransaction { implicit session =>
+          voxelyticsDAO.deleteWorkflow(workflowHash, request.identity._organization)
+        }
+      }
+      _ = if (result.isLeft) {
+        logger.error(s"Failed to delete workflow $workflowHash: ${result.left.get}")
+      }
+      _ <- bool2Fox(result.isRight) ?~> "voxelytics.deletionFailed" ~> INTERNAL_SERVER_ERROR
     } yield Ok
   }

Committable suggestion skipped: line range outside the PR's diff.


def storeWorkflowEvents(workflowHash: String, runName: String): Action[List[WorkflowEvent]] =
sil.SecuredAction.async(validateJson[List[WorkflowEvent]]) { implicit request =>
def createWorkflowEvent(runId: ObjectId, events: List[WorkflowEvent]): Fox[Unit] =
Expand Down
14 changes: 14 additions & 0 deletions app/models/voxelytics/VoxelyticsDAO.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1135,4 +1135,18 @@ class VoxelyticsDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContex
""".asUpdate)
} yield ()

def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
for {
_ <- run(q"""
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash
AND _organization = $organizationId;
""".asUpdate)
_ <- run(q"""
UPDATE webknossos.jobs
SET _voxelytics_workflowHash = NULL
WHERE _voxelytics_workflowHash = $hash;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, we will have to check here too that this happens only for jobs of this orga (as identified by the _owner reference in the jobs table). Sorry that I missed this in the first round

""".asUpdate)
} yield ()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Wrap database operations in a transaction for data consistency.

The workflow deletion involves multiple database operations that should be atomic to maintain data consistency. If the first operation succeeds but the second fails, it could leave the database in an inconsistent state.

Apply this diff to wrap operations in a transaction:

def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
  for {
-   _ <- run(q"""
-                 DELETE FROM webknossos.voxelytics_workflows
-                 WHERE hash = $hash
-                 AND _organization = $organizationId;
-                 """.asUpdate)
-   _ <- run(q"""
-                 UPDATE webknossos.jobs
-                 SET _voxelytics_workflowHash = NULL
-                 WHERE _voxelytics_workflowHash = $hash;
-       """.asUpdate)
+   _ <- sqlClient.withTransaction { implicit client =>
+     for {
+       _ <- run(q"""
+         DELETE FROM webknossos.voxelytics_workflows
+         WHERE hash = $hash
+         AND _organization = $organizationId;
+         """.asUpdate)
+       _ <- run(q"""
+         UPDATE webknossos.jobs
+         SET _voxelytics_workflowHash = NULL
+         WHERE _voxelytics_workflowHash = $hash;
+         """.asUpdate)
+     } yield ()
+   }
  } yield ()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
for {
_ <- run(q"""
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash
AND _organization = $organizationId;
""".asUpdate)
_ <- run(q"""
UPDATE webknossos.jobs
SET _voxelytics_workflowHash = NULL
WHERE _voxelytics_workflowHash = $hash;
""".asUpdate)
} yield ()
def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
for {
_ <- sqlClient.withTransaction { implicit client =>
for {
_ <- run(q"""
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash
AND _organization = $organizationId;
""".asUpdate)
_ <- run(q"""
UPDATE webknossos.jobs
SET _voxelytics_workflowHash = NULL
WHERE _voxelytics_workflowHash = $hash;
""".asUpdate)
} yield ()
}
} yield ()

🛠️ Refactor suggestion

Consider implementing cascade deletes for associated data.

The current implementation only handles the workflow record and job references. Consider adding cascade deletes for:

  • Associated runs and their data
  • Related artifacts
  • Other dependent data

This will prevent orphaned data in the database.

Apply this diff to add cascade deletes:

def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
  for {
    _ <- sqlClient.withTransaction { implicit client =>
      for {
+       // Delete associated runs and their data first
+       _ <- run(q"""
+         DELETE FROM webknossos.voxelytics_artifacts a
+         USING webknossos.voxelytics_tasks t,
+               webknossos.voxelytics_runs r
+         WHERE a._task = t._id
+         AND t._run = r._id
+         AND r.workflow_hash = $hash
+         AND r._organization = $organizationId;
+         """.asUpdate)
+       _ <- run(q"""
+         DELETE FROM webknossos.voxelytics_tasks t
+         USING webknossos.voxelytics_runs r
+         WHERE t._run = r._id
+         AND r.workflow_hash = $hash
+         AND r._organization = $organizationId;
+         """.asUpdate)
+       _ <- run(q"""
+         DELETE FROM webknossos.voxelytics_runs
+         WHERE workflow_hash = $hash
+         AND _organization = $organizationId;
+         """.asUpdate)
        _ <- run(q"""
          DELETE FROM webknossos.voxelytics_workflows
          WHERE hash = $hash
          AND _organization = $organizationId;
          """.asUpdate)
        _ <- run(q"""
          UPDATE webknossos.jobs
          SET _voxelytics_workflowHash = NULL
          WHERE _voxelytics_workflowHash = $hash;
          """.asUpdate)
      } yield ()
    }
  } yield ()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
for {
_ <- run(q"""
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash
AND _organization = $organizationId;
""".asUpdate)
_ <- run(q"""
UPDATE webknossos.jobs
SET _voxelytics_workflowHash = NULL
WHERE _voxelytics_workflowHash = $hash;
""".asUpdate)
} yield ()
def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
for {
_ <- sqlClient.withTransaction { implicit client =>
for {
// Delete associated runs and their data first
_ <- run(q"""
DELETE FROM webknossos.voxelytics_artifacts a
USING webknossos.voxelytics_tasks t,
webknossos.voxelytics_runs r
WHERE a._task = t._id
AND t._run = r._id
AND r.workflow_hash = $hash
AND r._organization = $organizationId;
""".asUpdate)
_ <- run(q"""
DELETE FROM webknossos.voxelytics_tasks t
USING webknossos.voxelytics_runs r
WHERE t._run = r._id
AND r.workflow_hash = $hash
AND r._organization = $organizationId;
""".asUpdate)
_ <- run(q"""
DELETE FROM webknossos.voxelytics_runs
WHERE workflow_hash = $hash
AND _organization = $organizationId;
""".asUpdate)
_ <- run(q"""
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash
AND _organization = $organizationId;
""".asUpdate)
_ <- run(q"""
UPDATE webknossos.jobs
SET _voxelytics_workflowHash = NULL
WHERE _voxelytics_workflowHash = $hash;
""".asUpdate)
} yield ()
}
} yield ()

}
1 change: 1 addition & 0 deletions conf/webknossos.latest.routes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ POST /verifyEmail
POST /voxelytics/workflows controllers.VoxelyticsController.storeWorkflow()
GET /voxelytics/workflows controllers.VoxelyticsController.listWorkflows()
GET /voxelytics/workflows/:workflowHash controllers.VoxelyticsController.getWorkflow(workflowHash: String, runId: Option[String])
DELETE /voxelytics/workflows/:workflowHash controllers.VoxelyticsController.deleteWorkflow(workflowHash: String)
POST /voxelytics/workflows/:workflowHash/events controllers.VoxelyticsController.storeWorkflowEvents(workflowHash: String, runName: String)
GET /voxelytics/workflows/:workflowHash/chunkStatistics controllers.VoxelyticsController.getChunkStatistics(workflowHash: String, runId: Option[String], taskName: String)
GET /voxelytics/workflows/:workflowHash/artifactChecksums controllers.VoxelyticsController.getArtifactChecksums(workflowHash: String, runId: Option[String], taskName: String, artifactName: Option[String])
Expand Down
6 changes: 6 additions & 0 deletions frontend/javascripts/admin/admin_rest_api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2501,6 +2501,12 @@ export function getVoxelyticsArtifactChecksums(
);
}

export function deleteWorkflow(workflowHash: string): Promise<void> {
return Request.triggerRequest(`/api/voxelytics/workflows/${workflowHash}`, {
method: "DELETE",
});
}

// ### Help / Feedback userEmail
export function sendHelpEmail(message: string) {
return Request.receiveJSON(
Expand Down
32 changes: 31 additions & 1 deletion frontend/javascripts/admin/voxelytics/task_list_view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,12 @@ import TaskView from "./task_view";
import { formatLog } from "./log_tab";
import { addAfterPadding, addBeforePadding } from "./utils";
import { LOG_LEVELS } from "oxalis/constants";
import { getVoxelyticsLogs } from "admin/admin_rest_api";
import { getVoxelyticsLogs, deleteWorkflow } from "admin/admin_rest_api";
import ArtifactsDiskUsageList from "./artifacts_disk_usage_list";
import { notEmpty } from "libs/utils";
import type { ArrayElement } from "types/globals";
import { useSelector } from "react-redux";
import type { OxalisState } from "oxalis/store";

const { Search } = Input;

Expand Down Expand Up @@ -272,6 +274,8 @@ export default function TaskListView({
const highlightedTask = params.highlightedTask || "";
const location = useLocation();

const isCurrentUserSuperUser = useSelector((state: OxalisState) => state.activeUser?.isSuperUser);

const singleRunId = report.runs.length === 1 ? report.runs[0].id : runId;

useEffect(() => {
Expand Down Expand Up @@ -421,6 +425,26 @@ export default function TaskListView({
}
}

async function deleteWorkflowReport() {
await modal.confirm({
title: "Delete Workflow Report",
content:
"Are you sure you want to delete this workflow report? This can not be undone. Note that if the workflow is still running, this may cause it to fail.",
okText: "Delete",
okButtonProps: { danger: true },
onOk: async () => {
try {
await deleteWorkflow(report.workflow.hash);
history.push("/workflows");
message.success("Workflow report deleted.");
} catch (error) {
console.error(error);
message.error("Could not delete workflow report.");
}
},
});
}

const overflowMenu: MenuProps = {
items: [
{ key: "1", onClick: copyAllArtifactPaths, label: "Copy All Artifact Paths" },
Expand All @@ -442,6 +466,12 @@ export default function TaskListView({
{ key: "5", onClick: showArtifactsDiskUsageList, label: "Show Disk Usage of Artifacts" },
],
};
if (isCurrentUserSuperUser)
overflowMenu.items?.push({
key: "6",
onClick: deleteWorkflowReport,
label: "Delete Workflow Report",
});

type ItemType = ArrayElement<CollapseProps["items"]>;

Expand Down