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

Change PV and PVC status capacity on resize #350

Closed
wants to merge 2 commits into from

Conversation

marjus45
Copy link

@marjus45 marjus45 commented Jul 30, 2023

We add the proper interface for volume expansion. This will not have any effect on the underlying volume, but it will change properly the PV capacity, the PVC status, and the proper events when a claim is made.

To do this I used the external-resizer provided by Kubernetes for this purpose. This requires a Golang version upgrade to 1.20.

This relates to #190 and is a follow up to #189 which allowed volume expansion on the storage class level.

As it is now, when a claim is made the provisioner does nothing, and the PV capacity, as well as the PVC status, remain unchanged. This enhances the consistency, and will further assist if we end up adding quotas as described in #190.

@marjus45 marjus45 marked this pull request as draft July 30, 2023 14:57
@marjus45 marjus45 force-pushed the feature-external-resizer branch from 90bb109 to 3326727 Compare July 30, 2023 15:09
@marjus45 marjus45 marked this pull request as ready for review July 30, 2023 15:51
@laurivosandi
Copy link

How about adding possibility to invoke helper pod when this happens? eg to change XFS quota when this happens?

@derekbit
Copy link
Member

@marjus45 Can you check @laurivosandi's comment? Thank you.

@yiippee
Copy link

yiippee commented Jan 30, 2024

Any update on this?

@marjus45
Copy link
Author

Hi @laurivosandi , and sorry for the (very) late reply. I am happy proceed with this and to help but I am not that familiar with the XFS quota setup. If you, or @derekbit could give me some pointers on how I could test that it would be great.

I see that there is an example in local-path-provisioner/examples/quota but it is not clear with me how to build the helper Pod image (xxx/storage-xfs-quota:v0.1) and if I need to consider anything else regarding the Kubernetes nodes. Any help would be appreciated.

@marjus45 marjus45 force-pushed the feature-external-resizer branch from 3326727 to c0c1c13 Compare January 31, 2024 22:23
@marjus45
Copy link
Author

marjus45 commented Jan 31, 2024

Hi @derekbit and @laurivosandi , I have added the invocation of the helper pod, with a new "resize" script and action. I have not updated any related example for the XFS quota since I do not have such setup to test, but that shouldn't block this PR.

@marjus45
Copy link
Author

marjus45 commented Feb 6, 2024

Hey @derekbit I think something is off with the CI. It says "build was killed" but there is no error as far as I can understand. Also it has a message about an expired license which might be relevant.

@mrdracon
Copy link

This is a very nice feature, any chance this PR gets a little bit of attention?

@fs185143
Copy link

Is this still in development?

@marjus45
Copy link
Author

@fs185143 This is waiting feedback from the maintainers

@dnaeon
Copy link

dnaeon commented Mar 11, 2024

Tested this PR last week and it works fine in a local setup (thanks @marjus45 !).

Can we expect this to be reviewed and merged soon?

Thanks!

@derekbit
Copy link
Member

Tested this PR last week and it works fine in a local setup (thanks @marjus45 !).

Can we expect this to be reviewed and merged soon?

Thanks!

Sure. This is in the scope of the upcoming release (before the end of May).

@@ -39,6 +39,9 @@ rules:
- apiGroups: ["storage.k8s.io"]
resources: ["storageclasses"]
verbs: ["get", "list", "watch"]
- apiGroups: [ "" ]

Choose a reason for hiding this comment

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

@marjus45 shouldn't we do these changes (to the ClusterRole and to the ConfigMap) to the corresponding resources as well in the chart under deploy/chart/local-path-provisioner?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, you are right, I had missed that. I updated and tested the helm installation as well.

@@ -9,13 +9,16 @@ import (
"syscall"

"github.com/Sirupsen/logrus"
resizeController "github.com/kubernetes-csi/external-resizer/pkg/controller"

Choose a reason for hiding this comment

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

I don't have much context but I can share that the external-resizer runs as a sidecar next to a CSI driver. It is one of the many CSI sidecars - external-provisioner, external-attacher, csi-driver-node-registrar and liveness-probe. Isn't possible to run the external-resizer as a sidecar container?

Copy link
Author

Choose a reason for hiding this comment

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

You are right @ialidzhikov, the resizer would fit better as a separate container. Such change though would require a change in the directory structure. I can draft a solution as such, but I would like a comment also from the maintainers if they are okay with such a change. What do you think @derekbit ?

@Littlehhao
Copy link

Tested this PR last week and it works fine in a local setup (thanks @marjus45 !).
Can we expect this to be reviewed and merged soon?
Thanks!

Sure. This is in the scope of the upcoming release (before the end of May).

Great, I hope this version can support the capacity limit and volume expansion of pvc

We add the interface for volume expansion. This will not have any effect
on the underlying volume, but will change properly the PV capacity and
PVC status when a claim is made, and the proper events will be produced
as well.

To do that the external-resizer is used provided by Kubernetes for this
purpose.

Signed-off-by: Marjus Cako <[email protected]>
Update the resize function to invoke the helper Pod with the "resize"
script.

This also adds some dummy resize scripts, which store the resize data in
a file in the PV. It does not add though any sample script for the XFS
quota resize.

Signed-off-by: Marjus Cako <[email protected]>
@marjus45 marjus45 force-pushed the feature-external-resizer branch from c0c1c13 to 238474c Compare March 27, 2024 07:27
@Littlehhao
Copy link

你好@laurivosandi,对于(非常)迟到的回复深表歉意。我很高兴继续这样做并提供帮助,但我不太熟悉 XFS 配额设置。如果你,或者@derekbit 可以给我一些关于如何测试它是否很棒的指示。

我看到其中有一个示例local-path-provisioner/examples/quota,但我不清楚如何构建辅助 Pod 映像 ( xxx/storage-xfs-quota:v0.1) 以及是否需要考虑有关 Kubernetes 节点的其他任何内容。任何帮助,将不胜感激。

docker pull nicktming/storage-xfs-quota:v0.1

@exp0nge
Copy link

exp0nge commented Apr 20, 2024

Should the storage class not have volume expansion set to true?
https://github.com/marjus45/local-path-provisioner/blob/feature-external-resizer/deploy/local-path-storage.yaml#L117

@Littlehhao
Copy link

In the latest version v0.0.27, is pvc changesize supported?
@derekbit

@derekbit
Copy link
Member

derekbit commented May 29, 2024

In the latest version v0.0.27, is pvc changesize supported? @derekbit

Not yet. I put it into v0.0.28 instead. I hope v0.0.28 can be released in Aug.

@maaft
Copy link

maaft commented Jul 2, 2024

eagerly waiting for this to be included :)

Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale label Aug 17, 2024
Copy link

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this Aug 27, 2024
@HoustonPutman
Copy link

This would be very helpful for users trying to test volume expansion when using KinD (which uses this provisioner by default). Is this PR still being considered for inclusion in the next release?

@Ethereal-O
Copy link

Is pvc changesize supported in 0.0.30? Thanks. @derekbit

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

Successfully merging this pull request may close these issues.