Skip to content

Ignore failed chown during breeze env config#35905

Merged
uranusjr merged 1 commit intoapache:mainfrom
astronomer:ignore-failed-chown
Nov 28, 2023
Merged

Ignore failed chown during breeze env config#35905
uranusjr merged 1 commit intoapache:mainfrom
astronomer:ignore-failed-chown

Conversation

@uranusjr
Copy link
Member

In an edge case, a host's user or group ID may cause the chown method to not succeed if they have special meanings in the container. This does not necessarily make Breeze unfunctional (depending on the actual permission in the host), so it may be better to simply skip the chown instead of failing the entire command and cause Breeze to exit.

In an edge case, a host's user or group ID may cause the chown method to
not succeed if they have special meanings in the container. This does
not necessarily make Breeze unfunctional (depending on the actual
permission in the host), so it may be better to simply skip the chown
instead of failing the entire command and cause Breeze to exit.
Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

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

LGTM +1

@uranusjr uranusjr merged commit 6649e7d into apache:main Nov 28, 2023
@uranusjr uranusjr deleted the ignore-failed-chown branch November 28, 2023 10:27
@potiuk
Copy link
Member

potiuk commented Nov 28, 2023

Nice. Yeah. Some Windows FS does not support chown if you have the Windows FS mounted. It does not bring full Windows breeze functionality in, but in some cases it might help.

Suggestion @uranusjr : If we do want to make Breeze to work on windows - maybe a good idea to add a separate CI job on Windows machines where we run installation and breeze shell command on Windows. This would prevent any future windows-related failures. We run plenty of breeze commands in our CI to prevent similar things creeping in,

Was i right that It was Windows? What's your setup when you experienced it?

@uranusjr
Copy link
Member Author

This specific case is not Windows but just the user has a weird ID that can’t be chown-ed. I didn’t look too close into it tbh and it’s likely possible to fix this from another angle, but since the chown is not strictly necessary anyway I opted for the easy way out of the situation.

@potiuk
Copy link
Member

potiuk commented Nov 28, 2023

This specific case is not Windows but just the user has a weird ID that can’t be chown-ed. I didn’t look too close into it tbh and it’s likely possible to fix this from another angle, but since the chown is not strictly necessary anyway I opted for the easy way out of the situation.

Ah I see. I thought it was on the Windows FS - I saw similar problems reported before - because there chown would fail as Windows FS is not POSIX and does not support chown, Interesting case with the ID though.. BTW. We also have another case (i.e. rootless docker) and maybe that could be the case as well (in both case the solution will work however).

@potiuk
Copy link
Member

potiuk commented Nov 28, 2023

For the latter case we also have DOCKER_IS_ROOTLESS env var passed to in-container (just in case it is the rootless docker case and some other things will not work)

@potiuk
Copy link
Member

potiuk commented Nov 28, 2023

Just want to get to the bottom of the weird UID - I think that could have actually be the case with ROOTLESS_DOCKER - because there there is user remapping going on under-the hood - and depending on the rootles docker solution (there are slight differences in podman, docker-desktop configured in rootless mode and colima)

The important thing in this case is that we do not have to do two thing in ROOTLESS_DOCKER:

  1. we do not have to change ownership of files (and even cannot because HOST_UID has different meaning in-docker than out-docker (the UIDs are remapped)

  2. we do not have to fix ownership of created files, because the user we run with by default is usually remapped from the original user from the host, so we should not even attempt to fix the ownership of the files when exiting the container.

We are allready doing the lattter in breeze, but likely I will have to take a look if there are other places where it we might have similar problems. Rootless docker is gaining populrity as this is quite a bit more secure way of running docker containers and many of the solution (no docker-desktop yet) start to switch to it by default, so we might want to make sure it is the first-class-citizen for Breeze as well.

@potiuk
Copy link
Member

potiuk commented Nov 28, 2023

More complete fix in #35917 (I left the || true as well, because it makes no harm and possiby handles other cases) - but I documented the rootless docker changing our past decisions about changing ownership inside docker ( I forgot to add it when I added rootles docker in #34537 in September

@uranusjr
Copy link
Member Author

It could be. Unfortunatly I don’t have access to the machine anymore, but the setup is not a very trivially fresh Docker Desktop installation but with a bunch of configs mixed in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants