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

[question] What is use of @Delete method in Storage interface? #1547

Closed
xytan0056 opened this issue Feb 18, 2020 · 7 comments
Closed

[question] What is use of @Delete method in Storage interface? #1547

xytan0056 opened this issue Feb 18, 2020 · 7 comments

Comments

@xytan0056
Copy link
Contributor

xytan0056 commented Feb 18, 2020

Since the storage of module is immutable, we should not delete a stashed module, then do we need to have a deleter at all?

@marwan-at-work
Copy link
Contributor

I think this will be helpful if we ever need to do migrations of an underlying storage which I imagine is only necessary until we have a v1 then we can remove it.

Happy to hear more opinions

@xytan0056
Copy link
Contributor Author

We just happened to have done such a migration recently. We wanted to switch storage location, but then realized that there's a chance that a newly stashed module can have a different sum than existing one, resulting in mismatched checksum for users. That means we cannot permit a change/deletion of modules in storage.

@marwan-at-work
Copy link
Contributor

Do note that this functionality is not exposed in the http server, so there's no way for someone to delete a module by calling an endpoint on Athens.

If you are doing a migration, I imagine copying the modules to the new storage will ensure the same sum, and then you can delete modules from the old storage once everything looks good.

The scenario I see for Athens to use this Delete method is the following:

  1. We decide that the GCP storage layout should look different
  2. We don't want to force users to migrate manually
  3. Therefore, when Athens boots up, it will prompt the user to copy all the modules into the new layout
  4. It will use the Delete endpoint to remove the old layout once everything looks good.

Obviously, we would never do that implicitly without major communication and discussion, and the chances are low for such a scenario but it's just an example :)

@xytan0056
Copy link
Contributor Author

By migration, I meant we changed the way we stash modules. However we can't delete the old modules because re-stashed ones will have different sums.

for 3, do you mean do this in config? config read is a one time thing, how can athens know that the storage changed?
for 4, Don't we still have to keep old instance up or somehow call the Delete method within Athens if we want to remove old storage?

@arschles
Copy link
Member

By migration, I meant we changed the way we stash modules

@xytan0056 which storage driver you are using now?

@xytan0056
Copy link
Contributor Author

@arschles Our internal storage. We had to implement the API ourselves. I'm just wondering if we need to delete modules at all

@arschles
Copy link
Member

got it. @xytan0056 currently Athens never calls Delete. As @marwan-at-work said, we put it in the interface for future proofing.

That said, if I was building out a custom storage driver for internal usage, I would probably return an error for delete right now, to prevent deletions for the same reason you said in #1547 (comment).

On a somewhat related note, I'm designing the interface for generic external storage drivers in #1551.

Since you already have built an external storage, you would be a great person to give feedback on the interface. Would you be able to go make some comments on that PR? The specific interface is here

I am going to close this issue now. If we didn't answer your question @xytan0056, please feel free to comment in here again. We'll get the notification and come back to continue the discussion.

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

No branches or pull requests

3 participants