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 filesystem migration syscall #15

Closed
wants to merge 2 commits into from
Closed

Conversation

sosthene-nitrokey
Copy link
Collaborator

Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

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

I don’t fully understand this PR currently. Do you have a branch that shows who this would look like in the runner?

My understanding is:

  1. Applications and backends can define migration functions with the signature fn(&dyn DynFilesystem, &dyn DynFilesystem).
  2. The runner registers these migration functions when setting up the trussed-staging backend and assigns a versioning scheme.
  3. The migrations are then triggered by the runner using a syscall that provides the versions.

My open questions:

  • Why are the migrations implemented as a syscall if they are only triggered by the runner? Couldn’t the runner just call some function of the backend?
  • This provides applications with much more privileges than they actually need. Can’t we use a DynClientFilestore for application migrations and something like a BackendFilestore for backend migrations?
  • How is the current FS version stored? Shouldn’t it be managed by the backend?

@sosthene-nitrokey
Copy link
Collaborator Author

sosthene-nitrokey commented Mar 11, 2024

  • Why are the migrations implemented as a syscall if they are only triggered by the runner? Couldn’t the runner just call some function of the backend?

Migrations are triggered by the admin app.

  • This provides applications with much more privileges than they actually need. Can’t we use a DynClientFilestore for application migrations and something like a BackendFilestore for backend migrations?

That's true, but the reason for that is to be able to perform migrations much more efficiently. Iterating over directories with the ClientFilestore API is very painful and slow.
I'm not sure it's worth it to build an entire wrapper just for that. The migrators are registered by the runners and are pretty simple. They should be trusted.

  • How is the current FS version stored? Shouldn’t it be managed by the backend?

This could be managed by the backend, but that makes 1 more file to store somewhere on the IFS.
I instead made it stored by the AdminApp in the Config struct for less overhead. The admin app is also loaded before the other apps so it can still run the migrations prior to other application observing the unmigrated state.

@robin-nitrokey
Copy link
Member

Migrations are triggered by the admin app.

But still during the boot/setup process and not triggered by a command?

@sosthene-nitrokey
Copy link
Collaborator Author

sosthene-nitrokey commented Mar 11, 2024

Initially yes, but it was made lazy before the dispatches because until then the trussed service is actually not yet running.

Maybe running it manually would be better, but the trussed service owns the filesystem when the admin app is loaded.

@sosthene-nitrokey
Copy link
Collaborator Author

Maybe this has its place in the actual configuration loading of the admin app actually. It has access to a clientfilestore and it could get access to the raw filestore that way.

@robin-nitrokey
Copy link
Member

Maybe running it manually would be better, but the trussed service owns the filesystem when the admin app is loaded.

I think we can always directly access the filesystems in the runner because Store requires Copy. Similar to what we do with the provisioner.

@sosthene-nitrokey
Copy link
Collaborator Author

Right. 

I'll migrate to that then. We can merge the split and merge migrations later.

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.

2 participants