git: Use VolumeHost.GetExec() to execute stuff in volume plugins #51098

jsafrane · 2017-08-22T11:22:58Z

This prepares volume plugins to run things in containers instead of running
them on the host.

Special notes for your reviewer:

instead of cmd.SetDir(<dir>); cmd.Exec("git <command>"), we do cmd.Exec("git -C <dir> <command>") - mounter.Exec does not have SetDir()
there are lot of changes in the tests because a different exec interface is used.

@kubernetes/sig-storage-pr-reviews

gitRepo volumes in pods require git 1.8.5 or later

/assign @rootfs

jsafrane · 2017-08-22T11:59:30Z

/sig storage

jsafrane · 2017-08-22T12:59:18Z

/retest

rootfs · 2017-08-22T13:08:10Z

pkg/volume/git_repo/git_repo_test.go

+	var commandLog []expectedCommand
+	execCallback := func(cmd string, args ...string) ([]byte, error) {
+		// command is 'git -C <dir> <command> <args>
+		gitCommand := args[2]


error out if len(args) is invalid

good catch, fixed. Now I check that arg[0] == "-C", just to be sure.

rootfs · 2017-08-22T13:18:29Z

pkg/volume/git_repo/git_repo_test.go

 		}
+		// add the command to log with de-randomized gitDir
+		args[1] = strings.Replace(gitDir, mounter.GetPath(), "volume-dir", 1)


based on the context, it should just return volume-dir right?

Yes, it should always return "volume-dir". However, if there is bug in the volume plugin it may be called with a different directory and the test should catch it.

why not just force it: args[1] = "volume-dir", replace doesn't reset the entire string

Because I want to check that correct arg[1] was used by the plugin. If the plugin uses /var/lib/kubelet/pods/<uid>/kubernetes.io~git/foo/123 instead of correct ..kubernetes.io~git/foo/, Replace will return volume-plugin/123 and the test (correctly) fails.

jsafrane · 2017-08-23T08:57:02Z

/retest

jsafrane · 2017-08-30T14:33:10Z

/test all

jsafrane · 2017-08-30T14:34:15Z

@rootfs not sure where we are with this, can it be merged?

rootfs · 2017-08-30T14:47:59Z

/approve no-issue
/lgtm

rootfs · 2017-08-30T17:59:11Z

/retest

fejta-bot · 2017-08-30T21:10:49Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-31T01:00:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-31T04:09:52Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

jsafrane · 2017-08-31T06:26:47Z

/retest

jsafrane · 2017-08-31T07:23:03Z

/retest

jsafrane · 2017-08-31T08:15:54Z

/retest

jsafrane · 2017-08-31T10:05:28Z

/retest

This prepares volume plugins to run things in containers instead of running them on the host.

jsafrane · 2017-08-31T11:34:42Z

Fixed e2e tests (luckily we have them): I forgot to fill exec here: https://github.com/kubernetes/kubernetes/pull/51098/files#diff-45aabca84f5daab0bb5d8194a7282018R104

Which led to panic when exec was actually used here: https://github.com/kubernetes/kubernetes/pull/51098/files#diff-45aabca84f5daab0bb5d8194a7282018R250

@rootfs PTAL

rootfs · 2017-08-31T12:26:41Z

/test pull-kubernetes-e2e-gce-etcd3

rootfs · 2017-08-31T12:26:51Z

/test pull-kubernetes-e2e-gce-bazel

jsafrane · 2017-08-31T14:08:09Z

