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

Conversation

deerskindoll
Copy link
Contributor

What does this pull request change?

added pvc resize info

What issues does this pull request fix or reference?

https://issues.redhat.com/browse/CRW-6544

Specify the version of the product this pull request applies to

main

Pull Request checklist

The author and the reviewers validate the content of this pull request with the following checklist, in addition to the automated tests.

  • Any procedure:
    • Successfully tested.
  • Any page or link rename:
    • The page contains a redirection for the previous URL.
    • Propagate the URL change in:
  • Builds on Eclipse Che hosted by Red Hat.
  • the Validate language on files added or modified step reports no vale warnings.

@deerskindoll deerskindoll requested a review from a team as a code owner May 30, 2024 11:41
@deerskindoll deerskindoll requested a review from azatsarynnyy May 30, 2024 11:41
Copy link

github-actions bot commented May 30, 2024

Click here to review and test in web IDE: Contribute

Copy link

github-actions bot commented May 30, 2024

🎊 Navigate the preview: https://669665564108cf98fd302367--eclipse-che-docs-pr.netlify.app 🎊

@deerskindoll
Copy link
Contributor Author

@AObuchow is this wording ok?

Copy link
Contributor

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

Some small notes. Will get back with further clarification. Great work @deerskindoll :)

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.

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

If you want to resize a PVC, modify the default workspace storage size in the `CheCluster` custom resource.
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 and restart the workspace.
Copy link
Contributor

@AObuchow AObuchow Jul 15, 2024

Choose a reason for hiding this comment

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

There's no actual need to modify the default workspace storage size in the Che-Cluster CR after modifying the PVC.

I think this sentence could just be "If you choose to expand a PVC that is in use by a workspace, the workspace must be restarted before PVC expansion can occur"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think after this change we should be good to merge. Sorry for being slow to follow-up on this @deerskindoll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no worries! I'll edit the wording.

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 I reworded your suggestion a bit. please have a look


If you want to resize a PVC,
modify the default workspace storage size in the `CheCluster` custom resource and restart the workspace.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think after this change we should be good to merge. Sorry for being slow to follow-up on this @deerskindoll

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

If you want to resize a PVC that is in use by a workspace, you must restart the workspace must before the PVC change can occur.
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo, must is repeated twice "you must restart the workspace must before"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops, fixing immediately

Copy link
Contributor

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

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

Successfully merging this pull request may close these issues.

3 participants