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

Specify tags & pullPolicy for alpine/git: image #3165

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

yuvipanda
Copy link
Member

Without this, the :latest tag was being used, and we were checking the registry each time even if the image is already present. Makes deployments slightly faster

Without this, the :latest tag was being used, and we were checking
the registry each time even if the image is already present. Makes
deployments *slightly* faster
@yuvipanda yuvipanda requested a review from a team as a code owner September 21, 2023 17:53
@github-actions

This comment was marked as resolved.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Don't merge this yet - restarting the hub atm could cause issues until we have a juyterhub image using the latest kubespawner with a bugfix.

@damianavila
Copy link
Contributor

we need #3165 merged first!

I think you mislinked some other PR, right?

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Yepp @damianavila, I meant #3168 - now merged!

Comment on lines +408 to +409
image: alpine/git:2.40.1
imagePullPolicy: IfNotPresent
Copy link
Member

@consideRatio consideRatio Sep 25, 2023

Choose a reason for hiding this comment

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

I think using a tag besides latest, or to use latest and specify IfNotPresent, is sufficient to ensure the behavior of IfNotPresent I think.

In a way I like that we use :latest here rather than having another image that can get outdated or that we automatically merge automated version bumps without further review because we assume its fine no matter what?

To pin:

  • A minus: we ought to have automation to bump it over time to avoid security issues

To not pin:

  • A minus: we don't know for sure when something updates, because :latest combined with IfNotPresent makes us not know that

In this case, I think I'm leaning towards favoring not needing the complexity of updating this automatically or the alternative drawback of possibly exposing security vulernabilities that we don't patch.

Reference

When you (or a controller) submit a new Pod to the API server, your cluster sets the imagePullPolicy field when specific conditions are met:

  • if you omit the imagePullPolicy field, and you specify the digest for the container image, the imagePullPolicy is automatically set to IfNotPresent.
  • if you omit the imagePullPolicy field, and the tag for the container image is :latest, imagePullPolicy is automatically set to Always;
  • if you omit the imagePullPolicy field, and you don't specify the tag for the container image, imagePullPolicy is automatically set to Always;
  • if you omit the imagePullPolicy field, and you specify the tag for the container image that isn't :latest, the imagePullPolicy is automatically set to IfNotPresent.

Copy link
Member

Choose a reason for hiding this comment

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

We can go for this as it is though, I mostly wanted to ensure that that we don't spread an understanding that we would need IfNotPresent specified alongside any image tag beisdes :latest

@yuvipanda yuvipanda merged commit 8981967 into 2i2c-org:master Sep 26, 2023
34 checks passed
@github-actions
Copy link

🎉🎉🎉🎉

Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/6317281190

yuvipanda added a commit to yuvipanda/pilot-hubs that referenced this pull request Sep 27, 2023
This makes each user server start faster, as kubernetes
doesn't try to reach out to the docker registry to check
for correct version of the busybox image to pull each time
a user server starts.

Follow-up to 2i2c-org#3165
yuvipanda added a commit to yuvipanda/pilot-hubs that referenced this pull request Sep 28, 2023
This makes each user server start faster, as kubernetes
doesn't try to reach out to the docker registry to check
for correct version of the busybox image to pull each time
a user server starts.

Follow-up to 2i2c-org#3165
yuvipanda added a commit to yuvipanda/pilot-hubs that referenced this pull request Sep 29, 2023
- Git clone into a subdirectory of the `emptyDir` mount, as the
  mount itself is default owned by root. This was the cause of
  2i2c-org#1695, which
  was previously solved by adding another initContainer. This PR
  removes that, speeding up starts.
- Remove existing directory if it exists before cloning. This prevents
  the container from failing when the *pod* is restarted due to node
  issues, as that doesn't clear out `emptyDirs`. And git freaks out
  if the repo already exists
- Remove redundant `IfNotPresent` imagePullPolicy from alpine/git,
  as that is not needed when we specify a non :latest tag. Thanks
  to Erik's investigation in 2i2c-org#3165 (comment)

Ref 2i2c-org#3194
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

4 participants