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

Get rid of Gitpod layer #4923

Merged
merged 7 commits into from
Aug 4, 2021
Merged

Conversation

csweichel
Copy link
Contributor

@csweichel csweichel commented Jul 23, 2021

fixes #4899

This PR removes the need for the Gitpod layer, by moving the user and group creation to supervisor.

How to test

Start workspaces with as many different images you can think of.
Especially interesting: images that don't have a Gitpod user to begin with, e.g. https://github.com/csweichel/test-repo/tree/image-ubuntu

Open questions:

  • do we still need the .bashrc modifications? IMHO that was somewhat of an antipattern to begin with and we should find other ways if need be.
  • what about images that don't have git. Can we live with that or should we ship a "fallback Git" as part of supervisor?

@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #4923 (e7b942d) into main (453cbeb) will increase coverage by 36.18%.
The diff coverage is 21.84%.

Impacted file tree graph

@@            Coverage Diff            @@
##           main    #4923       +/-   ##
=========================================
+ Coverage      0   36.18%   +36.18%     
=========================================
  Files         0       16       +16     
  Lines         0     4068     +4068     
=========================================
+ Hits          0     1472     +1472     
- Misses        0     2473     +2473     
- Partials      0      123      +123     
Flag Coverage Δ
components-supervisor-app 36.18% <21.84%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/supervisor/pkg/supervisor/supervisor.go 0.00% <0.00%> (ø)
components/supervisor/pkg/supervisor/user.go 33.12% <33.12%> (ø)
components/supervisor/pkg/terminal/service.go 29.09% <0.00%> (ø)
components/supervisor/pkg/supervisor/services.go 25.50% <0.00%> (ø)
components/supervisor/pkg/ports/ports.go 60.03% <0.00%> (ø)
...mponents/supervisor/pkg/supervisor/notification.go 83.95% <0.00%> (ø)
components/supervisor/pkg/ports/exposed-ports.go 0.00% <0.00%> (ø)
components/supervisor/pkg/ports/served-ports.go 76.00% <0.00%> (ø)
components/supervisor/pkg/supervisor/config.go 4.51% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 453cbeb...e7b942d. Read the comment docs.

@aledbf
Copy link
Member

aledbf commented Jul 23, 2021

Can we live with that or should we ship a "fallback Git" as part of supervisor?

It would be great if we could use any of the Go libraries available to avoid distribution dependent binaries

@csweichel
Copy link
Contributor Author

csweichel commented Jul 23, 2021

Can we live with that or should we ship a "fallback Git" as part of supervisor?

It would be great if we could use any of the Go libraries available to avoid distribution dependent binaries

I agree. We have used go-git in the past and found incompatibilities at the time regarding the global Git config. It's well possible those have been resolved by now.

Now I reckon the question is if this way we could use the latest --filter optimisations :)

Update after looking at the COMPATIBILITY.md:

  • global Git config is still not supported
  • git clone --filter=blob:none is not supported

Are there any other comprehensive Git libraries we should be looking at?

@csweichel csweichel force-pushed the csweichel/image-builder-get-rid-4899 branch 3 times, most recently from 46a17cd to 533584c Compare July 27, 2021 10:05
@csweichel csweichel force-pushed the csweichel/image-builder-get-rid-4899 branch 2 times, most recently from e65c69f to a340375 Compare July 29, 2021 09:20
usually the login process would set that,
but we're not executing child processes using a login process,
but rather run them directly without a login shell.
@csweichel csweichel force-pushed the csweichel/image-builder-get-rid-4899 branch from a340375 to 8e25346 Compare July 29, 2021 09:42
@csweichel csweichel marked this pull request as ready for review July 29, 2021 09:55
@csweichel
Copy link
Contributor Author

/hold

we really need to test this thoroughly with different images

@aledbf
Copy link
Member

aledbf commented Aug 2, 2021

/werft run

👍 started the job as gitpod-build-csweichel-image-builder-get-rid-4899.18

@aledbf
Copy link
Member

aledbf commented Aug 2, 2021

@csweichel I tested all the examples, gitpod, and even your image-ubuntu repository. Everything works.

Just one question, what's the expected behavior in case the gitpod account exists but the UID is different?
example https://github.com/aledbf/gitpod-user-uid/blob/main/.gitpod.Dockerfile

@csweichel
Copy link
Contributor Author

@csweichel I tested all the examples, gitpod, and even your image-ubuntu repository. Everything works.

Just one question, what's the expected behavior in case the gitpod account exists but the UID is different?
example https://github.com/aledbf/gitpod-user-uid/blob/main/.gitpod.Dockerfile

Excellent question. Two ways come to mind:

  1. the cheap and easy way: fail the workspace startup with a sensible, actionable error message
  2. the clever way: produce a user with a different name but correct UID. I don't think we actually rely on Gitpod as name but rather use the UID/GID (might have to double check the content-initializer code).

The first one is clearly much easier to implement. WDYT?

@aledbf
Copy link
Member

aledbf commented Aug 2, 2021

the cheap and easy way: fail the workspace startup with a sensible, actionable error message

👍

(right now stays in Pulling container image …)

@roboquat roboquat removed the approved label Aug 2, 2021
@aledbf
Copy link
Member

aledbf commented Aug 2, 2021

@csweichel awesome.

Screenshot from 2021-08-02 13-23-42

@roboquat
Copy link
Contributor

roboquat commented Aug 2, 2021

LGTM label has been added.

Git tree hash: 3d4baa050cf30fbe095186e76fc63c5cc0ba9365

@JohannesLandgraf
Copy link
Contributor

JohannesLandgraf commented Aug 2, 2021

This looks like Italy 🇮🇹 🍕CleanShot 2021-08-02 at 19 32 46@2x cc @fntlnz @leodido

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Whoa, many thanks @csweichel for taking this on! 💯 🎖️

Screenshot 2021-08-02 at 22 21 19

Epic!! 🎸 (Although I'd have preferred fewer lines of new Go code 🙈 looks a bit scary to my untrained eye, as each new line of code has a non-zero risk-of-new-bug -- regardless of how excellent the developer is!)

Added a few comments on the code. Will now test. 😇



USER ROOT
COPY ./gitpod-cli /usr/bin/gp
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR removes the need for the Gitpod layer, by moving the user and group creation to supervisor.

What about the gp CLI? Surely we want to continue installing it as well. (Should at least test that it's still there.)

Comment on lines -21 to -30
# Configure user shell
# TODO Remove this in the near future when we do not need ~/.bashrc appends/prepends any more
RUN \
# REALLY do not print motd
touch ~/.hushlogin && \
# Configure shell
BASH_RC=~/.bashrc; if [ -f "$BASH_RC" ]; then cp "$BASH_RC" ~/.bashrc-org; else touch ~/.bashrc-org; fi && \
cat /var/gitpod/gitpod/.bashrc-prepend > "$BASH_RC" && \
cat ~/.bashrc-org >> "$BASH_RC" && \
cat /var/gitpod/gitpod/.bashrc-append >> "$BASH_RC"
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! 😍

Comment on lines -1 to -58


###############################################################################
########## Gitpod - prepend
###############################################################################

# Prompt color and bash_completion
export PS1='\[\e]0;\u \w\a\]${debian_chroot:+($debian_chroot)}\[\033[01;32m\]\u\[\033[00m\] \[\033[01;34m\]\w \$ \[\033[00m\]'
#source /etc/bash_completion

# editor config - should be removed when registry facade is default
if [ -z "$EDITOR" ]; then
export EDITOR="gp open -w"
fi
if [ -z "$VISUAL" ]; then
export VISUAL="$EDITOR"
fi
if [ -z "$GIT_EDITOR" ]; then
export GIT_EDITOR="$EDITOR"
fi

# Workaround Java pre v10 by explicitly setting "-Xmx" for all Hotspot/openJDK VMs
if [ -n "$GITPOD_MEMORY" ]; then
export JAVA_TOOL_OPTIONS="-Xmx${GITPOD_MEMORY}m";
fi

# Completion for gp command
. <(gp completion)
# ide cli config - should be removed when registry facade is default
if [ ! -d "/ide/bin/" ]; then
alias open='gp open'
alias code='gp open'
fi

export GEM_HOME=/workspace/.rvm
export GEM_PATH=$GEM_HOME:$GEM_PATH
export PATH=/workspace/.rvm/bin:$PATH

export PIPENV_VENV_IN_PROJECT=true
export PIP_USER=yes
export PYTHONUSERBASE=/workspace/.pip-modules
export PATH=$PYTHONUSERBASE/bin:$PATH
unset PIP_TARGET
unset PYTHONPATH

# Set CARGO_HOME to reside in workspace if:
# - it's RUNTIME (/workspace present)
if [ -d /workspace ]; then
export CARGO_HOME=/workspace/.cargo
export PATH=$CARGO_HOME/bin:$PATH
fi

export BROWSER="${BROWSER:=gp-preview}"

###############################################################################
########## Gitpod - prepend
###############################################################################

Copy link
Contributor

@jankeromnes jankeromnes Aug 2, 2021

Choose a reason for hiding this comment

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

Burn it, burn it all with fire! 🔥🔥🔥

TODO:

  • PS1 prompt gets re-defined anyway so line had no effect
  • Test that making a new commit from the console is still okay (EDITOR vars)
  • Test that JAVA_TOOL_OPTIONS is still defined so that the JVM won't eat up all Gitpod's precious RAM
  • Test gp and its auto-completion
  • Test open and code aliases (nice to haves / but not mandatory)
  • GEM_HOME=/workspace/.rvm gets overwritten by nearly all Ruby projects anyway
  • We want a less crazy-custom Python setup so fine by me
  • Test cargo install persistence across restarts
  • I've never liked the BROWSER automatic behavior (i.e. "automatically open IDE preview when some specific web servers start") (also did that even work in Code?)

I guess most of these changes are fine, because we want fewer non-standard customizations:

  • except for gp and JAVA_TOOL_OPTIONS, which look dangerous and really should be tested
  • also, big question: is Full Workspace Backup enabled everywhere now? (If yes, it's fine to remove all /workspace hacks -- if not, this PR breaks Rust, Python and maybe Ruby package persistence)

@jankeromnes
Copy link
Contributor

jankeromnes commented Aug 2, 2021

Hmm, I can't seem to open any repository in the current deployment 😕

Screenshot 2021-08-02 at 22 48 13

I tried:

@AlexTugarev
Copy link
Member

AlexTugarev commented Aug 3, 2021

/werft run

👍 started the job as gitpod-build-csweichel-image-builder-get-rid-4899.20

@AlexTugarev
Copy link
Member

AlexTugarev commented Aug 3, 2021

Notes and Questions

  • new failure mode: cannot ensure Gitpod user exists: user: unknown user
  • this will break persistence of packages/modules installed outside of /workspace by default. looking forward to see FWB in action, which would equalize this issue. I'm sure you are aware of this. do we need to track this in an issue?
  • does this have any impact on snapshots/prebuilds?

@csweichel csweichel force-pushed the csweichel/image-builder-get-rid-4899 branch from 36533ee to 05dc5d5 Compare August 3, 2021 06:53
@roboquat roboquat removed the lgtm label Aug 3, 2021
@csweichel csweichel force-pushed the csweichel/image-builder-get-rid-4899 branch from fdf87d1 to c5a7b32 Compare August 3, 2021 08:47
@csweichel
Copy link
Contributor Author

Thanks @jankeromnes and @AlexTugarev for testing this - and sorry for the obvious blunder.

I've fixed the "unknown user" issue, added gp to the path and set JAVA_TOOL_OPTIONS.
I have yet to test the code and editor behaviour, but those should be superseded by registry-facade's behaviour at this point.

@csweichel csweichel force-pushed the csweichel/image-builder-get-rid-4899 branch from bb8f148 to e7b942d Compare August 3, 2021 09:52
@AlexTugarev
Copy link
Member

AlexTugarev commented Aug 3, 2021

New workspaces starting just fine.

The one that failed previously doesn't start. How can that issue be permanent?

Screen Shot 2021-08-03 at 12 12 56

Visiting https://csweichel-image-builder-get-rid-4899.staging.gitpod-dev.com/start/#apricot-dove-rz8cloog
let you visit a loop of Preparing...

@csweichel
Copy link
Contributor Author

@AlexTugarev I reckon that's because the "reconciliation loop" of ws-manager-bridge hasn't "stopped" the old workspace yet. It's still stuck in the stopping state from the last attempt

@csweichel
Copy link
Contributor Author

/hold cancel

@aledbf
Copy link
Member

aledbf commented Aug 4, 2021

/lgtm

@roboquat roboquat added the lgtm label Aug 4, 2021
@roboquat
Copy link
Contributor

roboquat commented Aug 4, 2021

LGTM label has been added.

Git tree hash: 029093ef5eda27ecc17d9e17c08f7eafd83f4445

@roboquat
Copy link
Contributor

roboquat commented Aug 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf

Associated issue: #4899

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

The pull request process is described 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

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.

[image-builder] Get rid of the Gitpod layer
6 participants