Fix a panic for in-tree drivers that partialy support Block volume metrics

[nixpanic]

What type of PR is this?

/kind feature
/sig storage

What this PR does / why we need it:

#97972 added support for gathering metrics for Block volumes provided by CSI drivers. The current in-tree drivers that support Block volumes can return at least the Capacity of the block-device.

CSI drivers are currently not tested for metrics gathering in the e2e framework. Adding support for this in the in-tree drivers makes it possible to verify the functionality and prevent regressions.

Which issue(s) this PR fixes:

Fixes #101431

Special notes for your reviewer:

Block volume metrics detection is quite limited. Except for the Capacity (size of the volume), there is little that can be gathered with standard tools. The contents of a Block volume can not be inspected like a filesystem. In theory drivers could thin-provision (like a sparse file) volumes, and provide Used/Available in addition to the Capacity. However, this needs access and details knowledge of the storage platform, and can not be detected with standard tools.

Does this PR introduce a user-facing change?

Some of the in-tree storage drivers indicate support for the MetricsProvider interface, but fail to configure this for BlockMode volumes. With a recent change, Kubelet will call GetMetrics() for BlockMode volumes, and the in-tree drivers that miss the support cause a Go panic. Now the in-tree storage drivers that support BlockMode volumes, will return the Capacity of the volume in the GetMetrics() call.

[k8s-ci-robot]

Hi @nixpanic. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[nixpanic]

@gnufied, you expressed interest in adding a metrics test for BlockMode volumes, so assigning you already, before others have posted review comments.

/assign gnufied

[jsafrane]

/ok-to-test

[nixpanic]

The Azure jobs (not required to pass) seem to have failed due to something unrelated:

ProvisioningFailed: Failed to provision volume with StorageClass "azurefile-3090-kubernetes.io-azure-file-dynamic-sc-vv4t2": invalid option "enableLargeFileshares" for volume plugin kubernetes.io/azure-file

[andyzhangx]

The Azure jobs (not required to pass) seem to have failed due to something unrelated:

ProvisioningFailed: Failed to provision volume with StorageClass "azurefile-3090-kubernetes.io-azure-file-dynamic-sc-vv4t2": invalid option "enableLargeFileshares" for volume plugin kubernetes.io/azure-file

kubernetes-sigs/azurefile-csi-driver#645 would fix the issue soon

[andyzhangx]

/retest

[nixpanic]

/kind bug

Without this change, some of the in-tree storage drivers can cause a panic. See #101431 (comment) for more details.

[pohly]

/kind bug

Without this change, some of the in-tree storage drivers can cause a panic. See #101431 (comment) for more details.

That should be mentioned in the release note section of the PR description.

[gnufied]

So, without a solution to this, I am not able to make calcAndStoreStats() safe when drivers expose the MetricsProvider interface, but fails to provide a GetMetrics() function.

Hmm - we are running into golang limitations. :-) But this code has broken in past and somehow gets past review process and hence I think is in urgent need of fixing.

I am thinking we should tweak the way we are embedding interfaces in volume plugin interface such that it is possible to query if MetricsProvider has been set. For example - BlockVolume interface can have an explicit function:

type BlockVolume interface {
	GetGlobalMapPath(spec *Spec) (string, error)
	GetPodDeviceMapPath() (string, string)

        // if MetricsProvider is set return it , otherwise nil
        GetMetricsProvider() MetricsProvider

	MetricsProvider
}

This way, it should be possible to write code like:

	if v.GetMetricsProvider() != nil {
		fmt.Printf("name is: %s\n", g.GetMetrics())
	}

That is just one idea though, there could be other ways of solving this. But I do think - this code is bit fragile. :(

[nixpanic]

Thanks for the idea, @gnufied! Instead of GetMetricsProvider() I have added SupportsMetrics() which returns a bool with commit Add SupportsMetrics() for Block-mode volumes. Please have a look again and see if this addresses your concerns.

[nixpanic]

/retest

[k8s-ci-robot]

@nixpanic: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-storage-snapshot	b997e0e	link	/test pull-kubernetes-e2e-gce-storage-snapshot
pull-kubernetes-e2e-gce-csi-serial	b997e0e	link	/test pull-kubernetes-e2e-gce-csi-serial

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[ehashman]

/priority important-longterm

Kubelet changes LGTM.

[gnufied]

/lgtm

[gnufied]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, mrunalp, nixpanic

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/kubelet/OWNERS [mrunalp]
• pkg/volume/OWNERS [gnufied]
• test/e2e/storage/OWNERS [gnufied]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment