Fixed creation of ControllerCreateVolumeRequest.

[apollo13]

CapacityRange should only be set if RequiredBytes and/or
LimitBytes are set.

[apollo13]

This was discovered in https://gitlab.com/rocketduck/csi-plugin-nfs/-/issues/5

[lgfa29]

Thanks for the fix @apollo13. I'm not too familiar with the CSI spec, but it doesn't seem to dictate any specific behaviour in this scenario:

// This field is OPTIONAL. This allows the CO to specify the capacity
// requirement of the volume to be provisioned. If not specified, the
// Plugin MAY choose an implementation-defined capacity range. If
// specified it MUST always be honored, even when creating volumes
// from a source; which MAY force some backends to internally extend
// the volume after creating it.

This issue interprets this as something that is up to each container orchestrator (CO) to decide, which is not great. Kubernetes seems to fail if the volume size is zero, as reported in this issue ceph/ceph-csi#2472, so I would expect most plugins to expect this behaviour from other COs as well.

Your approach would push the implementation details to plugins (If not specified, the Plugin MAY choose an implementation-defined capacity range) if Min and Max are not defined. Would that be OK or should an error be thrown?

[apollo13]

@lgfa29 Thank you for your response. I am also not sure what the best approach spec-wise would be. That said creating a volume currently fails due to:

nomad/plugins/csi/plugin.go

Lines 487 to 491 in 2998c4b

	if r.CapacityRange != nil {
	if r.CapacityRange.LimitBytes == 0 && r.CapacityRange.RequiredBytes == 0 {
	return errors.New(
	"one of LimitBytes or RequiredBytes must be set if CapacityRange is set")
	}

So either my patch, relaxing the above check or more validation logic around volume create (ie already throw an error there alread) is needed.

Your approach would push the implementation details to plugins (If not specified, the Plugin MAY choose an implementation-defined capacity range) if Min and Max are not defined. Would that be OK or should an error be thrown?

It would be OK for me to push this to the plugins, then they can perform their own validation as needed.

[apollo13]

Thinking more about it I think the current code in nomad:

nomad/client/structs/csi.go

Lines 233 to 236 in f639f71

	CapacityRange: &csi.CapacityRange{
	RequiredBytes: req.CapacityMin,
	LimitBytes: req.CapacityMax,
	},

is indeed wrong. The spec clearly says that CapacityRange is optional, but if it is set at least one of the fields has to have a value. It also says that "A value of 0 is equal to an unspecified field value." for the fields in CapacityRange. So no matter what the CO or plugin later on does, the nomad clients should never submit a CapacityRange where both fields are zero because that would be a spec violation. As such I think my patch is the correct way and required.

[lgfa29]

The spec clearly says that CapacityRange is optional, but if it is set at least one of the fields has to have a value. It also says that "A value of 0 is equal to an unspecified field value." for the fields in CapacityRange. So no matter what the CO or plugin later on does, the nomad clients should never submit a CapacityRange where both fields are zero because that would be a spec violation. As such I think my patch is the correct way and required.

Ah good catch, I missed that part from the spec, and that would actually be the behaviour I would expect.

I pushed a commit to document this requirement as a comment in the code and also to fix a small type in the CHANGELOG entry.

Thanks for catching and fixing this!

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.