Add localdir buildcontext to kaniko builder by priyawadhwa · Pull Request #983 · GoogleContainerTools/skaffold

priyawadhwa · 2018-09-14T00:29:06Z

The localdir buildcontext creates a configMap of all sources and mounts
it in to the kaniko pod. It points the kaniko context to the mounted
directory.

Begins to fix #950

The localdir buildcontext creates a configMap of all sources and mounts it in to the kaniko pod. It points the kaniko context to the mounted directory.

…ted skaffold

r2d4 · 2018-09-14T16:34:39Z

pkg/skaffold/build/kaniko/run.go

+	for _, path := range paths {
+		files = append(files, []string{"--from-file", path}...)
+	}
+	cmd := exec.Command("kubectl", append([]string{"create", "configmap", configMapName(tag), "-n", ns}, files...)...)


I'm not super familiar with this strategy, but this just stores the actual files in configmaps and mounts them? AFAIK the size limit for configmaps is the size limit for keys in etcd, which is 1.5 MB by default.

Have you tried this with larger repositories?

I looked into it and you're right, this probably won't work with larger repositories.

What if we created a tarball of the files and mounted that in via configMap? Would require adding a --context=tar:// option to kaniko but that should be pretty easy.

Is that a per-file limit or a limit on the overall size of the repo?

I think it's the overall size

What if we use Persitent Volumes? It would be a bit of a pain to maintain them - we'd have to take care of creating and cleaning up the PVs and figure out how to interact across namespaces as well potentially, but it would free the users from having to depend on any external storage solutions. If we figure this out, this might be useful for BuildCRD too.

WDYT of using an emptyDir to hold the build context? We could have an init container that runs while we kubectl cp the buildcontext into the emptyDir volume, and then mount that into the kaniko container.

I think the emptyDir can use up to however much memory is allocated to the cluster.

It would allow for easier cleanup than a PV/PVC, although would be slower since we have to copy the entire buildcontext over for every dev loop.

It could be a good place to start, and if it isn't working for people we can switch to the PV/PVC?

Thats assuming that the entire configmap fits in a 1.5 MB key.

If it doesn't:

If you don't shard, then a very small number of repos are going to be able to fit in 1.5 MB.

If you shard, you end up with a key per 1.5 MB, which for the skaffold repo would be 110 keys (164M/1.5M). The storage limit for etcd v3 is 2 GB but supports up to 8 GB. I'm not sure what the performance would be on that, but using 8% of your etcd for a build context could easily reach a performance limit pretty quickly on a shared cluster.

I like @priyawadhwa's idea

use the skaffold docker context generation to create a tarball as usual

run the kaniko pod with an init container that waits for a special file to be written to signify completion

kubectl cp the tarball over to the emptyDir

kubectl cp the special completion file

init container completes

kaniko runs

On the other hand, if we reuse a PV to store the build context, we can do incremental build context uploads, which will be very fast.

Cool, I think we might have to extract the tarball within the init container as well, since that's what kaniko expects, but that shouldn't be difficult (or we could add the a tar:// build context to kaniko, I'm good with that as well).

@balopat are you good with trying this out?

yeah, if 1.5MB is not good for most of the repos, then configmap doesn't work.
+100 on @priyawadhwa's idea - and switch over to PVs as a follow-up feature for incremental builds later down the line if there's enough usage to warrant it

r2d4 · 2018-09-14T16:35:36Z

pkg/skaffold/build/kaniko/run.go

+	}}
+
+	context := ""
+	if cfg.BuildContext.GCSBucket != "" {


I would change this to a switch statement, and maybe break out the implementations into their own functions

The function could be something like func(ctx, artifact, cfg.BuildContext, cfg.Namespace, initialTag) (context, func())

r2d4 · 2018-09-14T16:39:38Z

examples/kaniko-local/skaffold.yaml

+  - imageName: gcr.io/k8s-skaffold/skaffold-example
+  kaniko:
+    buildContext: 
+      localDir: true


maybe this should just be a struct? localDir: {}?

specifying localDir: false doesn't really make sense

+1, if you make these pointers to structs in the config, the oneOf yamltag will do implicit validation for you.

nkubala · 2018-09-18T17:15:53Z

pkg/skaffold/schema/v1alpha3/config.go

 // a kaniko build context
 type KanikoBuildContext struct {
 	GCSBucket string `yaml:"gcsBucket,omitempty" yamltags:"oneOf=buildContext"`
+	LocalDir  bool   `yaml:"localDir,omitempty" yamltags:"oneOf=buildContext"`


I don't think using the oneOf tag with different types will work. IIRC the type needs to be a shared pointer type, don't think they can be primitives even if they're the same (since the "nil" type for e.g. a string isn't actually nil, it's "")

nkubala · 2018-09-18T17:17:31Z

examples/kaniko-local/skaffold.yaml

+  - imageName: gcr.io/k8s-skaffold/skaffold-example
+  kaniko:
+    buildContext: 
+      localDir: true


+1, if you make these pointers to structs in the config, the oneOf yamltag will do implicit validation for you.

shashankkoppar · 2018-10-01T01:49:57Z

Any update on this?

priyawadhwa · 2018-10-01T22:00:20Z

Hey @shashankkoppar we've been discussing an alternate solution in the comments on this PR, I should have something ready for review in the next day or two.

shashankkoppar · 2018-10-02T05:30:55Z

@priyawadhwa thanks , currently working on including kaniko build and push part of our CI/CD. So this will really help. For now, have been using Jenkins PodTemplate to spin up kaniko pod

…o localdir

I created a BuildContextSource interface to interact with setup and cleanup for kaniko build contexts. I transferred the code for GCS buckets there, and added the LocalDir build context. This should hopefully make it easier to add more build contexts (such as S3 buckets) in the future. I also moved the integration test from examples into integration/examples.

codecov-io · 2018-10-03T00:19:37Z

Codecov Report

Merging #983 into master will decrease coverage by 0.14%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
- Coverage   42.96%   42.81%   -0.15%     
==========================================
  Files          74       74              
  Lines        3403     3419      +16     
==========================================
+ Hits         1462     1464       +2     
- Misses       1801     1815      +14     
  Partials      140      140

Impacted Files	Coverage Δ
pkg/skaffold/kubernetes/wait.go	`26.19% <16.66%> (-3.23%)`	⬇️

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 b809bd5...a87a459. Read the comment docs.

r2d4

Nice! A few comments. I’ll give it a go tomorrow. Looks good though

r2d4 · 2018-10-03T00:33:12Z

pkg/skaffold/build/kaniko/sources/gcs.go

+// Setup uploads the context to the provided GCS bucket
+func (g *GCSBucket) Setup(ctx context.Context, artifact *latest.Artifact, cfg *latest.KanikoBuild, initialTag string) (string, error) {
+	g.tarName = fmt.Sprintf("context-%s.tar.gz", initialTag)
+	if err := docker.UploadContextToGCS(ctx, artifact.Workspace, artifact.DockerArtifact, cfg.BuildContext.GCSBucket, g.tarName); err != nil {


What if these returned a io.WriterCloser and the actual context generation and writing happened outside each source’s setup?

r2d4 · 2018-10-03T00:35:34Z

pkg/skaffold/build/kaniko/sources/localdir.go

+}
+
+// waitForInitContainer tries to exec into the init container until it doesn't fail
+func waitForInitContainer(p *v1.Pod) error {


Query the apiserver for init container status instead?

There’s some example of pod watchers in the code.

We should remove all the PollImmediates when we get a chance. I added those a long time ago in minikube before I did it right.

yah that's way better, added it in, thanks!

r2d4 · 2018-10-03T00:38:15Z

pkg/skaffold/build/kaniko/sources/localdir.go

+	// Generate the init container, which will run until the /tmp/complete file is created
+	ic := v1.Container{
+		Name:  initContainer,
+		Image: constants.DefaultDebianImage,


Can we use something smaller like alpine? Debian seems a bit heavy. The only requirement is the tar binary right?

r2d4 · 2018-10-03T00:40:08Z

pkg/skaffold/schema/latest/config.go

 type KanikoBuildContext struct {
-	GCSBucket string `yaml:"gcsBucket,omitempty" yamltags:"oneOf=buildContext"`
+	GCSBucket string    `yaml:"gcsBucket,omitempty" yamltags:"oneOf=buildContext"`
+	LocalDir  *struct{} `yaml:"localDir,omitempty" yamltags:"oneOf=buildContext"`


Might as well make this its own type

Query apiserver while waiting for init container to start; use alpine image since it's more lightweight than debian.

balopat · 2018-10-08T13:52:28Z

@priyawadhwa can you please rebase? we should get this in soon then. Thank you!

priyawadhwa · 2018-10-08T18:44:27Z

Done!

priyawadhwa · 2018-10-09T18:53:23Z

Hey @nkubala @dgageot, any chance you could take a look at this? I think it's almost there, just wanted to get it in before the release Thursday :)

priyawadhwa · 2018-10-10T17:36:14Z

Thanks @dgageot !

Priya Wadhwa added 2 commits September 13, 2018 17:20

Add localdir buildcontext to skaffold builder

edb2d1f

The localdir buildcontext creates a configMap of all sources and mounts it in to the kaniko pod. It points the kaniko context to the mounted directory.

Add integration test for kaniko local dir buildcontext and fix annota…

a57f082

…ted skaffold

priyawadhwa requested review from balopat, dgageot and r2d4 as code owners September 14, 2018 00:29

priyawadhwa changed the title ~~Add localdir buildcontext to skaffold builder~~ Add localdir buildcontext to kaniko builder Sep 14, 2018

Create configmap in same namespace

3f597e2

r2d4 reviewed Sep 14, 2018

View reviewed changes

nkubala reviewed Sep 18, 2018

View reviewed changes

Priya Wadhwa added 2 commits October 2, 2018 16:48

Merge branch 'master' of github.com:GoogleContainerTools/skaffold int…

c06a59e

…o localdir

priyawadhwa force-pushed the localdir branch from ee6852a to 97e29a1 Compare October 3, 2018 00:14

r2d4 reviewed Oct 3, 2018

View reviewed changes

Address code review comments

8143f6c

Query apiserver while waiting for init container to start; use alpine image since it's more lightweight than debian.

rebased

a87a459

balopat mentioned this pull request Oct 8, 2018

Release v0.16.0 #1121

Closed

dgageot approved these changes Oct 10, 2018

View reviewed changes

dgageot merged commit dd7f219 into GoogleContainerTools:master Oct 10, 2018

priyawadhwa deleted the localdir branch October 10, 2018 17:36

Conversation

priyawadhwa commented Sep 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priyawadhwa Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shashankkoppar commented Oct 1, 2018

Uh oh!

priyawadhwa commented Oct 1, 2018

Uh oh!

shashankkoppar commented Oct 2, 2018

Uh oh!

codecov-io commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

r2d4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balopat commented Oct 8, 2018

Uh oh!

priyawadhwa commented Oct 8, 2018

Uh oh!

priyawadhwa commented Oct 9, 2018

Uh oh!

priyawadhwa commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

priyawadhwa Oct 1, 2018 •

edited

Loading

codecov-io commented Oct 3, 2018 •

edited

Loading