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

System Console button to manually run a legal hold immediately (#89) #90

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

Conversation

grundleborg
Copy link
Collaborator

@grundleborg grundleborg commented Sep 1, 2024

  • Pressing this button will fail if the legal hold job is executing at the time it is pressed.
  • I'm not sure what happens in terms of preventing duplicate runs across a cluster setup - I need to get to the bottom of what kind of protection this plugin has for that eventuality and how it really works.
  • UI needs some love.
  • No tests (yet)
  • No progress indication in the webapp.

Potentially mutexing and progress indication could be solved by marking holds as "in progress" in the store, but this means if the job dies/fails for any reason they will be stale and stuck in an a state that will prevent further execution, so this really does need some careful thought.

Suggestions for the UI welcome - see screenshots below.

image

image

* Pressing this button will fail if the legal hold job is executing at the
  time it is pressed.
* I'm not sure what happens in terms of preventing duplicate runs across
  a cluster setup - I need to get to the bottom of what kind of
  protection this plugin has for that eventuality and how it really
  works.
* UI needs some love.
* No tests (yet)
* No progress indication in the webapp.

Potentially mutexing and progress indication could be solved by marking
holds as "in progress" in the store, but this means if the job
dies/fails for any reason they will be stale and stuck in an a state
that will prevent further execution, so this really does need some
careful thought.
@grundleborg grundleborg added Do Not Merge Should not be merged until this label is removed Work In Progress Not yet ready for review labels Sep 1, 2024
@grundleborg
Copy link
Collaborator Author

#89

@wiggin77
Copy link
Member

@grundleborg will you be completing this or should I assign it to someone here? We are planning to release 1.0 early next week - would be great to have this.

@grundleborg
Copy link
Collaborator Author

@wiggin77 @jasonblais reviewing the job infrastructure code, this button is robust against simultaneous execution of a legal hold in a single-server Mattermost instance already. However, it is not robust against simultaneous execution in a Mattermost cluster. Does it need to be handle this case? I'm reluctant to add a lot of complexity to the codebase to handle this unless it is absolutely required.

As an aside, the cluster job scheduling API in Mattermost Server really needs an additional method for "ignore the usual schedule and run a job right now". Without that we are left reinventing all the cluster-aware locking infrastructure in individual plugins, which is going to be buggy and painful.

@grundleborg grundleborg added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester and removed Do Not Merge Should not be merged until this label is removed Work In Progress Not yet ready for review labels Oct 5, 2024
@grundleborg
Copy link
Collaborator Author

This is now ready to review, subject to the caveat in the above comment about the possibility if multiple simultaneous runs in a cluster environment.

Let me know if it is critical to handle the cluster case and I can see what hack I can come up with for it.

@grundleborg grundleborg changed the title First pass at manually run legal hold now button. System Console button to manually run a legal hold immediately (#89) Oct 5, 2024
@mattermost-build
Copy link

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@wiggin77
Copy link
Member

it is not robust against simultaneous execution in a Mattermost cluster.

There is a cluster mutex available. See https://github.com/mattermost/mattermost-plugin-channel-export/blob/3e5043f2a5ddaa67652308330b41f31d29fc5460/server/command_hooks.go#L52 for example usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester Lifecycle/1:stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants