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

fix: Update .stow-local-ignore with files in /home/user that would break running stow #196

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

isuftin
Copy link

@isuftin isuftin commented Dec 4, 2024

Copy link
Contributor

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

@isuftin just a heads up that UBI8 is going to be removed from the codebase as part of - #192 / eclipse-che/che#23034

We can technically merge this PR so that latest UBI8 image contain the fix, but going forward we are not going to support / udpate it in the upstream.

@ibuziuk
Copy link
Contributor

ibuziuk commented Dec 5, 2024

@dkwon17 @AObuchow @SDawley folks, we need to make sure that the fix is properly downstreamed for Dev Spaces 3.19

@isuftin
Copy link
Author

isuftin commented Dec 5, 2024

@isuftin just a heads up that UBI8 is going to be removed from the codebase as part of - #192 / eclipse-che/che#23034

We can technically merge this PR so that latest UBI8 image contain the fix, but going forward we are not going to support / udpate it in the upstream.

The sooner the better ;) Our group has been working off of the current UBI/UDI9 image in this repo but added the update for UDI8 in this MR since it's still in there.

@svor
Copy link
Collaborator

svor commented Dec 5, 2024

Hello @isuftin and thank you for the contribution!
I see changes from another PR that was merged before #193. Could you please rebase your PR with main to have only stow related changes

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.

@isuftin Thank you so much for this detailed PR, your contribution to Che/Dev Spaces is greatly appreciated 😁 I'm hoping to give a more detailed review, but tomorrow is my last day at Red Hat and I may not get the chance.

One thing I wanted to quickly point out: We actually have moved away from invoking stow from the UDI's entrypoint and now are creating a initContainer that invokes stow as part of the workspace pod. So I believe the functionality provided by this PR will likely have to be moved to the DevWorkspace Operator project instead of the UDI.

Which version of Che (or Dev Spaces) and DevWorkspace Operator have you tested this on?Hopefully, the version your running is up-to-date enough to be using the home persistence initContainer that gets injected by DevWorkspace Operator.

Edit: Nevermind, I see you mention OpenShift DevSpaces 3.17.0 in the bug report.

As a side-note: @dkwon17 it might be worth removing all stow-related logic from the entrypoint (not the Dockerfile) of the UDI and rely solely on the home persistence initContainer injected by DWO.

@AObuchow
Copy link
Contributor

AObuchow commented Dec 5, 2024

This also works fine if we keep the init container enabled because the init-container runs before the workspace container starts. At that point /home/user/.config/containers/storage.conf does not yet exist, so the init container finishes, writes /home/user/.stow_completed and by the time the workspace image's /entrypoint.sh runs, it will skip running the stow command because /home/user/.stow_completed exists.

@isuftin I may be misunderstanding something here: Is there a case where you do not want to rely on the initContainer that sets up home persistence, and instead rely on the UDI's entrypoint logic to setup home persistence?

@isuftin
Copy link
Author

isuftin commented Dec 5, 2024

This also works fine if we keep the init container enabled because the init-container runs before the workspace container starts. At that point /home/user/.config/containers/storage.conf does not yet exist, so the init container finishes, writes /home/user/.stow_completed and by the time the workspace image's /entrypoint.sh runs, it will skip running the stow command because /home/user/.stow_completed exists.

@isuftin I may be misunderstanding something here: Is there a case where you do not want to rely on the initContainer that sets up home persistence, and instead rely on the UDI's entrypoint logic to setup home persistence?

That's correct. This issue was discovered because we build a derivative, large, UDI image off of UDI9 which includes lots of files in /home/tooling due to libraries installed in virtualenv as well as npm. When the initContainer runs, if it's the initial run for that workspace, it gets OOMKilled. As of right now there's nothing we can do about that since the initContainer mem limits can't be set by the user/admin.

So we ended up disabling the init container (especially since the entrypoint in the image performs the same functionality) but because we also set up overlayfs and we persist userHome, we end up with /home/user/.config/containers/storage.conf written by DevSpaces during workspace startup but before /entrpoint.sh runs, so by the time /entrypoint.sh runs, the stow command pukes.

@isuftin
Copy link
Author

isuftin commented Dec 5, 2024

I'd also say that this issue would be hit by you, me or anyone even using even the empty sample workspace if they:

  • disable init container
  • enable overlayfs
  • persist user home

The base UDI8/UDI9 image's stow would hit that same issue in that scenario.

@ibuziuk
Copy link
Contributor

ibuziuk commented Dec 11, 2024

The sooner the better ;) Our group has been working off of the current UBI/UDI9 image in this repo but added the update for UDI8 in this MR since it's still in there.

@isuftin we just merged deprecation / removal of UBI 8 and going to maintain UBI 9 only as part of eclipse-che/che#23034. Could you please remove the ubi8 related changes and rebase on main?

@isuftin
Copy link
Author

isuftin commented Dec 11, 2024

@ibuziuk Done!

Copy link

openshift-ci bot commented Dec 12, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dkwon17, ibuziuk, isuftin

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

Needs approval from an approver in each of these files:

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

@ibuziuk
Copy link
Contributor

ibuziuk commented Dec 12, 2024

@isuftin thank you for the follow-up, please rebase on main and we can merge

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.

6 participants