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

Remove mkdir redundancy #444

Merged
merged 4 commits into from Dec 30, 2019
Merged

Conversation

@chriskim06
Copy link
Contributor

chriskim06 commented Dec 27, 2019

The call to os.Mkdir was creating a directory at $KREW_ROOT/store/grep/v1.2.2 (for example if installing grep). Then when renameOrCopy gets called, os.RemoveAll deletes the /v1.2.2 part, leaving just $KREW_ROOT/store/grep. This is what results in the logging mentioned in the issue:

I1227 11:03:20.733616 4969 move.go:192] There's already a directory at move target "/home/kimc/playground/store/grep/v1.2.2". deleting.
I1227 11:03:20.733681 4969 move.go:196] Move target directory "/home/kimc/playground/store/grep/v1.2.2" cleaned up

We can't remove just the mkdir part though, because without $KREW_ROOT/store/grep the call to os.Rename will return an error:

W1227 11:06:26.208294 6026 install.go:131] failed to install plugin "grep": install failed: failed while moving files to the installation directory: could not rename/copy directory "/tmp/krew-temp-move705873968" to "/home/kimc/playground/store/grep/v1.2.2": rename /tmp/krew-temp-move705873968 /home/kimc/playground/store/grep/v1.2.2: no such file or directory

This change adds the plugin directory to installOperation so that moveToInstallDir can create that directory so that its available to move to from the temp move directory.

Here is the output with the change:

$ KREW_ROOT="/home/kimc/playground" /home/kimc/go/src/sigs.k8s.io/krew/out/bin/krew-linux_amd64 install grep -v=5
I1227 11:23:51.908203   10284 update.go:45] Updating the local copy of plugin index (/home/kimc/playground/index)
I1227 11:23:51.908271   10284 git.go:73] Going to run git fetch -v
From https://github.com/kubernetes-sigs/krew-index
 = [up to date]      master     -> origin/master
 = [up to date]      v0.2.1     -> origin/v0.2.1
I1227 11:23:51.969041   10284 git.go:73] Going to run git reset --hard @{upstream}
HEAD is now at a5bc422 Bump open-svc to v2.3.0 (#403)
I1227 11:23:51.972448   10284 git.go:73] Going to run git clean -xfd
Updated the local copy of plugin index.
I1227 11:23:51.974510   10284 scanner.go:82] Reading plugin "grep"
I1227 11:23:51.975443   10284 install.go:116] Will install plugin: grep
Installing plugin: grep
I1227 11:23:51.975470   10284 install.go:60] Looking for installed versions
I1227 11:23:51.975498   10284 platform.go:43] Matching platform for labels(arch=amd64,os=linux)
I1227 11:23:51.975537   10284 platform.go:51] Found matching platform with index (1)
I1227 11:23:51.975554   10284 install.go:79] Install plugin grep at version=v1.2.2
I1227 11:23:51.975573   10284 install.go:98] Creating download staging directory "/tmp/krew-downloads/grep"
I1227 11:23:51.975646   10284 fetch.go:39] Fetching "https://github.com/guessi/kubectl-grep/releases/download/v1.2.2/kubectl-grep-Linux-x86_64.tar.gz"
I1227 11:23:52.254230   10284 downloader.go:42] Reading archive file into memory
I1227 11:23:52.358874   10284 downloader.go:47] Read 8009053 bytes from archive into memory
I1227 11:23:52.358915   10284 verifier.go:51] Compare sha256 (cdf8fd13e9fcd502199b0abccfdc539ef39895648670c9f281c023e7b7986228) signed version
I1227 11:23:52.358944   10284 downloader.go:203] detected "application/x-gzip" file type
I1227 11:23:52.358964   10284 downloader.go:100] tar: extracting to "/tmp/krew-downloads/grep"
I1227 11:23:52.359646   10284 downloader.go:118] tar: processing "kubectl-grep" (type=48, mode=-rwxr-xr-x)
I1227 11:23:52.359678   10284 downloader.go:137] tar: ensuring parent dirs exist for regular file, dir=/tmp/krew-downloads/grep
I1227 11:23:52.619464   10284 downloader.go:154] tar: processed "kubectl-grep"
I1227 11:23:52.619516   10284 downloader.go:118] tar: processing "LICENSE" (type=48, mode=-rw-r--r--)
I1227 11:23:52.619540   10284 downloader.go:137] tar: ensuring parent dirs exist for regular file, dir=/tmp/krew-downloads/grep
I1227 11:23:52.619666   10284 downloader.go:154] tar: processed "LICENSE"
I1227 11:23:52.619686   10284 downloader.go:156] tar extraction to /tmp/krew-downloads/grep complete
I1227 11:23:52.619701   10284 move.go:156] Creating plugin directory "/home/kimc/playground/store/grep"
I1227 11:23:52.619866   10284 move.go:162] Creating temp plugin move operations dir "/tmp/krew-temp-move162422170"
I1227 11:23:52.619880   10284 move.go:125] Finding move targets from "/tmp/krew-downloads/grep" to "/tmp/krew-temp-move162422170" with file operation=index.FileOperation{From:"kubectl-grep", To:"."}
I1227 11:23:52.619918   10284 move.go:44] Trying to move single file directly from="/tmp/krew-downloads/grep" to="/tmp/krew-temp-move162422170" with file operation=index.FileOperation{From:"kubectl-grep", To:"."}
I1227 11:23:52.619942   10284 move.go:48] Detected single move from file operation=index.FileOperation{From:"kubectl-grep", To:"."}
I1227 11:23:52.619954   10284 move.go:132] Move file from "/tmp/krew-downloads/grep/kubectl-grep" to "/tmp/krew-temp-move162422170/kubectl-grep"
I1227 11:23:52.619999   10284 move.go:141] Move operations are complete
I1227 11:23:52.620009   10284 move.go:125] Finding move targets from "/tmp/krew-downloads/grep" to "/tmp/krew-temp-move162422170" with file operation=index.FileOperation{From:"LICENSE", To:"."}
I1227 11:23:52.620023   10284 move.go:44] Trying to move single file directly from="/tmp/krew-downloads/grep" to="/tmp/krew-temp-move162422170" with file operation=index.FileOperation{From:"LICENSE", To:"."}
I1227 11:23:52.620042   10284 move.go:48] Detected single move from file operation=index.FileOperation{From:"LICENSE", To:"."}
I1227 11:23:52.620053   10284 move.go:132] Move file from "/tmp/krew-downloads/grep/LICENSE" to "/tmp/krew-temp-move162422170/LICENSE"
I1227 11:23:52.620088   10284 move.go:141] Move operations are complete
I1227 11:23:52.620099   10284 move.go:172] Move directory "/tmp/krew-temp-move162422170" to "/home/kimc/playground/store/grep/v1.2.2"
I1227 11:23:52.620158   10284 install.go:215] No file found at "/home/kimc/playground/bin/kubectl-grep"
I1227 11:23:52.620175   10284 install.go:202] Creating symlink to "/home/kimc/playground/store/grep/v1.2.2/kubectl-grep" at "/home/kimc/playground/bin/kubectl-grep"
I1227 11:23:52.620211   10284 install.go:206] Created symlink at "/home/kimc/playground/bin/kubectl-grep"
I1227 11:23:52.620223   10284 install.go:103] Deleting the download staging directory /tmp/krew-downloads/grep
I1227 11:23:52.620282   10284 install.go:91] Storing install receipt for plugin grep
Installed plugin: grep
\
 | Use this plugin:
 |      kubectl grep
 | Documentation:
 |      https://github.com/guessi/kubectl-grep
 | Caveats:
 | \
 |  | This plugin requires an existing KUBECONFIG file, with a `current-context` field set.
 |  |
 |  | Usage:
 |  |
 |  |   $ kubectl grep # output help messages
 |  |
 |  | More Info:
 |  | - https://github.com/guessi/kubectl-grep
 | /
/
WARNING: You installed a plugin from the krew-index plugin repository.
   These plugins are not audited for security by the Krew maintainers.
   Run them at your own risk.

Fixes #383

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 27, 2019

Codecov Report

Merging #444 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
- Coverage   56.54%   56.47%   -0.07%     
==========================================
  Files          19       19              
  Lines         925      926       +1     
==========================================
  Hits          523      523              
- Misses        349      350       +1     
  Partials       53       53
Impacted Files Coverage Δ
internal/installation/move.go 32.85% <0%> (-0.24%) ⬇️

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 885f5ad...748994b. Read the comment docs.

if err := os.MkdirAll(installDir, 0755); err != nil {
return errors.Wrapf(err, "error creating installation directory at %q", installDir)
func moveToInstallDir(srcDir, pluginDir, installDir string, fos []index.FileOperation) error {
klog.V(4).Infof("Creating plugin directory %q", pluginDir)

This comment has been minimized.

Copy link
@ahmetb

ahmetb Dec 27, 2019

Member

I suspect this is not the right solution.
We don't really need the knowledge of PluginInstallPath in this method. We just need to know installDir.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Dec 27, 2019

Member

Can you try removing this os.MkdirAll here? I'm curious what'll happen. It might work.

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Dec 27, 2019

Author Contributor

That's sort of how I felt as well, I was going back and forth between this and something like removing the version part from the installDir string that gets passed to mkdir.

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Dec 27, 2019

Author Contributor

I did remove it, and posted the output in the description of the PR. It has this error:

W1227 11:06:26.208294 6026 install.go:131] failed to install plugin "grep": install failed: failed while moving files to the installation directory: could not rename/copy directory "/tmp/krew-temp-move705873968" to "/home/kimc/playground/store/grep/v1.2.2": rename /tmp/krew-temp-move705873968 /home/kimc/playground/store/grep/v1.2.2: no such file or directory

Its trying to move a directory to .../store/grep/v1.2.2 but .../store/grep doesn't exist without the call to mkdir, resulting in "no such file or directory".

This comment has been minimized.

Copy link
@ahmetb

ahmetb Dec 27, 2019

Member

I think we actually need to do this:

os.MkdirAll(filepath.Dir(installDir))

we are essentially trying to make sure installDir can be mkdir'ed further down in the call stack.

I think this would work, we don't need to pass pluginDir knowledge here (despite they're the same value, at least today).

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Dec 27, 2019

Author Contributor

Ah that's a good idea, I didn't know about that method. I'll make the change.

@k8s-ci-robot k8s-ci-robot added size/XS and removed size/S labels Dec 27, 2019
if err := os.MkdirAll(installDir, 0755); err != nil {
return errors.Wrapf(err, "error creating installation directory at %q", installDir)
installationDir := filepath.Dir(installDir)
klog.V(4).Infof("Creating installation directory %q", installationDir)

This comment has been minimized.

Copy link
@ahmetb

ahmetb Dec 30, 2019

Member

let's change "installation directory" to "directory" in logs.
The term installation directory is pretty overloaded already. :)

Also does this patch address redundant mkdir-ing, according to logs?

This comment has been minimized.

Copy link
@chriskim06

chriskim06 Dec 30, 2019

Author Contributor

Ok I can make the change, I was also thinking that and wasn't really sure what to call the variable as well. I figured it would be ok as installationDir since its only being used here (was also thinking maybe pluginDir would work, but wasn't sure if that has other meaning or if that's the right term for that directory).

Yes, the redundant mkdir logging is gone now, here are the logs without the "There's already a directory at move target "/home/kimc/playground/store/grep/v1.2.2". deleting...":

$ KREW_ROOT="/home/kimc/playground" /home/kimc/go/src/sigs.k8s.io/krew/out/bin/krew-linux_amd64 install grep -v=5
I1230 09:04:33.052997   22634 update.go:45] Updating the local copy of plugin index (/home/kimc/playground/index)
I1230 09:04:33.053065   22634 git.go:73] Going to run git fetch -v
POST git-upload-pack (984 bytes)
From https://github.com/kubernetes-sigs/krew-index
   a5bc422..5c1c1cb  master     -> origin/master
 = [up to date]      v0.2.1     -> origin/v0.2.1
