Skip to content

File operations for custom storage volumes #2081

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

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

Conversation

lahariguduru
Copy link

Finished Get and set up API Endpoint for issue #1527. This has been checked between client and daemon, and successfully executes the command.

@stgraber
Copy link
Member

stgraber commented May 6, 2025

Looking into this one

@stgraber
Copy link
Member

stgraber commented May 6, 2025

Did an initial cleanup pass, fixing the missing comments (reported by Code tests) and removed an incusd binary as well as a couple of test files that were mistakenly included into your branch.

@stgraber
Copy link
Member

stgraber commented May 6, 2025

Also re-generated the translations as that's needed when adding a new command.

@stgraber
Copy link
Member

stgraber commented May 6, 2025

Also found some debug comments and added some missing line breaks.

@stgraber
Copy link
Member

stgraber commented May 6, 2025

And also re-generated the API documentation.

stgraber and others added 2 commits May 6, 2025 17:27
@stgraber stgraber force-pushed the filesystem_custom_storage_volumes branch from 5900b21 to 75034c7 Compare May 6, 2025 21:32
@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels May 6, 2025
@stgraber
Copy link
Member

stgraber commented May 6, 2025

Okay, I've done enough cleanup that you can see the expected structure for this.

Now for the actual review.

This PR is extremely incomplete. The issue refers to implementing a file API similar to that for instances, this means:

  • incus storage volume file create
  • incus storage volume file delete
  • incus storage volume file edit
  • incus storage volume file mount
  • incus storage volume file pull
  • incus storage volume file push

At the API level, that means supporting:

  • GET /1.0/storage-pools/POOL/volumes/custom/VOLUME/files?path=/XYZ
  • POST /1.0/storage-pools/POOL/volumes/custom/VOLUME/files?path=/XYZ
  • HEAD /1.0/storage-pools/POOL/volumes/custom/VOLUME/files?path=/XYZ
  • DELETE /1.0/storage-pools/POOL/volumes/custom/VOLUME/files?path=/XYZ

As well as having a SFTP endpoint at: /1.0/storage-pools/POOL/volumes/custom/VOLUME/sftp

The CLI should be based on what we have for instances (code should be shared, no need to duplicate it) and similar to instances, the CLI should just use SFTP, not the slower /files API, that's for scripting.

All the API endpoints should behave identically to their /1.0/instances/NAME/files equivalent, same headers, same content type, ... Similar to the CLI comment above, the server-side logic should be shared, so it should really be about moving some code into helpers and then calling it from both the instance and custom volume endpoints.

And we obviously need some tests for this stuff too.

@stgraber stgraber force-pushed the filesystem_custom_storage_volumes branch from 75034c7 to af9242c Compare May 6, 2025 21:39
@stgraber stgraber marked this pull request as draft May 6, 2025 21:39
@stgraber stgraber changed the title inital implementation of filesystem custom storage volumes File operations for custom storage volumes May 6, 2025
Copy link
Member

@stgraber stgraber left a comment

Choose a reason for hiding this comment

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

Looks like the right general areas of the code have been identified, but now this needs to be fully fleshed out with a real files API including SFTP support and client and CLI functions that match those already there for instances.

The net new code for this feature should be pretty minimal. All the needed client side and server side logic already exists for instances, the logic in question needs to be moved to common functions that can be called with the relevant path (either the container's rootfs or the custom storage volume's path).

@stgraber
Copy link
Member

stgraber commented May 6, 2025

Moved the issue back to draft. Please mark it as ready for review when the remaining bits have been implemented.

@stgraber
Copy link
Member

stgraber commented May 6, 2025

I'm now looking into #2071 which will address part of this issue (there's overlap between the two). If we can get the /sftp endpoint and incus storage volume file mount implemented through that one, that'd leave the full /files API for this one as well as the remaining command line helpers (which would use the /sftp endpoint).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Development

Successfully merging this pull request may close these issues.

2 participants