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

Add options for build container rsync optimization #36361

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Nov 7, 2016

KUBE_RSYNC_COMPRESS env var sets rsync compression level.
KUBE_RSYNC_GENERATED_TO_BUILD_CONTAINER env var disables rsyncing
generated files to build containers.

Why KUBE_RSYNC_COMPRESS is needed -- from rsync manual on --compress option (implied by non-zero --compress-level):

Note that this option typically achieves better compression ratios than can be achieved by using a compressing remote shell or a compressing transport because it takes advantage of the implicit information in the matching data blocks that are not explicitly sent over the connection.

Use case for KUBE_RSYNC_GENERATED_TO_BUILD_CONTAINER: when you sometimes build stuff locally (e.g. make WHAT=cmd/kubectl) and sometimes do it on remote docker (build-tools/run.sh make WHAT=cmd/hyperkube), local builds touch generated files which causes them to be rsynced to the build data container, which may slow down the builds. Still, I'm not sure whether local->remote rsync of generated files is useful (e.g. someone may want to edit generated files for debugging purposes?), so I made not rsyncing these files an option instead of forcing such behavior.


This change is Reviewable

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Nov 7, 2016
@ivan4th ivan4th force-pushed the build-container-rsync-optimizations branch 2 times, most recently from 5bbb369 to a9d8e01 Compare November 7, 2016 13:49
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 5bbb3695a7138144c1d50e3ea667f25e004dc8fc. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 5bbb3695a7138144c1d50e3ea667f25e004dc8fc. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 7, 2016

@k8s-bot gci gce e2e test this

1 similar comment
@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 7, 2016

@k8s-bot gci gce e2e test this

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@apelisse apelisse removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@zmerlynn
Copy link
Member

Sorry for the slow response. I'm inclined to push back on this because it's a set of knobs for the build system that are unlikely to be tested in any continuous integration setting. Unless they have a lot of value being changed by default (which e.g. compression may), I don't think we need more knobs here.

cc @jbeda

@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 22, 2016

Does it make sense to copy locally generated files to the build container then? Maybe if knobs aren't acceptable, we may just always skip copying these files? As of compression, I suspect it doesn't add that much overhead and we can enable it by default, wdyt? It helps if you use remote docker that's indeed quite remote. Although if that's problematic I can remove this part & just use ssh compression in such cases, as rsync port is forwarded anyway.

@jbeda
Copy link
Contributor

jbeda commented Nov 22, 2016

I'm cool with the "compress" option. It is semantically identical and can make sense when you have a remote docker daemon that you are pushing to. Compression should be off, by default as it will slow down the local docker case. It is worth measuring though. Note that right now there is no ssh compression by default as we are doing rsync over the rsync protocol, not via ssh. You'd get ssh compression if you are using a remote daemon with an ssh tunnel, but rsync doesn't know about that.

As for copying the generated files back to the build container -- we should probably never do that. I'm okay with changing this so that we only copy generated files FROM the build container and never TO the build container.

@jbeda jbeda added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Nov 22, 2016
@zmerlynn zmerlynn removed their assignment Nov 22, 2016
@ivan4th ivan4th force-pushed the build-container-rsync-optimizations branch from a9d8e01 to 0162b4e Compare November 22, 2016 20:02
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 22, 2016
@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 22, 2016

@jbeda updated the PR, now it always ignores generated files when rsyncing to the container. As of compress option, yes, it's off by default.

@k8s-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit 0162b4e42bb4351fb5d5a79e623ceb231c0bbd7c. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 0162b4e42bb4351fb5d5a79e623ceb231c0bbd7c. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 0162b4e42bb4351fb5d5a79e623ceb231c0bbd7c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 0162b4e42bb4351fb5d5a79e623ceb231c0bbd7c. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 0162b4e42bb4351fb5d5a79e623ceb231c0bbd7c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 0162b4e42bb4351fb5d5a79e623ceb231c0bbd7c. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 0162b4e42bb4351fb5d5a79e623ceb231c0bbd7c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 22, 2016

Hmm looks like not copying generated files to the container is not going to work, let me see

@ivan4th ivan4th force-pushed the build-container-rsync-optimizations branch 2 times, most recently from f7258ab to c68f0bd Compare November 22, 2016 21:28
@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 22, 2016

There was a problem with staging/ - generated files from there need to be rsynced to the container anyway because make doesn't re-generate them. Fixed the filter, should work now

@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit c68f0bded6f384c5dd1ac61997693677a22b3762. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 22, 2016

Flake similar to (3) from #35935 (comment)

@k8s-bot cri node e2e test this

KUBE_RSYNC_COMPRESS env var sets rsync compression level.
KUBE_RSYNC_GENERATED_TO_BUILD_CONTAINER env var disables rsyncing
generated files to build containers.
@ivan4th ivan4th force-pushed the build-container-rsync-optimizations branch from c68f0bd to 2256b27 Compare November 23, 2016 02:40
@k8s-ci-robot
Copy link
Contributor

Jenkins Bazel Build failed for commit 2256b27. Full PR test history.

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 24, 2016

@k8s-bot bazel test this

@ivan4th
Copy link
Contributor Author

ivan4th commented Dec 8, 2016

ping...

@jbeda
Copy link
Contributor

jbeda commented Dec 11, 2016

/lgtm

Sorry for dropping this. Was waiting for the test issues to be figured out.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE e2e failed for commit 2256b27. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 38277, 36361, 38452)

@k8s-github-robot k8s-github-robot merged commit 48cae78 into kubernetes:master Dec 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants