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

Move cluster image loading logic into Deployer #6124

Merged
merged 4 commits into from
Jul 7, 2021

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Jul 1, 2021

Related: #6117, #6107

Description

The Runner has historically been responsible for loading images into local kubernetes clusters before running, as is required by kind, k3d, and even minikube (when running with containerd). However, this is really a behavior that should be carried out by the Deployer, as it is the component that has knowledge of the cluster - the Runner is just an orchestrator.

This change creates an ImageLoader component, which is responsible for performing any image loading into the cluster before doing a deploy.

This component has a single method on it:

type ImageLoader interface {
  LoadImages(context.Context, io.Writer, images []graph.Artifact)
}

A single implementation of this interface is added in the pkg/skaffold/kubernetes/loader package, which embeds the existing image load logic from the Runner inside of it.

One method is added to the Deployer interface to give the Runner the ability to pass images it has marked as local:

type Deployer interface {
  ...

  RegisterLocalImages([]graph.Artifact)
}

Update: Each Deployer is now responsible for loading the set of images that it will be deploying, similar to the way each Deployer only tracks its own images for logging. This is determined by the union of the following 3 sets:

  • Images which have been deemed by the Runner as local
  • Images which have been found in a Deployer's set of manifests
  • Images which are passed by the Deployer to be loaded

@codecov
Copy link

codecov bot commented Jul 1, 2021

Codecov Report

Merging #6124 (3b44ece) into master (fe12a8a) will decrease coverage by 0.02%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6124      +/-   ##
==========================================
- Coverage   69.96%   69.94%   -0.03%     
==========================================
  Files         477      481       +4     
  Lines       18283    18336      +53     
==========================================
+ Hits        12792    12825      +33     
- Misses       4545     4560      +15     
- Partials      946      951       +5     
Impacted Files Coverage Δ
pkg/skaffold/deploy/deploy_mux.go 53.44% <0.00%> (-0.94%) ⬇️
pkg/skaffold/deploy/kubectl/cli.go 88.09% <ø> (ø)
pkg/skaffold/loader/loader_mux.go 0.00% <0.00%> (ø)
pkg/skaffold/runner/v1/apply.go 0.00% <0.00%> (ø)
pkg/skaffold/deploy/helm/deploy.go 77.25% <42.85%> (-0.98%) ⬇️
pkg/skaffold/deploy/kpt/kpt.go 79.93% <42.85%> (-0.93%) ⬇️
pkg/skaffold/deploy/kustomize/kustomize.go 72.95% <42.85%> (-1.39%) ⬇️
pkg/skaffold/loader/provider.go 42.85% <42.85%> (ø)
pkg/skaffold/loader/loader.go 50.00% <50.00%> (ø)
pkg/skaffold/kubernetes/loader/load.go 66.15% <66.15%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe12a8a...3b44ece. Read the comment docs.

@gsquared94
Copy link
Contributor

I tried this on examples/microservices and examples/multi-config-microservices with the default kind cluster but the image load doesn't happen correctky.

Loading images into kind cluster nodes...
Images loaded in 588ns
Loading images into kind cluster nodes...
Images loaded in 181ns
Starting deploy...
 - service/leeroy-app created
 - deployment.apps/leeroy-app created
 - deployment.apps/leeroy-web created
Waiting for deployments to stabilize...
 - deployment/leeroy-app: Unschedulable: 0/1 nodes available: 1 node is not ready
    - pod/leeroy-app-cd547645c-dcmrw: Unschedulable: 0/1 nodes available: 1 node is not ready
 - deployment/leeroy-web: Unschedulable: 0/1 nodes available: 1 node is not ready
    - pod/leeroy-web-77b44f4bc4-wvv98: Unschedulable: 0/1 nodes available: 1 node is not ready
 - deployment/leeroy-app: container leeroy-app is waiting to start: leeroy-app:49e54603b1b22ca355856617f5a8138de3c355492ee6752b26a0ed6ccc3cb51d can't be pulled
    - pod/leeroy-app-cd547645c-dcmrw: container leeroy-app is waiting to start: leeroy-app:49e54603b1b22ca355856617f5a8138de3c355492ee6752b26a0ed6ccc3cb51d can't be pulled
 - deployment/leeroy-app failed. Error: container leeroy-app is waiting to start: leeroy-app:49e54603b1b22ca355856617f5a8138de3c355492ee6752b26a0ed6ccc3cb51d can't be pulled.
Cleaning up...
 - service "leeroy-app" deleted
 - deployment.apps "leeroy-app" deleted
 - deployment.apps "leeroy-web" deleted
2/2 deployment(s) failed

@gsquared94
Copy link
Contributor

also, the image load is happening multiple times in a single dev loop for multiple deployers (or multi config)

Loading images into kind cluster nodes...
Images loaded in 588ns
Loading images into kind cluster nodes...
Images loaded in 181ns

@pull-request-size pull-request-size bot added size/XL and removed size/L labels Jul 2, 2021
@gsquared94
Copy link
Contributor

lgtm. An alternative approach probably could be to modify graph.Artifact to directly have a field isLocal when the Builder returns the result and do away with RegisterLocalImages method on the Deployer.

@nkubala nkubala enabled auto-merge (squash) July 2, 2021 22:34
@nkubala nkubala merged commit 92dc3ab into GoogleContainerTools:master Jul 7, 2021
@nkubala nkubala deleted the image-loader branch July 7, 2021 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants