-
Notifications
You must be signed in to change notification settings - Fork 246
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
[ci] Fix bug where we pushed test images to the main image cache #11907
Conversation
Actually, maybe we just don't want to push to any cache at all for test deployments. |
Tests pushing to the cache is important. For example, if a PR adds a new apt-get dependency, only the first build should have to rebuild the image. Subsequent commits / retries should be fast. |
self.cache_repository = f'{DOCKER_PREFIX}/{self.publish_as}:cache' | ||
else: | ||
self.cache_repository = f'{DOCKER_PREFIX}/ci-intermediate:cache' | ||
self.cache_repository = f'{DOCKER_PREFIX}/ci-intermediate/{self.publish_as}:cache' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if publish_as is None, this seems wrong
I'm missing something. Why shouldn't test deployments benefit from and contribute to the cache? Why isolate them somewhere else? |
I thought the purpose of the cache was to cache the latest version in production. Let's take service-base as an example. There's the deployment in production that we care about. But every PR is now going to change the cache each time to what it thinks service-base is. This means that the last 4 layers for service-base will change for every time we run a test PR and it changes hailtop, gear, or web-common. If you don't like this change, then feel free to close it. |
Ah, I understand. You're correct that pushing to the AFAICT, this change doesn't prevent PR tests from pushing to the AFAICT, this
Will use as a cache source the In particular, every push to the
If I rebuild [1] the most recently pushed image with
it succeeds in getting the cache. If I rebuild the other image with the same import-cache, it does not see that the (untagged) image is already there! This all suggests that all our attempts at image caching are failing terribly. Options:
It seems like 3 is actually a decent solution that should enable lots of caching.
What do you think of the #3 proposal? [1]: I had two files:
To build I use this command (slightly different syntax from the buildctl syntax, but, AFAIK, uses the same backend):
Before every build I clear the local cache with:
I can clear the remote cache with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x
3 is good. I'll try and find time next week to work on this. |
We don't want this in PR namespaces when running
test_ci