Skip to content

Conversation

@yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Jul 31, 2025

This PR adds a new pull policy such that we can handle the image with the update easily.
It adds one more check on top of if-not-present.
To avoid download docker image for checking, I go for the rest api to grab the push time
For SIF, we can get the information from inspect. Note: the build time is not the same as push time.
It only works when remote is docker.

I test the function in small rust own project but I am not sure whether there is easy way to test it here.
Also, to distinguish only local SIF (not in remote), I add local://

@yhmtsai yhmtsai requested a review from upsj July 31, 2025 11:53
@yhmtsai yhmtsai self-assigned this Jul 31, 2025
@upsj
Copy link
Member

upsj commented Jul 31, 2025

That would be a divergence from what the docker gitlab-runner executor supports. I think it would be better if we mirror their config files as much as possible. The underlying problem we're trying to solve is that of DockerHub rate limits, and we could fix that by using a different container registry (e.g. one on gitlab)
For that, we may need to implement docker authentication, but I would still prefer it to diverging from gitlab-runner.

}

// This uses docker hub rest api to grab the time from last push.
fn get_docker_tag_timestamp(pull_url: &str) -> anyhow::Result<DateTime<Utc>> {
Copy link
Member

Choose a reason for hiding this comment

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

If we were to add this, the function should preferably be async and the reqwest dependency should not use blocking

Copy link
Member Author

Choose a reason for hiding this comment

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

is the function calling this just async not multithread?
I thought it is multithread, so doing blocking in that thread was fine to me.

Copy link
Member

Choose a reason for hiding this comment

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

The functions are async, how they are actually being executed (multi-threaded or single-threaded) is up to the runtime system. The only as I have is that they should be non-blocking, in case we want to do other things at the same time in the future/make the requests cancellable.

@yhmtsai
Copy link
Member Author

yhmtsai commented Aug 1, 2025

@upsj I do not think this repo needs to provide the exact same config as gitlab although it can reduce the learning rate from gitlab.
However, I check the pull behavior from apptainer (at least) which might not introduce the rate limit anymore we faced before.
When pulling the exact same docker image, it does not increase the number of pull.
(I pull it 200 times, it is increased by one in the middle. I do not know whether it is from apptainer cache checking or someone else pull that)
Thus, we can use always pull policy without facing the rate limit unless the disk write/copy gives another concern.
Although we still need local to distinguish the only local sif from always policy, it will be another pr.

@upsj
Copy link
Member

upsj commented Aug 1, 2025

@yhmtsai You're right, and I might have mixed up where the config variables are set. Still it sounds like these changes may not be necessary after all?

@yhmtsai
Copy link
Member Author

yhmtsai commented Aug 1, 2025

yes, I will close it because always pull policy behavior should be good enough now

@yhmtsai yhmtsai closed this Aug 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants