Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

If auto_discover_path is set to /tmp/WHATEVER/.... plugins are erased when snapd stops #954

Closed
obourdon opened this issue May 26, 2016 · 5 comments

Comments

@obourdon
Copy link
Contributor

obourdon commented May 26, 2016

To reproduce the issue:
go get github.com/intelsdi-x/snap

Please note that currently this should fail due to latest commits in https://github.com/appc/spec :-(
but once you fixed those, run:

cd $GOPATH/src/github.com/intelsdi-x/snap
make all
mkdir -p /tmp/X/Y/Z
rsync -a build/ /tmp/X/Y/Z
/tmp/X/Y/Z/bin/snapd -t 0 -l 1 -a /tmp/X/Y/Z/plugin
...
^C
DEBU[0004] stopped _block=start-plugin _module=control-runner DEBU[0004] Removing plugin _module=control-plugin-mgr plugin-name=mock plugin-path=/tmp/X/Y/Z/plugin/snap-collector-mock2 plugin-type=collector plugin-version=2 DEBU[0004] Removing plugin _module=control-plugin-mgr plugin-name=passthru plugin-path=/tmp/X/Y/Z/plugin/snap-processor-passthru plugin-type=processor plugin-version=1 DEBU[0004] Removing plugin _module=control-plugin-mgr plugin-name=file plugin-path=/tmp/X/Y/Z/plugin/snap-publisher-file plugin-type=publisher plugin-version=3 DEBU[0004] Removing plugin _module=control-plugin-mgr plugin-name=mock plugin-path=/tmp/X/Y/Z/plugin/snap-collector-mock1 plugin-type=collector plugin-version=1
ls /tmp/X/Y/Z
bin

Only plugins loaded via snapctl should be cleaned up not the one from auto discovery path

@IRCody
Copy link
Contributor

IRCody commented May 26, 2016

Slightly modified the instructions above for os x. Listed the instructions I used below. I was able to reproduce the behavior.

cd $GOPATH/src/github.com/intelsdi-x/snap
make all
mkdir -p $TMPDIR/X/Y/Z
rsync -a build $TMPDIR/X/Y/Z
$TMPDIR/X/Y/Z/build/bin/snapd -t 0 -l 1 -a $TMPDIR/X/Y/Z/build/plugin

This seems to be related to this line where we check that the plugin path is in the tmp dir by comparing it with os.TempDir() and remove it if it is. I think this might have been done to remove files loaded over rest (that are written to the tmpdir).

@obourdon
Copy link
Contributor Author

Thanks for this note Cody. This makes it even more dangerous because it also concerns paths were the $TMPDIR string is contained anywhere like for instance on Linux ~user/tmp/X/Y/Z
I will soon submit a pull request which fixes this issue

@IRCody
Copy link
Contributor

IRCody commented May 27, 2016

Thanks for finding/posting the issue @obourdon.

Looking into it a little more this morning I noticed when loading plugins from rest we actually use ioutil.TempDir() not os.TempDir(). The code is here but I copied the relevant lines below:

    // Create temporary directory
    dir, err := ioutil.TempDir("", "")
    if err != nil {
        return "", err
    }
    f, err := os.Create(path.Join(dir, filename))

ioutil.TempDir() will create a new directory (that seems to be a set of numbers) and combine it with the result of os.TempDir() so for example you end up with /var/folders/f1/_drtvyf57sq8b3gqkb9wl6l80000gp/T/048753855 Then we join this with the filename to get something like /var/folders/f1/_drtvyf57sq8b3gqkb9wl6l80000gp/T/048753855/plugin-name.

This makes it a littler harder to correctly handle the removal. The ways to fix this that come to mind immediately are:

  • Define some sort of snap-specific subdir on temp (SNAP_TEMP_DIR?) and when deciding to remove check that it starts with os.TempDir()/SNAP_TEMP_DIR before removal.
  • Tag plugins on load if they were loaded via auto-discovery so we know not to remove those.

@obourdon
Copy link
Contributor Author

Very good catch, I did most of my investigations and testing on Linux systems so I guess my fix proposal might not be exhaustive in this respect.

I also thought about implementing the second of your proposal (tagging plugins to remember how they were uploaded which seems to be the more reliable way to go) but the fix was unfortunately out of my current competencies so I focused on a quicker one

@obourdon
Copy link
Contributor Author

I have just uploaded a new version of the pull request which, I think, implements the tagging of plugins at load time. I have successfully tested it on Linux and MacOS platforms.

Feel free to make any comments

IRCody added a commit that referenced this issue Jun 2, 2016
Fix for issue #954

Adds IsAutoLoaded to pluginDetails in order to remove only non-autoloaded plugins.
IRCody pushed a commit to IRCody/snap that referenced this issue Jun 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants