LINBIT / linstor-proxmox

Integration pluging bridging LINSTOR to Proxmox VE
31 stars 7 forks source link

Name given by proxmox-csi-plugin is not allowed by linstor-proxmox #61

Closed adoerler closed 5 months ago

adoerler commented 5 months ago

Hi,

we are evaluating linstor-proxmox on Proxmox 8.2.2 in combination with Talos and proxmox-csi-plugin.

When using proxmox-csi-plugin to provision persistent volumes for Kubernetes running as VMs on Proxmox, proxmox-csi-plugin creates disk named like: vm-9999-pvc-124c0df4-16f5-46bc-a295-ecd24f145fb2'.

Provisioning fails as this name is not allowed by linstor-proxmox.

The error is like:

failed to create vm disk: 500 allocated name ('vm-9999-pvc-124c0df4-16f5-46bc-a295-ecd24f145fb2') has to be a valid UUID, legacy, state, or cloud-init name at /usr/share/perl5/PVE/Storage/Custom/LINSTORPlugin.pm line 384

None of the regexes provided in PluginHelper.pm allows such a name.

The controller of proxmox-cis-plugin creates the name like this:

vol := volume.NewVolume(region, zone, params[StorageIDKey], fmt.Sprintf("vm-%d-%s", vmID, pvc))

Where vmID is hardcoded to 9999 and pvc equals request.GetName() from the csi request, which seems to be pvc-<UUID> provided by Kubernetes.

Would you consider adding another regex or should we try to ask on proxmox-cis-plugin side for a way to configure the naming of the disks?

Best regards and many thanks in advance!

Andreas

rck commented 5 months ago

can you test the version from the rck/pvc branch ( https://github.com/LINBIT/linstor-proxmox/tree/rck/pvc ) please:

git fetch
git checkout -b rck/pvc origin/rck/pvc
scp LINSTORPlugin.pm root@${HOST}:/usr/share/perl5/PVE/Storage/Custom/
scp ./LINBIT/PluginHelper.pm root@${HOST}:/usr/share/perl5/LINBIT/PluginHelper.pm
ssh root@${HOST} "systemctl restart pvedaemon"
adoerler commented 5 months ago

Hi @rck ,

an you test the version from the rck/pvc branch

thanks you!

I can confirm that the version in branch rck/pvc works :-)

I'm not sure if you've been notified about my comment in the closed issue.

I think the valid_state regex should either read:

sub valid_state_name {
    $_[0] =~ /^vm-\d+-state-.+\z/i
}

to allow uppercase snapshot names or you should ensure (which would be even better) that the snapshot name provided by Proxmox gets converted to lower case characters.

rck commented 5 months ago

I saw that one and thanks, I might squeeze in one of the variants. I like the "convert to lowercase" a bit more. In general I think the limitation as it is now is fair, then users just have to play according the rules the plugin has and using only lowercase names is not the end of the world. There are a lot more limitations, funny users could also trigger names not valid on DRBD or LINSTOR level. Or maybe names not valid on ZFS/LVM level and so on. "do stupid things, get stupid results"

adoerler commented 5 months ago

I like the "convert to lowercase" a bit more only lowercase names is not the end of the world.

absolutely. I see no reason for allowing upper case characters, but handling a case where creation of resource fails by converting the input string to lower case seems a good idea.

There are a lot more limitations, funny users could also trigger names not valid on DRBD or LINSTOR level.

I can imagine... many cases to handle on many many systems out there :-)

rck commented 5 months ago

fixed in https://github.com/LINBIT/linstor-proxmox/commit/70868c06757a539bd444492a90b879abae341dbf and v8.0.3