UTC (durationBeforeRetry 500ms). Error: MountVolume.SetUp failed for volume "git-volume" (UniqueName: "kubernetes.io/git-repo/7b075d4e-8e4c-11e7-bd35-42010a800002-git-volume") pod "pod-secrets-7b0481e5-8e4c-11e7-a485-0a580a3c456b" (UID: "7b075d4e-8e4c-11e7-bd35-42010a800002") : failed to exec 'git clone http://10.0.32.88:2345 test':
Unknown option: -C
usage: git [--version] [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
           [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
           [-c name=value] [--help]
           <command> [<args>]
: exit status 129

Damn

jsafrane · 2017-09-01T11:58:59Z

The best way out of this is to add exec.SetDir(), just like util/exec does. But this cannot be easily implemented either in controller manager that will need to execute attach/detach utils via something like kubectl exec or in kubeler where CRI does not allow specification of working directory.

And I do not know about any smart way how to run sh -c "cd <dir>; git <args>" that would prevent shell from interpreting characters like `, $, ", ', ; and so on - part of args comes from user and must not be interpreted by shell and escaping these is dangerous.

@rootfs, any idea?

rootfs · 2017-10-02T13:21:34Z

/keep-open

jsafrane · 2017-10-13T22:57:05Z

/retest
(to see if the tests moved from Wheezy to GCI)

jsafrane · 2017-10-19T11:42:11Z

/test e2e-gce

jsafrane · 2017-11-02T14:51:35Z

@rootfs, it seems that the test env. was updated and we can merge this PR. I added a release note to use newer git.

rootfs · 2017-12-04T13:11:18Z

/lgtm

k8s-github-robot · 2017-12-04T13:12:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, rootfs

Associated issue requirement bypassed by: rootfs

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/volume/git_repo/OWNERS~~ [jsafrane,rootfs]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jsafrane · 2018-01-11T14:56:32Z

/test all

jsafrane · 2018-01-11T14:57:59Z

/test kubernetes-cross

jsafrane · 2018-01-11T14:58:26Z

/test pull-kubernetes-cross

k8s-github-robot · 2018-01-11T16:36:19Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-01-11T17:22:21Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue (batch tested with PRs 60476, 62462, 61391, 62535, 62394). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Revert "git: Use VolumeHost.GetExec() to execute stuff in volume plugins" This reverts commit c578542 (PR #51098). The PR added support for containerized git, on the other hand it required git 1.8.5. This breaks git volumes on older distros (CentOS 7, Ubuntu 14.04) that have old git. Git volumes are getting deprecated (#60999) so we should restore it to the last working state and not touch it any longer. **Release note**: ```release-note gitRepo volumes in pods no longer require git 1.8.5 or newer, older git versions are supported too now. ``` I'd like to cherry-pick it into 1.10. /sig storage

k8s-ci-robot assigned rootfs Aug 22, 2017

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 22, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 22, 2017

rootfs reviewed Aug 22, 2017

View reviewed changes

jsafrane force-pushed the exec-git branch from 561f351 to 9cdc4da Compare August 22, 2017 14:41

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2017

git: Use VolumeHost.GetExec() to execute stuff in volume plugins

c578542

This prepares volume plugins to run things in containers instead of running them on the host.

jsafrane force-pushed the exec-git branch from 9cdc4da to c578542 Compare August 31, 2017 11:30

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 2, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2017

k8s-github-robot merged commit 820ea04 into kubernetes:master Jan 11, 2018

jsafrane mentioned this pull request Apr 11, 2018

Revert "git: Use VolumeHost.GetExec() to execute stuff in volume plugins" #62394

Merged

git: Use VolumeHost.GetExec() to execute stuff in volume plugins #51098

git: Use VolumeHost.GetExec() to execute stuff in volume plugins #51098

Conversation

jsafrane commented Aug 22, 2017 • edited

jsafrane commented Aug 22, 2017

jsafrane commented Aug 22, 2017

rootfs Aug 22, 2017

Choose a reason for hiding this comment

jsafrane Aug 22, 2017

Choose a reason for hiding this comment

rootfs Aug 22, 2017

Choose a reason for hiding this comment

jsafrane Aug 22, 2017

Choose a reason for hiding this comment

rootfs Aug 22, 2017

Choose a reason for hiding this comment

jsafrane Aug 23, 2017

Choose a reason for hiding this comment

jsafrane commented Aug 23, 2017

jsafrane commented Aug 30, 2017

jsafrane commented Aug 30, 2017

rootfs commented Aug 30, 2017

rootfs commented Aug 30, 2017

fejta-bot commented Aug 30, 2017

fejta-bot commented Aug 31, 2017

fejta-bot commented Aug 31, 2017

jsafrane commented Aug 31, 2017

jsafrane commented Aug 31, 2017

jsafrane commented Aug 31, 2017

jsafrane commented Aug 31, 2017

jsafrane commented Aug 31, 2017

rootfs commented Aug 31, 2017

rootfs commented Aug 31, 2017

jsafrane commented Aug 31, 2017

jsafrane commented Sep 1, 2017

rootfs commented Oct 2, 2017

jsafrane commented Oct 13, 2017

jsafrane commented Oct 19, 2017

jsafrane commented Nov 2, 2017

rootfs commented Dec 4, 2017

k8s-github-robot commented Dec 4, 2017

jsafrane commented Jan 11, 2018

jsafrane commented Jan 11, 2018

jsafrane commented Jan 11, 2018

k8s-github-robot commented Jan 11, 2018

k8s-github-robot commented Jan 11, 2018

jsafrane commented Aug 22, 2017 •

edited