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

docs: added pvc resize info #2740

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Choose a reason for hiding this comment

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

The current note says two options exist, but the text only describes one. What is the second option? Also, it is worth mentioning that changing the volume size in the devfile and restarting the workspace will not expand the PV automatically.

Copy link
Contributor Author

@deerskindoll deerskindoll Jun 19, 2024

Choose a reason for hiding this comment

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

good catch! apologies, I'll fix the wording right away. Andrew and I decided to remove the second option from the note because it's more trouble than it's worth.

Check the updated wording and let me know if the message that modifying PVC is risky (and there's only one way to do it) is explicit enough. If yes, I think mentioning that changing the volume + restarting the workspace won't work is unnecessary. I can add it if you think it's important, though.

Choose a reason for hiding this comment

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

Okay. Looks good. Concerning the change in the devfile I think it is worth adding a note, maybe not in the Admin guide, but in the User guide. Without a note a developer user may think it is just a matter of changing the volume size in its devfile and it will be automatically resized.

Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,11 @@ spec:
<2> Specify a claim size on the next line or omit the next line to set the default claim size value. The specified claim size is only used when you select this storage strategy.
<3> The claim size must be specified as a link:https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/quantity/[{kubernetes} resource quantity]. The available quantity units include: `Ei`, `Pi`, `Ti`, `Gi`, `Mi` and `Ki`.

pass:[<!-- vale RedHat.CaseSensitiveTerms = YES -->]
pass:[<!-- vale RedHat.CaseSensitiveTerms = YES -->]

[IMPORTANT]
====
Manually modifying a PVC on the cluster that was provisioned by {prod-short} is not officially supported and may result in unexpected consequences.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I should have mentioned this earlier, but this is also unsupported 🫠 Changing the default workspace storage size in the Che Cluster custom resource won't result in the existing PVCs being resized.

You'd have to delete and re-create your workspace (or delete the PVC and have it be automatically re-created, but this too is not officially supported and only works for the per-workspace storage strategy as it causes a bug with the per-user storage strategy).

So, it seems like the documentation we're adding here is either only a warning (that modifying the PVC on the cluster is not supported). We could also remark that you have to delete the workspace and re-create it, but that seems redundant, as we've discussed.

Choose a reason for hiding this comment

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

I see it. So, yes, let's add it as a warning/important note. We definitely need to have this note added to the docs. I've seen users asking for this, and we need to state this even as a warning to make it clear the right/supported procedure for this.

Copy link

@rafaeltuelho rafaeltuelho Jun 19, 2024

Choose a reason for hiding this comment

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

Whether in the devfile or CheCluster CR, users need to know the proper way to have a workspace PV resized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since both options that we planned to feature in the note are actually not supported, is there a proper way to resize PVC outside of setting everything up from scratch?

Choose a reason for hiding this comment

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

Well, in my experiments I noticed that changing the PVC manually and restarting the workspace works. But I'm not sure if the user itself would have permission to do that in its own namespace or if it would have to ask a cluster-admin to perform this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AObuchow @rafaeltuelho your call, guys. decide what should be included in the notification and I'll put it in the admin + end user guides.

Copy link
Contributor

Choose a reason for hiding this comment

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

After discussion, we've agreed upon informing the user that manually expanding the PVC is possible, but not officially supported. The user has to restart their workspace after requesting the expansion of their PVC (by modifiying the PVC on the cluster) in order for the PVC to actually get resized (as the workspace pod needs to detach from the PVC to allow expansion to occur).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! I hope I explained it correctly, please have a look.

If you want to resize a PVC, modify the default workspace storage size in the `CheCluster` custom resource.
====
Loading