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

Add new feature LifeCycle Management #6139

Open
wants to merge 65 commits into
base: develop
Choose a base branch
from

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Sep 5, 2024

Adds lifecycle modules to Opencast.

Lifecycle Management intends to allow users to automate their tasks. By adding them as a "lifecycle policy", Opencast will then execute the specified action at the specified time for the specified objects. For a more detailed explanation see the documentation.

This also adds external api endpoints for the lifecycle management. A UI interface is planned (e.g. in the admin ui) is planned.

Your pull request should…

@Arnei Arnei added feature elan e.V. Pull requests originating from elan e.V. java Pull requests that update Java code labels Sep 5, 2024
@Arnei Arnei changed the title Lifecycle core Add new feature LifeCycle Management Sep 6, 2024
If we have subtitles, we need captions/trimmed flavors for the publication
therefore we have to exclude the captions/trimmed from the tagging in the partial-theming WF.
Arnei added a commit to Arnei/opencast-admin-interface that referenced this pull request Oct 10, 2024
Adds a new tab to recordings called "LifeCycle
Policies". Much like other tabs this tab displays
information on its subject in a table format.
Unlike i.e. the events tab, the LifeCycle Policies
cannot be changed in any way, just be viewed.
Editing is supposed to be added at a later date.

Depends on PR opencast/opencast#6139
being merged to make any sense. Similarly, if you
would like to test this, your admin interface
should point to an Opencast with the PR
installed.
for (EventSearchQueryField<String> date : query.getStartDate()) {
addToQuery(EventIndexSchema.START_DATE,
date.getValue(), date.getType(), date.isMust());
}
Copy link
Member

Choose a reason for hiding this comment

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

Where is the "startDate" coming from that is set in elastic search? Is from the Mediapackge's created or dcCreated values? Or does it come from the event's initial processing workflow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Where the dates are coming from was not something I was concerning myself with. The goal here was to extend the querying capabilities, particularly if a given date as older/earlier than another.

mtneug and others added 6 commits October 24, 2024 01:14
The JWK provider cache can become stale (e.g. upon key rotation). When
using key IDs this can result in an empty algorithm selection. An
exception is thrown, however, the cache is not refreshed. This patch
will force a refresh if no key could be found.
This allows us to use literal `,` and `:` in filters, cf. opencast#1673,
specifically opencast#1673 (comment).

Note that this changes behavior!
Arguably a comma leading to filters being misinterpreted is a bug,
but this "fixes" it by also changing the meaning of certain other filters.
(Think literal `%2C` for example.)

Note also that I had to duplicate the old version of the method used
into the `GroupsEndpoint` of the external API as to not change its behavior.
This makes most (hopefully all) admin UI endpoints' behavior regarding
the `filter` parameter (more) inconsistent with the external API.

Fixing that would be unproportionally more effort in my opinion,
and if I were to do that, I would probably completely revamp
how filters are passed/processed to begin with, because frankly
naicely spliitting a string at `,` was never a good idea.

This way at leaast clicking on a name with a comma in the admin UI
does the right thing. 🤷‍♀️
@KatrinIhler
Copy link
Member

FYI: Branch cut for OC 17 is on November 6!

Copy link
Member

@gregorydlogan gregorydlogan left a comment

Choose a reason for hiding this comment

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

Mainly docs and/or logging issues. A few points for clarification and further thought. IMO does not block merge prior to 17 branch cut, though the Draft status does :)

## Policy Action

For now there is only one action that can be specified, called "START_WORKFLOW". “START_WORKFLOW” takes a workflow
definition ID (and workflow configuration) to define which workflow should be run. For example, in order to retract
Copy link
Member

Choose a reason for hiding this comment

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

What happens if, at the time of execution, the workflow definition doesn't exist? What if the user doesn't have the rights to touch one (or more!) of the events matching the filters?

Copy link
Member Author

Choose a reason for hiding this comment

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

For each event, a so called "policy task" is created. Should the task run into an exception for any reason, like no workflow definition, it will log the exception and set itself to "FAILED" (which effectively means it won't be retried).

The code does not currently check if the user who created the lifecycle policy has access to the filtered events. Maybe it should?

Copy link
Member

Choose a reason for hiding this comment

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

On one hand it might make sense to ensure that a user can't create policies for things they don't have access too, but it's also reasonable to suspect that the time between policy creation and policy execution might be long enough that their roles might change. I'd be tempted to check at both? Definitely need to check at execution time, but it'll probably make more sense to check at schedule time for UI uses...

Regardless, docs for the workflow definition bits, and whatever happens to address the above should be added.

Copy link
Member Author

@Arnei Arnei Nov 5, 2024

Choose a reason for hiding this comment

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

Feels like this requires a concept of "policy ownership" to decide which entities a policy can affect, but that seems not necessarily trivial?

  • Who should own the policy? The creator?
  • Even then, if someone else than the creator with write access to the policy makes changes to it, they could potentially still affect events that they would normally not be allowed to edit. Is that a problem?
  • Should the owner be changeable? What happens if the owner is deleted?

Copy link
Member

Choose a reason for hiding this comment

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

I would avoid creating policy ownership IMO. It raises, as you point out, a whole bunch of questions with no obvious answers. If that means there is more in terms of undefined behaviour (like being able to create policies against events you don't have access to at schedule time) then so be it. Adding ownership here doesn't get us much, especially since we more or less have to check ACLs at runtime regardless. A simple 'does the ACL listed in the policy match the event' is probably enough almost all usecases.


- SPECIFIC_DATE: A fixed date, for example 2024-02-31 at 11:30. This means the policy will be applied exactly once at
that point in time. Useful for example for retracting videos at the end of a semester.
- REPEATING: A periodic date, for example “Every day at 02:00 AM”. This means the policy will be applied to all events
Copy link
Member

Choose a reason for hiding this comment

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

So editing a policy is not allowed then? 'cause otherwise we have to keep track of what's been applied to what, and that might get complicated quickly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Editing is currently allowed, but not always in your best interest. When reaching the specified action date, a policy will create "lifecycle tasks" for each entity (currently only events) matching the filters. The tasks remember the id of the policy that created them, and a policy will not create a new task for an entity that already has a task with the policies id associated with it. Or in short, the same policy won't run on the same entity twice.

Have I not made mention of this in the docs? Seems important to know...

Copy link
Member

Choose a reason for hiding this comment

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

I didn't catch it if you did.

I don't know what your client is looking for, but I would almost be tempted to not allow editing. The fact that it might not be in your best interest is a code smell IMO. Not allowing editing simplifies some edge cases at the very least. Allowing the user to edit a policy with a action date in the past, and reschedule it to be in the future leads to confusion for the user - why hasn't this been reapplied to matching events? And now you have to keep track of which events it's already been applied to and expose that to the user in the UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely see your point.

On the other hand, having to delete and create a new policy everytime you want to make a small change to it is not terribly intuitive. Also I would not want to go through that again just because someone decided that that policy should run a day later or something.

Maybe we could at a sort of "reset" functionality to a policy, which would delete all associated tasks and thus allow the policy to run again on events it has already affected? Food for thought.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the create+delete cycle, that's what UI magic is for :) I wouldn't expect the user to do this, but the backend could require it to simplify the internal logic. Or maybe that just moves the problem to the front end!

A reset would likely also solve the issue, at least on first glance. You've worked on this more than I have, and I'm not picky - just trying to prevent issues in the future when people really start to use this.

@Arnei Arnei marked this pull request as ready for review November 4, 2024 08:06
@Arnei
Copy link
Member Author

Arnei commented Nov 4, 2024

Thanks for all the comments Greg :)

This is an updated version of closed opencast#4417

This adds four improvements to event catalog handling in the External
API

1. Avoid DB query is not necessary.
2. Cache map of organization to list of catalogs instead of always
calculating it.
3. Avoid calling list providers to fetch collection data if this is
later on not used (e.g. all series).
4. Remove collection data from responses for extended metadata.

The last point can be seen as a change to the External API. However, one
can also see this as a bug, as other methods in the class hide the
collection data from extended metadata.

All changes can add up when requesting multiple events with `/events`
especially with many events and series.

### How to test this patch

Performance degradation is most noticeable when there are a lot of
series in Opencast.

1. Create 1000 series (e.g. using
https://github.com/opencast/helper-scripts/tree/master/create-series)
2. Use External API to retrieve event infos and include
`withmetadata=true`
3. Apply patch and repeat 2.
4. Compare the performance of 2. and 3.

### Your pull request should…

* [x] have a concise title
* [ ] [close an accompanying
issue](https://docs.opencast.org/develop/developer/#participate/development-process/#automatically-closing-issues-when-a-pr-is-merged)
if one exists
* [x] [be against the correct
branch](https://docs.opencast.org/develop/developer/development-process#acceptance-criteria-for-patches-in-different-versions)
* [x] include migration scripts and documentation, if appropriate
* [x] pass automated tests
* [x] have a clean commit history
* [x] [have proper commit messages (title and body) for all
commits](https://medium.com/@steveamaza/e028865e5791)
* [ ] explain why it needs to be merged into the legacy branch, if it is
targeting the legacy branch
Copy link
Contributor

github-actions bot commented Nov 4, 2024

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Very basic barely working base code.
Very basic barely working base code.
Adds a service that looks for configuration files for lifecycle
policies in the `etc/lifecyclepolices` directory. This gives admin
another option to add lifecycle policies besides the API.
Some of the values returned for lifeCyclePolicies were in fact
not correct JSON, which would lead to parsing errors. This
should fix that.
This also restructures the returned value from GET /api/lifecyclemanagement/policies
to return some metainfo about the request which is useful to the admin ui.
Arnei and others added 28 commits November 5, 2024 10:17
…ncast into r/15.x

Pull request opencast#6156
Fixes opencast#6138
  Fix non-themed subtitle publications
Due to a bug in Pax Web, the default session timeout for Opencast was
set to 4 minutes instead of 4 hours. This caused users to be logged out
automatically if they do not have an additional remember-me cookie (e.g.
LTI users don't have that). This basically means that users are unable
to watch videos longer than four minutes for which they need
authentication.

This is a temporary fix for opencast#6260
The JWK provider cache can become stale (e.g. upon key rotation). When
using key IDs this can result in an empty algorithm selection. An
exception is thrown, however, the cache is not refreshed. This patch
will force a refresh if no key could be found.

### How to test this patch

1. Configure JWT, including a JWKS endpoint (that includes `kid` for
configured keys)
2. Start Opencast and authenticate using JWTs
3. Rotate keys
4. Try to authenticate using JWTs that are signed using new keys

### Your pull request should…

* [x] have a concise title
* [ ] [close an accompanying
issue](https://docs.opencast.org/develop/developer/#participate/development-process/#automatically-closing-issues-when-a-pr-is-merged)
if one exists
* [x] [be against the correct
branch](https://docs.opencast.org/develop/developer/development-process#acceptance-criteria-for-patches-in-different-versions)
* [x] include migration scripts and documentation, if appropriate
* [x] pass automated tests
* [x] have a clean commit history
* [x] [have proper commit messages (title and body) for all
commits](https://medium.com/@steveamaza/e028865e5791)
* [ ] explain why it needs to be merged into the legacy branch, if it is
targeting the legacy branch
…nto r/16.x

Pull request opencast#6111

  Treat `filter` parameter elements in admin UI APIs as URL encoded
Due to a bug in Pax Web, the default session timeout for Opencast was
set to 4 minutes instead of 4 hours. This caused users to be logged out
automatically if they do not have an additional remember-me cookie (e.g.
LTI users don't have that). This basically means that users are unable
to watch videos longer than four minutes for which they need
authentication.

This is a temporary fix for opencast#6260

### How to test this patch

To test this (check that the value is indeed seconds):

- Set `org.ops4j.pax.web.session.timeout=20`
- Go to http://localhost:8080, make sure to **remove the check mark for
remember me** and log-in
- Wait for 21 seconds
- Go to http://localhost:8080 again
  - You will be logged out and asked to log in again

### Your pull request should…

* [x] have a concise title
* [x] [close an accompanying
issue](https://docs.opencast.org/develop/developer/#participate/development-process/#automatically-closing-issues-when-a-pr-is-merged)
if one exists
* [x] [be against the correct
branch](https://docs.opencast.org/develop/developer/development-process#acceptance-criteria-for-patches-in-different-versions)
* [x] include migration scripts and documentation, if appropriate
* [x] pass automated tests
* [x] have a clean commit history
* [x] [have proper commit messages (title and body) for all
commits](https://medium.com/@steveamaza/e028865e5791)
* [x] explain why it needs to be merged into the legacy branch, if it is
targeting the legacy branch
…ownouts will occur Nov 14 and 21. This should preserve current behvaiour in our PRs.
This patch fixes the problem that in our documentation the scrollbars
sometimes overlay the actual code which is pretty annoying if you want
to copy parts of the code. And also, it doesn't look great :D
This patch removes `opencast-plugin-paella-player-6` from the list of
install-features. The plugin doesn't exist anymore in Opencast 16.
This patch removes `opencast-plugin-paella-player-6` from the list of
install-features. The plugin doesn't exist anymore in Opencast 16.

### Your pull request should…

* [x] have a concise title
* [x] [close an accompanying
issue](https://docs.opencast.org/develop/developer/#participate/development-process/#automatically-closing-issues-when-a-pr-is-merged)
if one exists
* [x] [be against the correct
branch](https://docs.opencast.org/develop/developer/development-process#acceptance-criteria-for-patches-in-different-versions)
* [x] include migration scripts and documentation, if appropriate
* [x] pass automated tests
* [x] have a clean commit history
* [x] [have proper commit messages (title and body) for all
commits](https://medium.com/@steveamaza/e028865e5791)
* [x] explain why it needs to be merged into the legacy branch, if it is
targeting the legacy branch
This patch removes the section about supported Java versions from the
Debian guide. The information is incorrect and we have a dedicated
section for this by now.
This patch removes the section about supported Java versions from the
Debian guide. The information is incorrect and we have a dedicated
section for this by now.

### Your pull request should…

* [x] have a concise title
* [x] [close an accompanying
issue](https://docs.opencast.org/develop/developer/#participate/development-process/#automatically-closing-issues-when-a-pr-is-merged)
if one exists
* [x] [be against the correct
branch](https://docs.opencast.org/develop/developer/development-process#acceptance-criteria-for-patches-in-different-versions)
* [x] include migration scripts and documentation, if appropriate
* [x] pass automated tests
* [x] have a clean commit history
* [x] [have proper commit messages (title and body) for all
commits](https://medium.com/@steveamaza/e028865e5791)
* [x] explain why it needs to be merged into the legacy branch, if it is
targeting the legacy branch
This PR updates the `upload-artifacts` workflow action from v3 to v4. v3
will be deprecated Dec 5, with brownouts occurring sooner. This PR
should not change behaviour in terms of uploaded builds.

This PR should go into `r/15.x` because the v3 will likely stop working
around Dec 5, and this will also prevent hte brownouts (Nov 14 and 21)
from causing unneccessary workflow failures.

### Your pull request should…

* [x] have a concise title
* [ ] ~[close an accompanying
issue](https://docs.opencast.org/develop/developer/#participate/development-process/#automatically-closing-issues-when-a-pr-is-merged)
if one exists~
* [x] [be against the correct
branch](https://docs.opencast.org/develop/developer/development-process#acceptance-criteria-for-patches-in-different-versions)
* [ ] ~include migration scripts and documentation, if appropriate~
* [x] pass automated tests
* [x] have a clean commit history
* [x] [have proper commit messages (title and body) for all
commits](https://medium.com/@steveamaza/e028865e5791)
* [x] explain why it needs to be merged into the legacy branch, if it is
targeting the legacy branch
This patch fixes the problem that in our documentation the scrollbars
sometimes overlay the actual code which is pretty annoying if you want
to copy parts of the code. And also, it doesn't look great :D

<img
src=https://github.com/user-attachments/assets/b1904619-35a2-4a22-8837-d9614c6f730a
width=300 />


### How to test this patch

- Build the docs locally
- The example screenshot is from the [install via RPMs
page](https://docs.opencast.org/develop/admin/#installation/rpm-el/)

### Your pull request should…

* [x] have a concise title
* [x] [close an accompanying
issue](https://docs.opencast.org/develop/developer/#participate/development-process/#automatically-closing-issues-when-a-pr-is-merged)
if one exists
* [x] [be against the correct
branch](https://docs.opencast.org/develop/developer/development-process#acceptance-criteria-for-patches-in-different-versions)
* [x] include migration scripts and documentation, if appropriate
* [x] pass automated tests
* [x] have a clean commit history
* [x] [have proper commit messages (title and body) for all
commits](https://medium.com/@steveamaza/e028865e5791)
* [x] explain why it needs to be merged into the legacy branch, if it is
targeting the legacy branch
This patch links the webinar showing users how to upgrade Opencast if
they are using the RPM repository.
This patch links the webinar showing users how to upgrade Opencast if
they are using the RPM repository.

### Your pull request should…

* [x] have a concise title
* [x] [close an accompanying
issue](https://docs.opencast.org/develop/developer/#participate/development-process/#automatically-closing-issues-when-a-pr-is-merged)
if one exists
* [x] [be against the correct
branch](https://docs.opencast.org/develop/developer/development-process#acceptance-criteria-for-patches-in-different-versions)
* [x] include migration scripts and documentation, if appropriate
* [x] pass automated tests
* [x] have a clean commit history
* [x] [have proper commit messages (title and body) for all
commits](https://medium.com/@steveamaza/e028865e5791)
* [x] explain why it needs to be merged into the legacy branch, if it is
targeting the legacy branch
Gave the parser the interface instead of the implementing class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elan e.V. Pull requests originating from elan e.V. feature java Pull requests that update Java code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants