Skip to content

Make ov base image updates overall more convenient and take immediate effect #4560

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

Merged
merged 5 commits into from
Jul 8, 2024

Conversation

sarayourfriend
Copy link
Collaborator

@sarayourfriend sarayourfriend commented Jun 26, 2024

Fixes

Fixes #4545 by @dhruvkb

Description

@dhruvkb and I came up with the fix during a donut chat just now, so I thought I'd go ahead and open the PR for it 😁

The PR ended up being slightly more complicated than just the solution Dhruv and I came up with. Additionally, I worked on this PR today on my laptop, which has been behaving very strangely and being suuuper slow for some things. I noticed a significant issue with dev_env/run.sh performance, and I made some additional improvements to increase ov's execution speed under most circumstances.

  1. The main fix for pipx is to use pipx install --global in the Dockerfile. Alongside configuring PIPX_GLOBAL_HOME to /pipx (it defaults to /opt/pipx), this has two effects that solve the problem described in the issue without creating a new one:
  2. It moves the virtual environments for pipx tools we install in the Dockerfile, such that they will not be overwritten by a reused /opt volume on the host.
  3. It retains the ability to ov pipx install without causing permissions issues on Linux hosts that would be caused if we moved pipx entirely out of opt for all cases, while also allowing users to retain those tools even if the base system is updated. The issue with using an entirely separate location for all of pipx was that it would always get wiped out if the base system is updated or changed, and in those cases, you'd unexpectedly have to reinstall a tool you relied on being available in ov. You could cache those in a volume, but then you're back to where we started. Instead, we separate the concerns of the base system's pipx install usage from the user, so that they don't overlap and run into each other.
  • This is identical to how npm install -g works, by the way, and you can prove that by ov which n showing that it's installed in /usr/local/bin just like ov which pdm will now show.
  1. pnpm store path can be super slow for some reason, and so can the pdm config queries. I've cached the relevant results of these queries into a new dev_env/.cache directory, and ov execution is noticeably faster on slower computers.
  2. While we can update the base image, the fix in the first point was not enough to solve the issue! I was still seeing pipx packages not show up. That's when I realised that the running container was never stopped and a new one started with the new image. Not even docker stop was enough because of how we would restart a stopped container anyway. Instead, I updated run.sh to detect whether the existing container is using the latest base system image, and if not, to replace it with one that is. This will unfortunately have an effect of wiping out modifications outside /opt, but that's already the case with our existing non-working strategy (where you need ov clean to get it to work, which actually destroys /opt as well). This way we always stay up to date with the base image, while keeping /opt safe in these circumstances. The main silent negative effect this will have is if you run dnf install in the container (to install some additional personal tool), you wouldn't have those packages installed anymore. We could devise a way of syncing personalisations of this sort, similar to .ov_aliases.json, for example.
  3. Finally, there was an issue with the way setup_env.sh handled PNPM_HOME. It was exporting PATH, but that only worked when setup_env was the entrypoint. Now that it's just a one-time script after ov init, that won't work. Instead, now the Dockerfile configures a pre-set location already in PATH in the base system, and setup-env just puts the right thing there when PNPM_HOME exists. On main, ov pnpm store path would fail with an error about the path not being right. On this branch, it should work.
  • This does mean that setup_env.sh needs to run as root inside the container. I already added the capability to run commands as root in Refactor ov to create a persistent container #4490, but that only affects linux hosts. macOS already runs everything as root in the container anyway, so you wouldn't have write issues when creating the symlink to PNPM_HOME.

I added pgcli to the base system image as a test, I thought I'd just leave it and we could eventually replace running pgcli inside the various containers with just running it in ov directly (and then remove it as a dev dependency from our apps!).

Testing Instructions

ov init then install a pipx package like, ov pipx install mycli and confirm that (a) you can install it, (b) you can run it (ov mycli --help), (c) you cannot reproduce the problem from the issue.

Confirm after ov init, you get a message logged about creating a new container with the latest base system image. Confirm that this does not affect modifications in /opt such as personally installed pipx packages (e.g., mycli is retained after ov init). Remove pgcli from the Dockerfile, and ov init, and then confirm that ov which pgcli can't find it.

Checklist

  • My pull request has a descriptive title (not a vague title likeUpdate index.md).
  • My pull request targets the default branch of the repository (main) or a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • [N/A] I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.
  • [N/A] I ran the DAG documentation generator (./ov just catalog/generate-docs for catalog
    PRs) or the media properties generator (./ov just catalog/generate-docs media-props
    for the catalog or ./ov just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@openverse-bot openverse-bot added 🟨 priority: medium Not blocking but should be addressed soon 🛠 goal: fix Bug fix 🤖 aspect: dx Concerns developers' experience with the codebase 🧱 stack: mgmt Related to repo management and automations labels Jun 26, 2024
@sarayourfriend
Copy link
Collaborator Author

I'm working on a broader change for this, because just doing this doesn't help. There are a couple of problems that make an evolving environment cumbersome that we can fix, I think. The main issue is that updates to the base image don't automatically take effect in the running container, so even with this change, new base-system packages aren't immediately available. Technically just docker stop <container> && docker rm <container> would fix that, but we need to be selective of when to do it of course.

Since #4490, we reuse the same container each time. That's great, but when the base image changes, we aren't automatically using it, and we should be doing so (otherwise you have to manually run ov clean which also destroys the volume, which doesn't need to be destroyed if the base image changes.

I have changes that are almost working for this I'll push soon.

Also make run.sh a lot faster for most runs
@sarayourfriend sarayourfriend marked this pull request as ready for review June 27, 2024 02:01
@sarayourfriend sarayourfriend requested a review from a team as a code owner June 27, 2024 02:01
@sarayourfriend sarayourfriend changed the title Keep pipx outside opt so its packages are updatable Make ov base image updates overall more convenient and take immediate effect Jul 1, 2024
Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

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

Really cool. The testing instructions worked perfectly locally. ov pnpm store path works too.

I am not too worried about losing personal customizations, as I don't intend to make any myself.

Copy link
Member

@dhruvkb dhruvkb left a comment

Choose a reason for hiding this comment

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

While I would love to be able to extend the ov container with custom commands, we need to keep ov simple enough that supporting it never becomes more work than maintaing Openverse.

The changes look good and make sense based on the goals in the description. Nevertheless, I will checkout this PR and use it for a while so that I can make sure these changes work just as well on macOS and there aren't any strange edge cases.

@sarayourfriend
Copy link
Collaborator Author

It's true, and because host-machine tools still work, you can pipe ov results to and from, I guess it's not necessary for someone to be able to install and keep a tool inside ov.

Regardless, this makes certain things (like optional extensions, for example, to support Playwright inside of ov instead of another container) possible without necessarily overloading the base image. In any case, it's nice to not close the door on that, if a need aside from minute customisations arises.

@sarayourfriend sarayourfriend merged commit 3870967 into main Jul 8, 2024
49 checks passed
@sarayourfriend sarayourfriend deleted the fix/pipx-env branch July 8, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 aspect: dx Concerns developers' experience with the codebase 🛠 goal: fix Bug fix 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: mgmt Related to repo management and automations
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Install pipx packages outside the dev-env volume
4 participants