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

Make the image load lock configurable per runtime #7057

Closed
wants to merge 1 commit into from

Conversation

afbjorklund
Copy link
Collaborator

The container runtimes that have a central server daemon to
coordinate loads, don't need to lock image load at each client.

We still need to limit the number of simultaneous loads for
runtimes that load directly, in order to not run of resources.

Closes #6992

The container runtimes that have a central server daemon to
coordinate loads, don't need to lock image load at each client.

We still need to limit the number of simultaneous loads for
runtimes that load directly, in order to not run of resources.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 15, 2020
@k8s-ci-robot k8s-ci-robot requested review from medyagh and RA489 March 15, 2020 15:58
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2020
@tstromberg
Copy link
Contributor

Alternatively, what do you think about runtimes being specify a maximum number of simultaneous loads?

I theorize that 1 is not likely to be the right answer for any, but neither is 20.

@afbjorklund
Copy link
Collaborator Author

@tstromberg, yes we could at least add that to the "API" (i.e. change from a bool to an int)

I think there was a neat class for it, change the sync.Mutex for a thread pool / worker pool ?

@tstromberg
Copy link
Contributor

@tstromberg, yes we could at least add that to the "API" (i.e. change from a bool to an int)

I think there was a neat class for it, change the sync.Mutex for a thread pool / worker pool ?

I wouldn't call it a neat class, but It is not difficult to implement with buffered channels: https://gist.github.com/AntoineAugusti/80e99edfe205baf7a094

Alternatively, there are a handful of libraries which have packaged this up:

https://github.com/korovkin/limiter
https://github.com/sirsean/go-pool
https://github.com/nozzle/throttler

@tstromberg
Copy link
Contributor

Need any help with this PR?

@afbjorklund
Copy link
Collaborator Author

Need any help with this PR?

Yes, I was only trying to speed up the Docker case - haven't looked at the other ones (or pooling)

@afbjorklund
Copy link
Collaborator Author

I will not have the time to implement this "properly", so will close the simple attempt (this PR)

@afbjorklund afbjorklund closed this Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LoadImages in parallel
3 participants