Skip to content

Conversation

@davelopez
Copy link
Contributor

@davelopez davelopez commented Apr 26, 2023

Implementation of #14771

Depends on #16075 and #16143

UI

There is a new option on the histories menu to let you "archive" a history (moving it out of your active histories)

image

When Archiving a history, depending on the server configuration, we are presented with 2 options, one to Keep the storage space and another to free the storage space taken by the history.

Keep storage space (option A)

image

This option will just mark the history as being Archived and will no longer be shown in the regular histories listing or the "History Selector".
Some characteristics of this kind of archived histories:

  • The contents of the history will remain as they are.
  • The UI will not allow certain operations on the history (the API will still allow them until a Read Only mode for histories is implemented on the backend as a future feature)
  • The history can be restored at any moment, effectively moving it back to your active histories.

Free storage space (option B)

image

This option will allow the user to package & export the history to a permanent remote source as a backup snapshot and then purge the history and its contents to free up storage space.

After generating the export record the user should acknowledge that the original history will be permanently deleted and then proceed to archive it.

image

Some characteristics of this kind of archived histories:

  • If there is already a recent (up-to-date) export record to a permanent file source you don't need to generate another one.
  • These histories are purged. So they are read-only by definition.
  • You can always import a new copy out of the associated export record in case you want to restore it, this will import back the backup snapshot that you associated when archiving it.

Archived Histories

Just a simple list where you can explore your archived histories and restore or reimport them as necessary.

image


Backend

The backend implementation builds on top of #14839, this allows to optionally associate an export record at the time of archival so the actual contents of the history can be purged.

Only export records targeting a permanent file source are considered for archival backup.

Changes in the database

  • The history table now contains 2 new columns:
    • archived: just a flag to indicate that this history is no longer part of the active histories of the user.
    • archive_export_id: the optional ID of an export record that contains a persisted backup snapshot of the history.

New API routes

  • GET /api/histories/archived: returns the list of archived histories of the user. Supports filtering and pagination as other similar endpoints.
  • POST /api/histories/{history_id}/archive: marks a history as archived. Optionally it can associate an export record and purge the history.
  • PUT /api/histories/{history_id}/archive/restore: will restore the history (unarchive, move it back to your active histories). Purged histories are not restored by default but you can still "force" the unarchival by using the force=true query parameter. It will be then like one of your regular purged histories.

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@hexylena
Copy link
Member

hexylena commented May 2, 2023

This is fantastic! looks like it addresses both of the main use cases, how exciting!

@davelopez davelopez force-pushed the add_history_archiving branch 4 times, most recently from 8ae31f9 to 694da99 Compare May 11, 2023 16:40
@jdavcs
Copy link
Member

jdavcs commented May 12, 2023

@davelopez so here's what's happening with the migrations error. There are two. The first one (the one we discussed) happens because of the index parameter is ignored (ref: see gray box). A separate statement is needed.

Once that one is fixed, another error is raised, that one is SQLite-specific: handling foreign keys in batch operations mode requires separate statements (here's a detailed discussion).

So, my suggestions are as follows:

  1. I think separating this into 2 revisions might be a little cleaner, given the constraints? (that's how I tested, but having everything in 1 file should work too)
  2. For the archived column:
def upgrade():
    with transaction():
        add_column(
            table_name,
            sa.Column(column_name, sa.Boolean(), default=False, server_default=sa.false()),
        )
        create_index(index_name, table_name, [column_name])

def downgrade():
    with transaction():
        drop_index(index_name, table_name)
        drop_column(table_name, column_name)
  1. For the archive_export_id column:
fk_name = f"fk_{table_name}_{column_name}"

column = sa.Column(
    column_name,
    sa.Integer,
    nullable=True,
    default=None,
)

def upgrade():
    with transaction():
        add_column(table_name, column)
        create_foreign_key(fk_name, table_name, ref_table_name, [column_name], ["id"])

def downgrade():
    with transaction():
        drop_constraint(fk_name, table_name)
        drop_column(table_name, column_name)

(We definitely need an automated naming convention - to ensure that the indexes and constraints automatically created from our model definitions match those created via migrations. It's on my todo list.)

@davelopez davelopez force-pushed the add_history_archiving branch from 694da99 to f3ac881 Compare May 12, 2023 09:56
@davelopez
Copy link
Contributor Author

davelopez commented May 12, 2023

Thank you again for all the help @jdavcs!

I think separating this into 2 revisions might be a little cleaner, given the constraints? (that's how I tested, but having everything in 1 file should work too)

I decided to keep both in the same revision file, but I'm happy to split them if this is important. The updated commit is 110361d.

Update: looks like I'm still doing something wrong... https://github.com/galaxyproject/galaxy/actions/runs/4957569426/jobs/8869412400?pr=16003

@davelopez davelopez force-pushed the add_history_archiving branch from f3ac881 to 69fc540 Compare May 12, 2023 12:48
@jdavcs
Copy link
Member

jdavcs commented May 12, 2023

Update: looks like I'm still doing something wrong... https://github.com/galaxyproject/galaxy/actions/runs/4957569426/jobs/8869412400?pr=16003

It's not your code - it's related to the naming convention I mentioned in my previous comment. The test starts out when the foreign key constraint had been already added by SQLAlchemy (as per the model definition). It was added with a different name - so when the test tries to run a downgrade, the constraint is not found. (if you were to start on a previous version - as you did in development - make the change and add the revision, then run upgrade - then the constraint would have the name you gave it and all would've been tip-top! However, since the test starts with a blank database (as it should), the constraint name specified in the revision script is never applied.

Changing the name to match what's generated is easy for postgres (history_archive_export_id_fkey). However, as an added bonus, we have to deal with SQLite as well, which does things differently: to quote Alembic's docs, "SQLite, unlike any other database, allows constraints to exist in the database that have no identifying name." [ref]. The correct solution is an automated naming convention - Alembic has a whole section on The Importance of Naming Constraints, which, of course, was "not urgent" at the time... I'll fix this.

@davelopez davelopez force-pushed the add_history_archiving branch 3 times, most recently from 3eef81a to 7852258 Compare May 16, 2023 08:55
@davelopez davelopez changed the title [RFC] Add History Archiving feature Add History Archival feature May 17, 2023
@davelopez davelopez force-pushed the add_history_archiving branch 3 times, most recently from fb4cfdc to b15c51a Compare May 22, 2023 11:49
@davelopez davelopez marked this pull request as ready for review May 22, 2023 15:58
@github-actions github-actions bot added this to the 23.1 milestone May 22, 2023
@davelopez davelopez marked this pull request as draft May 24, 2023 07:26
@davelopez davelopez force-pushed the add_history_archiving branch from b15c51a to 012713d Compare May 24, 2023 12:08
@davelopez davelopez marked this pull request as ready for review May 25, 2023 07:59
davelopez and others added 23 commits June 19, 2023 09:44
Display a different confirmation message when unarchiving a purged
history to make it clear that the history contents can only be restored if
a new copy is imported from the export record.
This will be confusing as the upload will be done in the current history and not necessarily this one.
If there are no writeable file sources available in the server then the export record cannot be persisted. In this situation we only allow regular archival without freeing up space.
Using the export tracking functionality to associate export records requires Celery to be enabled in the instance.
It is redundant with the Snapshot badge
The fetcher call return an ApiError that was not handled in `errorMessageAsString`
Otherwise the purge will be rejected due to immutability restrictions on archived.
- Do not use _ on fetch function names
- Fix optional query parameter in unarchiveHistory
We are already allowing @ts-ignore comments inside Vue files, but the linter is stricter in plain TS files.
We should remove it anyway when we update or replace the fetcher library.
Co-authored-by: Enis Afgan <afgane@gmail.com>
@davelopez davelopez force-pushed the add_history_archiving branch from 9c37961 to 3583e8e Compare June 19, 2023 08:04
@davelopez
Copy link
Contributor Author

I've fixed all the conflicts and updated de db migration script commit with the latest revision. This should be ready to go if someone has time to review it 🙇‍♂️

@dannon
Copy link
Member

dannon commented Jun 19, 2023

This is really great work @davelopez. It looks like it's working well, to me, so let's get it in and just iterate on it if anything pops up during testing!

@dannon dannon merged commit a119c94 into galaxyproject:dev Jun 19, 2023
@davelopez davelopez deleted the add_history_archiving branch June 19, 2023 15:06
@mvdbeek mvdbeek mentioned this pull request Jul 21, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/backend area/histories area/UI-UX highlight Included in user-facing release notes at the top kind/feature

Projects

Development

Successfully merging this pull request may close these issues.

6 participants