I1230 09:04:33.214188   22634 git.go:73] Going to run git reset --hard @{upstream}
HEAD is now at 5c1c1cb Update release info (#405)
I1230 09:04:33.218980   22634 git.go:73] Going to run git clean -xfd
Updated the local copy of plugin index.
I1230 09:04:33.221132   22634 scanner.go:82] Reading plugin "grep"
I1230 09:04:33.222053   22634 install.go:116] Will install plugin: grep
Installing plugin: grep
I1230 09:04:33.222084   22634 install.go:59] Looking for installed versions
I1230 09:04:33.222116   22634 platform.go:43] Matching platform for labels(arch=amd64,os=linux)
I1230 09:04:33.222163   22634 platform.go:51] Found matching platform with index (1)
I1230 09:04:33.222182   22634 install.go:78] Install plugin grep at version=v1.2.2
I1230 09:04:33.222200   22634 install.go:96] Creating download staging directory "/tmp/krew-downloads/grep"
I1230 09:04:33.222276   22634 fetch.go:39] Fetching "https://github.com/guessi/kubectl-grep/releases/download/v1.2.2/kubectl-grep-Linux-x86_64.tar.gz"
I1230 09:04:33.522821   22634 downloader.go:42] Reading archive file into memory
I1230 09:04:33.627228   22634 downloader.go:47] Read 8009053 bytes from archive into memory
I1230 09:04:33.627268   22634 verifier.go:51] Compare sha256 (cdf8fd13e9fcd502199b0abccfdc539ef39895648670c9f281c023e7b7986228) signed version
I1230 09:04:33.627490   22634 downloader.go:203] detected "application/x-gzip" file type
I1230 09:04:33.627516   22634 downloader.go:100] tar: extracting to "/tmp/krew-downloads/grep"
I1230 09:04:33.628201   22634 downloader.go:118] tar: processing "kubectl-grep" (type=48, mode=-rwxr-xr-x)
I1230 09:04:33.628234   22634 downloader.go:137] tar: ensuring parent dirs exist for regular file, dir=/tmp/krew-downloads/grep
I1230 09:04:33.889731   22634 downloader.go:154] tar: processed "kubectl-grep"
I1230 09:04:33.889782   22634 downloader.go:118] tar: processing "LICENSE" (type=48, mode=-rw-r--r--)
I1230 09:04:33.889814   22634 downloader.go:137] tar: ensuring parent dirs exist for regular file, dir=/tmp/krew-downloads/grep
I1230 09:04:33.889943   22634 downloader.go:154] tar: processed "LICENSE"
I1230 09:04:33.889974   22634 downloader.go:156] tar extraction to /tmp/krew-downloads/grep complete
I1230 09:04:33.889994   22634 move.go:157] Creating installation directory "/home/kimc/playground/store/grep"
I1230 09:04:33.890132   22634 move.go:163] Creating temp plugin move operations dir "/tmp/krew-temp-move805780475"
I1230 09:04:33.890154   22634 move.go:125] Finding move targets from "/tmp/krew-downloads/grep" to "/tmp/krew-temp-move805780475" with file operation=index.FileOperation{From:"kubectl-grep", To:"."}
I1230 09:04:33.890201   22634 move.go:44] Trying to move single file directly from="/tmp/krew-downloads/grep" to="/tmp/krew-temp-move805780475" with file operation=index.FileOperation{From:"kubectl-grep", To:"."}
I1230 09:04:33.890234   22634 move.go:48] Detected single move from file operation=index.FileOperation{From:"kubectl-grep", To:"."}
I1230 09:04:33.890256   22634 move.go:132] Move file from "/tmp/krew-downloads/grep/kubectl-grep" to "/tmp/krew-temp-move805780475/kubectl-grep"
I1230 09:04:33.890314   22634 move.go:141] Move operations are complete
I1230 09:04:33.890336   22634 move.go:125] Finding move targets from "/tmp/krew-downloads/grep" to "/tmp/krew-temp-move805780475" with file operation=index.FileOperation{From:"LICENSE", To:"."}
I1230 09:04:33.890357   22634 move.go:44] Trying to move single file directly from="/tmp/krew-downloads/grep" to="/tmp/krew-temp-move805780475" with file operation=index.FileOperation{From:"LICENSE", To:"."}
I1230 09:04:33.890385   22634 move.go:48] Detected single move from file operation=index.FileOperation{From:"LICENSE", To:"."}
I1230 09:04:33.890403   22634 move.go:132] Move file from "/tmp/krew-downloads/grep/LICENSE" to "/tmp/krew-temp-move805780475/LICENSE"
I1230 09:04:33.890447   22634 move.go:141] Move operations are complete
I1230 09:04:33.890466   22634 move.go:173] Move directory "/tmp/krew-temp-move805780475" to "/home/kimc/playground/store/grep/v1.2.2"
I1230 09:04:33.890529   22634 install.go:213] No file found at "/home/kimc/playground/bin/kubectl-grep"
I1230 09:04:33.890557   22634 install.go:200] Creating symlink to "/home/kimc/playground/store/grep/v1.2.2/kubectl-grep" at "/home/kimc/playground/bin/kubectl-grep"
I1230 09:04:33.890602   22634 install.go:204] Created symlink at "/home/kimc/playground/bin/kubectl-grep"
I1230 09:04:33.890623   22634 install.go:101] Deleting the download staging directory /tmp/krew-downloads/grep
I1230 09:04:33.890698   22634 install.go:89] Storing install receipt for plugin grep
Installed plugin: grep
\
 | Use this plugin:
 |      kubectl grep
 | Documentation:
 |      https://github.com/guessi/kubectl-grep
 | Caveats:
 | \
 |  | This plugin requires an existing KUBECONFIG file, with a `current-context` field set.
 |  |
 |  | Usage:
 |  |
 |  |   $ kubectl grep # output help messages
 |  |
 |  | More Info:
 |  | - https://github.com/guessi/kubectl-grep
 | /
/
WARNING: You installed a plugin from the krew-index plugin repository.
   These plugins are not audited for security by the Krew maintainers.
   Run them at your own risk.
chriskim06 added 4 commits Dec 23, 2019
Update some of the logging around installation
@chriskim06 chriskim06 force-pushed the chriskim06:mkdir-redundancy branch from bfc7aec to 748994b Dec 30, 2019
@ahmetb

This comment has been minimized.

Copy link
Member

ahmetb commented Dec 30, 2019

Thanks.
/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link

k8s-ci-robot commented Dec 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, chriskim06

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit e0a3f6f into kubernetes-sigs:master Dec 30, 2019
2 of 3 checks passed
2 of 3 checks passed
tide Not mergeable. Needs approved, lgtm labels.
Details
cla/linuxfoundation chriskim06 authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.