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.

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

Capacity tracking support #89

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jleeh
Copy link

@jleeh jleeh commented May 18, 2023

Enable storage capacity tracking as per the CSI documentation:
https://kubernetes-csi.github.io/docs/storage-capacity-tracking.html

While using this CSI driver in-cluster, I hit issues where pods would be attempted to be scheduled on nodes that didn't have enough disk space remaining to satisfy the request while the topology requirements kept the same so the provisioner would get in an endless error loop.

Since capacity requests aren't supported in the node server, this takes the same approach to provisioning in which a pod is created on the specified node to check remaining capacity based on the device pattern.

Tested in-cluster and is working.

pkg/lvm/lvm.go Outdated
provisionerPod, deleteFunc, err := createPod(
ctx,
"sh",
[]string{"-c", fmt.Sprintf("pvdisplay %s %s %s", va.devicesPattern, "--units=B", "-C")},
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it would be much better to execute

 pvs --reportformat json

and parse the output into a go struct instead of using regex

@majst01
Copy link
Contributor

majst01 commented May 19, 2023

Nice !

@jleeh
Copy link
Author

jleeh commented May 19, 2023

Thanks! Will get the comments actioned and add a test that asserts the storage capacity objects get created.

@jleeh
Copy link
Author

jleeh commented Jul 13, 2023

Got this updated and tested in-cluster. Couldn't unfortunately add tests as I realised it depended on the helm chart changes to enable capacity tracking.

@majst01
Copy link
Contributor

majst01 commented Jul 13, 2023

Got this updated and tested in-cluster. Couldn't unfortunately add tests as I realised it depended on the helm chart changes to enable capacity tracking.

having tests for this would be very much appreciated

@jleeh
Copy link
Author

jleeh commented Jul 17, 2023

@majst01 since the tests install the helm chart and for capacity tracking to be enabled it requires the helm chart changes as per this PR:
https://github.com/metal-stack/helm-charts/pull/68/files

How would you propose to add tests? One solution would be to disable capacity tracking in that PR by default, then enable it in the values file in this repo for testing.

@Gerrit91
Copy link
Contributor

Hey @jleeh,

having the feature disabled by default and then enable it for integration testing in this repository sounds good to me.

I just allowed your PR in the helm repo to be built such that there will also be a PR helm repo available that you can use for trying out your changes (https://helm.metal-stack.io/pull_requests/<branch-name>).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants