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

investigate, refactor preload tar balls #7804

Open
medyagh opened this issue Apr 20, 2020 · 5 comments
Open

investigate, refactor preload tar balls #7804

medyagh opened this issue Apr 20, 2020 · 5 comments
Labels
co/preload kind/process Process oriented issues, like setting up CI lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@medyagh
Copy link
Member

medyagh commented Apr 20, 2020

when doing preload PR for containerd I had a couple ideas and question

if user adds images using cache add

if they add one couple images using cache add ( as part of a development workflow)

if they start agian are we invoking a reset on all pull ?

// DockerImagesPreloaded returns true if all images have been preloaded
func DockerImagesPreloaded(runner command.Runner, images []string) bool {
	rr, err := runner.RunCmd(exec.Command("docker", "images", "--format", "{{.Repository}}:{{.Tag}}"))
	if err != nil {
		return false
	}
	preloadedImages := map[string]struct{}{}
	for _, i := range strings.Split(rr.Stdout.String(), "\n") {
		preloadedImages[i] = struct{}{}
	}

	glog.Infof("Got preloaded images: %s", rr.Output())

	// Make sure images == imgs
	for _, i := range images {
		if _, ok := preloadedImages[i]; !ok {
			glog.Infof("%s wasn't preloaded", i)
			return false
		}
	}
	return true
}

can we refactor to DockerImagesPreloaded to return list of not preloaded images ( instead of bool)

so we only pull and load the images that do not exist as opposed load all images again ?

@medyagh medyagh added co/preload priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Apr 20, 2020
@tstromberg tstromberg added the kind/process Process oriented issues, like setting up CI label Apr 21, 2020
@medyagh medyagh changed the title investigate, if preload restarts when not needed- investigate, refactor preload tar balls Apr 22, 2020
@medyagh
Copy link
Member Author

medyagh commented Apr 22, 2020

alternatively we could separate the tarballs for kubernetes core images and aux images and binaries

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 21, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 20, 2020
@priyawadhwa
Copy link

priyawadhwa commented Sep 16, 2020

This would improve performance, but before we do LoadImage we check if the image already exists in the daemon in the needsTransfer function --

func needsTransfer(imgClient *client.Client, imgName string, cr cruntime.Manager) error {

My guess is we're looking at a small performance improvement (<1s), but it should be easy enough to do, especially if we compare preloaded images to images that need to be loaded here:

if cr.ImagesPreloaded(images) {

@priyawadhwa priyawadhwa added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Sep 23, 2020
@priyawadhwa priyawadhwa added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Jan 25, 2021
@priyawadhwa priyawadhwa removed their assignment Feb 26, 2021
@priyawadhwa
Copy link

If we end up implementing the idea mentioned here -- #10438 (comment) -- then this issue is obsolete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
co/preload kind/process Process oriented issues, like setting up CI lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

No branches or pull requests

5 participants