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.

Sign up for GitHub

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

Jump to bottom

API docs don't include invariants for CSI volume delete #24756

Open
Vickysomtee opened this issue Dec 23, 2024 · 5 comments
Open

API docs don't include invariants for CSI volume delete #24756

Vickysomtee opened this issue Dec 23, 2024 · 5 comments
Assignees
@tgross
Labels
theme/api HTTP API and SDK issues theme/docs Documentation issues and enhancements theme/storage
Milestone
1.10.0

Comments

@Vickysomtee
Copy link

Nomad version

Nomad v1.6.0
BuildDate 2023-07-18T18:51:11Z
Revision 87d411f

Operating system and Environment details

Ubuntu

Issue

I am using the go sdk to successfully create Hetzner volumes for Postgres Databases on Nomad but I am unable to delete the volume from the external storage provider (Hetzner) after deregistering it from nomad.

I use the info func to get the volume

	res, _, err := client.CSIVolumes().Info(id, &nomad.QueryOptions{
		Namespace: "default",
		AuthToken: "xxxxxxxxxxxxxxxxxxxxx",
	})

Then pass the externalID gotten from the above res to the DeleteOpts func to delete the volume from the external storage provider (Hetzner)

	errr := client.CSIVolumes().DeleteOpts(&nomad.CSIVolumeDeleteRequest{
		ExternalVolumeID: res.ExternalID,
	}, &nomad.WriteOptions{
		Namespace: "default",
		AuthToken: "xxxxxxxxxxxxxxxxxxxxxxxxx",
	})

But I get this error

Unexpected response code: 500 (rpc error: volume not found: xxxxxxxxx)
@tgross
Copy link
Member

tgross commented Jan 6, 2025

I am unable to delete the volume from the external storage provider (Hetzner) after deregistering it from nomad.

Once you've deregistered the volume from Nomad, Nomad no longer has any ability to manipulate the volume. You'd need to re-register it before making the DeleteOpts API call. The equivalent action in the CLI is volume delete which has the following note:

The volume must still be registered with Nomad in order to be deleted. Deleting will fail if the volume is still in use by an allocation or in the process of being unpublished. If the volume no longer exists, this command will silently return without an error.

We should probably expand the API documentation to make sure that invariant is described. I'll mark this as a documentation issue but we'd also welcome a PR to improve the API docs if you're interested.

@tgross tgross added theme/docs Documentation issues and enhancements theme/storage and removed type/bug labels Jan 6, 2025
@tgross tgross moved this from Needs Triage to Needs Roadmapping in Nomad - Community Issues Triage Jan 6, 2025
@tgross tgross changed the title Unable to delete Volume from external provider API docs don't include invariants for CSI volume delete Jan 6, 2025
@Vickysomtee
Copy link
Author

Once you've deregistered the volume from Nomad, Nomad no longer has any ability to manipulate the volume. You'd need to re-register it before making the DeleteOpts API call. The equivalent action in the CLI is volume delete which has the following note:

@tgross I just tried Deleting the volume from the external provider while it's still registered in nomad, and I am still caught with the same error

@tgross
Copy link
Member

tgross commented Jan 6, 2025

@Vickysomtee I did a little more digging and it looks like there's a bug in the API here, but not one we can easily fix without breaking backwards compatibility:

The architectural problem is this: even with the external storage provider's ID, we can't delete the volume unless we can lookup the plugin for that volume, and to do that, we need Nomad's ID for the volume. The error you're seeing comes only one place on the server csi_endpoint.go#L570, which you'll only get if the volume isn't registered. But the DeleteOpts API says:

The ID passed in the request is for the storage provider's ID, so a volume that's already been deregistered can be deleted.

But as we just saw, that's not possible because we can't lookup the plugin to send the delete RPC. So the ExternalID field we're passing from commands like nomad volume delete is not the external ID but the Nomad ID, which is why commands like volume delete work if you pass the ID Nomad gave you.

So the docstring for the DeleteOpts method is wrong, but so is the name of the field we're passing in. We should probably update that to add a new (correct) field name and deprecated (but not yet remove) the old field.

@tgross tgross self-assigned this Jan 8, 2025
@tgross tgross added this to the 1.10.0 milestone Jan 8, 2025
@tgross tgross added the theme/api HTTP API and SDK issues label Jan 8, 2025
@Vickysomtee
Copy link
Author

@tgross It took a while for me to understand your explaination so I decided to run a test and I understand it now. So I passed the volume ID for nomad instead of the external storage id and it worked. The volume got deleted from the the storage provider and deregistred from nomad.

You are right. What needs to be done is to add a new field name. Any chance I pick this up?

@tgross
Copy link
Member

tgross commented Jan 9, 2025

Sure, I'd be happy to review a PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tgross tgross

Labels
theme/api HTTP API and SDK issues theme/docs Documentation issues and enhancements theme/storage
Projects
Nomad - Community Issues Triage
Status: Needs Roadmapping
Milestone
1.10.0
Development

No branches or pull requests
2 participants
@tgross @Vickysomtee