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.

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

Retention Module #5344

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Retention Module #5344

wants to merge 16 commits into from

Conversation

Sverre-W
Copy link

@Sverre-W Sverre-W commented May 7, 2024

I've ported the retention module from Elsa V2 to V3.

Usage is similar to before:

        elsa
            .UseRetention(options =>
            {
                options.ConfigureCleanupOptions = cleanUp =>
                {
                    cleanUp.TimeToLive = TimeSpan.FromDays(7);
                    cleanUp.SweepInterval = TimeSpan.FromHours(12);
                    cleanUp.WorkflowInstanceFilter = new WorkflowInstanceFilter
                    {
                        WorkflowStatus = WorkflowStatus.Finished
                    };
                };
            })

To be able to change the configured WorkflowInstanceFilter to add the time to live filter, I also made WorkflowInstanceFilter implement IClonable to make a deep clone of the filter.


This change is Reviewable

@jdevillard
Copy link
Contributor

Hello,

thanks for this PR , I think we need to check some elements because the different persistence stores are not managed in the same way as in the v2.

For example, I don't think the WorkflowInstanceStore delete all the dependency logs or events around the instance.

A WorkflowInstanceManager was introduced and used for eg by the Rest Api to delete Instance. This Manager trigger event that are then handle by other services to remove dependency.

@Sverre-W
Copy link
Author

Sverre-W commented Jun 6, 2024

Ah, I missed that! I've changed the job to use the IWorkflowInstanceManager. Now It's similar to how it's done in the WorkflowInstance delete endpoint.

I wonder if there is a better way to combine 2 workflow instance filters, maybe using ICloneable interface is not the best way.

@jayachandra21
Copy link

Hello @Sverre-W,

I do have a same requirement of usage of Retention Module is Elsa 3.0.
So can I contribute if there is any pending activity to complete this task or do you have any tentative plan to merge this PR into master branch.

Thank You!

@Sverre-W
Copy link
Author

@jayachandra21 I've updated it to be current with the main branch.

I also moved the cloning of the WorkflowInstanceFilter to an extension method inside the module, so no code changes outside the Retention module. I think It's ready to be merged now.

@jayachandra21
Copy link

@jayachandra21 I've updated it to be current with the main branch.

I also moved the cloning of the WorkflowInstanceFilter to an extension method inside the module, so no code changes outside the Retention module. I think It's ready to be merged now.

Thank You very much!
is it merged already with Master branch?

@Sverre-W
Copy link
Author

@jayachandra21 I've updated it to be current with the main branch.

I also moved the cloning of the WorkflowInstanceFilter to an extension method inside the module, so no code changes outside the Retention module. I think It's ready to be merged now.

Thank You very much!
is it merged already with Master branch?

No not yet, this I can't do myself.

@sfmskywalker
Copy link
Member

@Sverre-W Thank you for the initiative! In general, we definitely want this module. There are some good suggestions from @raymonddenhaan that would be good to review. In addition, I would like to see the following enhancement:

Currently, we're doing a bulk delete on workflow instances that match the filter. However, it would be good to have the option of customizing the cleanup job with something else, like for instance: archiving the workflow instances and related data by sending it to cheap storage, like Elastic Search or a local DB file.

To that end, it might be good to introduce a strategy for the cleanup job, e.g. IRetentionCleanupStrategy and have a default implementation called e.g. DeleteCleanupStrategy that performs the bulk delete that you have now. Later, we, or the application developer himself, can then implement an archiving strategy.

What do you think?

@Sverre-W
Copy link
Author

Sverre-W commented Jul 5, 2024

Sounds good, I'll make the requested changes. Do you have in mind a contract for IRetentionCleanupStrategy

Something along the lines of:

interface IRetentionCleanupStrategy {
    public Task Handle(IWorkflowInstanceFilter filter, CancellationToken CancellationToken);
}

Or should we already load the workflow instances that match the filter? In this case, we would need to have something that calls IRetentionCleanupStrategy in paged batches.

@sfmskywalker
Copy link
Member

Awesome! I believe the cleanup strategies should be responsible only for handling the items they receive. This means the hosted service will manage the querying and batching, while the cleanup strategy processes each batch (deleting, archiving, etc.).

The hosted service should load not only the workflow instance to be cleaned up but also related entities such as workflow execution log records, activity execution records, bookmarks, and potentially third-party tables dependent on a workflow instance.

The challenge may be implementing support for third-party modules that want to hook into the cleanup process. Currently, we have the following known entities eligible for cleanup:

  • WorkflowInstance
  • WorkflowExecutionLogRecord
  • ActivityExecutionRecord
  • Bookmark

Ideally, we should design an API that also accommodates third-party entities, for example:

  • CustomWorkflowInstanceLookup (an entity that links a customer to a workflow instance)

One solution could be to raise events and let individual handlers perform the cleanup. This approach would allow third-party modules to handle the cleanup event as well.

These handlers should ideally receive the objects to be cleaned up without having to query them first. This would reduce code repetition when supporting both deletion and archiving to Elasticsearch and other platforms. Furthermore, this approach would enable scenarios where data is sent to multiple destinations.

Perhaps this could be handled nicely with generic type interfaces, for example:

/// Gets entities related to a given batch of workflow instances.
interface IRelatedEntityCollector<TEntity>
{
   // Returns an async enumerable stream of entities to be cleaned up.
   IAsyncEnumerable<TEntity> GetRelatedEntities(ICollection<WorkflowInstance> workflowInstances);
}

class BookmarkCollector : IRelatedEntityCollector<Bookmark>
{
   IAsyncEnumerable<Bookmark> GetRelatedEntities(ICollection<WorkflowInstance> workflowInstances)
   {
      var workflowInstanceIds = workflowInstances.Select(x => x.Id).ToList();
      
      // Load bookmarks in batches
      // For each batch:
      yield return bookmark;
   }   
}

// A strategy responsible for cleaning up a set of entities.
interface ICleanupStrategy<TEntity>
{
   Task Cleanup(ICollection<TEntity> entities);
}

class DeleteBookmarkCleanupStrategy : ICleanupStrategy<Bookmark>
{
   Task Cleanup(ICollection<Bookmark> entities)
   {
      // Delete bookmarks
   }
}

class ElasticsearchBookmarkCleanupStrategy : ICleanupStrategy<Bookmark>
{
   Task Cleanup(ICollection<Bookmark> entities)
   {
      // Send bookmarks to Elasticsearch
   }
}

These are just initial ideas that serve as an illustration and may not be entirely viable. But please consider them and see if they capture the outlined requirements, which, I think, should be:

  • Having an extensible cleanup process that supports cleaning up related entities and enables third-party modules to hook into it (so they can clean up associated entities).
  • Having an extensible approach to what happens when something gets "cleaned up," e.g., delete, send to Elasticsearch, or other actions.

We might start simple by just sending a notification and letting each handler in the enabled modules figure out what to do. We can refine the API from there if we start to see too much code repetition.

@Sverre-W
Copy link
Author

Sverre-W commented Jul 5, 2024

This approach might work well. I will also check if this approach can be used if we would introduce a RetentionPolicy which would be a combination of a IRetentionInstanceFilter and a ICleanupStrategy. This would allow retaining different workflows at different intervals or moving some workflow instances to an archive while deleting others.

@sfmskywalker
Copy link
Member

I just stumbled upon this old thread that may be interesting to consider as part of this PR: #412

Even if not in one go, it could be part of one of the next steps.

@sfmskywalker
Copy link
Member

I like the retention policy idea 👍

@Sverre-W
Copy link
Author

Sverre-W commented Jul 5, 2024

If we allow a custom clean-up strategy which in the end does not enforce the deletion of workflow, we could have some scenarios that are not really retention focused.

For example, I could create a clean-up strategy that just cancels a workflow and use a filter that targets long running suspended workflows.

I wonder if deleting a workflow is some concrete job that you would schedule using Elsa scheduling module. Maybe where you have some generic helpers around the principle of scheduling a job that filters on some workflows and then does some transformation on the filtered workflows, like deleting, archiving, cancelling, restarting, ....

Copy link

gitguardian bot commented Aug 10, 2024

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
13223926 Triggered Generic Password e2014d3 test/component/Elsa.Workflows.ComponentTests/Helpers/Fixtures/Infrastructure.cs View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@Sverre-W
Copy link
Author

@sfmskywalker, I've updated the module with the following usage:

elsa.UseRetention(retention =>
{
    retention.ConfigureCleanupOptions = options =>
    {
        options.SweepInterval = TimeSpan.FromHours(4);
        options.PageSize = 25;
    };

    retention.AddDeletePolicy("Delete completed workflows", sp =>
    {
        ISystemClock systemClock = sp.GetRequiredService<ISystemClock>();
        RetentionWorkflowInstanceFilter filter = new()
        {
            TimestampFilters = new List<TimestampFilter>(),
            WorkflowStatus = WorkflowStatus.Finished
        };

        filter.TimestampFilters.Add(
            new TimestampFilter
            {
                Column = nameof(WorkflowInstance.CreatedAt),
                Operator = TimestampFilterOperator.LessThanOrEqual,
                Timestamp = systemClock.UtcNow.Subtract(TimeSpan.FromDays(1))
            }
        );
        
        return filter;
    });
});

To implement the policy concept, I used a marker interface to determine which ICleanupStrategy<T> to apply. For example, the DeleteRetentionPolicy uses IDeletionCleanupStrategy to identify all ICleanupStrategy<> implementations for deleting workflows and related items.

You can view the implementation here: https://github.com/Sverre-W/elsa-core/blob/f7bb304643a281d23c1253fffe32741ec87f8efd/src/modules/Elsa.Retention/Policies/DeletionRetentionPolicy.cs#L9-L21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants