Solr
  1. Solr
  2. SOLR-3268

remove write acess to source tree (chmod 555) when running tests in jenkins

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None

      Description

      Some tests are currently creating files under the source tree.

      This causes a lot of problems, it makes my checkout look dirty after running 'ant test' and i have to cleanup.

      I opened and issue for this a month in a half for solrj/src/test-files/solrj/solr/shared/test-solr.xml (SOLR-3112),
      but now we have a second file (core/src/test-files/solr/conf/elevate-data-distrib.xml).

      So I think hudson needs to chmod these src directories to 555, so that solr tests that do this will fail.

        Activity

        Robert Muir created issue -
        Hide
        Uwe Schindler added a comment -

        LOL

        Show
        Uwe Schindler added a comment - LOL
        Hide
        Dawid Weiss added a comment -

        I agree that tests modifying sources or writing to source areas are a pain. I know these files can be svn:ignored but it just... feels dirty somehow. On a constructive note – maybe we can use this:

        http://ant.apache.org/manual/Tasks/sync.html

        and mirror whatever src folder structure is required for these tests in the build area?

        Show
        Dawid Weiss added a comment - I agree that tests modifying sources or writing to source areas are a pain. I know these files can be svn:ignored but it just... feels dirty somehow. On a constructive note – maybe we can use this: http://ant.apache.org/manual/Tasks/sync.html and mirror whatever src folder structure is required for these tests in the build area?
        Hide
        Robert Muir added a comment -

        Dawid: we already do that.

        Show
        Robert Muir added a comment - Dawid: we already do that.
        Hide
        Dawid Weiss added a comment -

        Oh... in that case the tests just need to be fixed

        Show
        Dawid Weiss added a comment - Oh... in that case the tests just need to be fixed
        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        Robert Muir added a comment -

        So I was wrong about the resource-sync idea: here's a patch implementing it.

        unfortunately this is not a great solution:

        • it only solves one of the problems (for some reason SOLR-3112 is not solved by this).
        • its just syncing the test-files to build/test-files and putting that in classpath instead: but this still doesnt "fix" anything, any tests that modify things in here could interfere with other tests because its not a per-jvm sandbox like TEMP_DIR, and its definitely not per-test.

        Bottom line: I think really the best solution is that tests that want to write files to their configuration area should create a temporary directory (via our normal tempdir mechanism) and write to that...

        Show
        Robert Muir added a comment - So I was wrong about the resource-sync idea: here's a patch implementing it. unfortunately this is not a great solution: it only solves one of the problems (for some reason SOLR-3112 is not solved by this). its just syncing the test-files to build/test-files and putting that in classpath instead: but this still doesnt "fix" anything, any tests that modify things in here could interfere with other tests because its not a per-jvm sandbox like TEMP_DIR, and its definitely not per-test. Bottom line: I think really the best solution is that tests that want to write files to their configuration area should create a temporary directory (via our normal tempdir mechanism) and write to that...
        Robert Muir made changes -
        Field Original Value New Value
        Attachment SOLR-3268_sync.patch [ 12519604 ]
        Hide
        Hoss Man added a comment -

        Issue is marked 3.6 and actively being discussed but has no assignee - assigning to most active committer contributing patches/discussion so far to triage wether this can/should be pushed to 4.0 or not.

        Show
        Hoss Man added a comment - Issue is marked 3.6 and actively being discussed but has no assignee - assigning to most active committer contributing patches/discussion so far to triage wether this can/should be pushed to 4.0 or not.
        Hoss Man made changes -
        Assignee Robert Muir [ rcmuir ]
        Hide
        Robert Muir added a comment -

        I don't think figuring out how to do this needs to block 3.6: fortunately Luca fixed all
        the tests in question!

        But we should look into a safe way to do this in the future: the main thing being is that
        we want to restore the correct chmod in jenkins even if the build fails, otherwise
        the next time it goes to svn update, I think there will be problems?

        Show
        Robert Muir added a comment - I don't think figuring out how to do this needs to block 3.6: fortunately Luca fixed all the tests in question! But we should look into a safe way to do this in the future: the main thing being is that we want to restore the correct chmod in jenkins even if the build fails, otherwise the next time it goes to svn update, I think there will be problems?
        Robert Muir made changes -
        Fix Version/s 3.6 [ 12319065 ]
        Hide
        Hoss Man added a comment -

        if locking down src/ so tests can't make changes is tricky to do safely, perhaps a we could do something simpler to get us part way towards the ultimate goal? ... add a final step to the jenkins build script that fails if "svn status | wc -l" returns non-zero?

        it wont't ensure no changes are made to src/, but it should ensure no changes are made to src/ unless explicitly allowed by an svn:ignore ... then we just have to (remove any existing svn:ignore under /src and) make sure we publicly shame anyone who adds svn:ignores to src/ because they wrote a sloppy test.

        Show
        Hoss Man added a comment - if locking down src/ so tests can't make changes is tricky to do safely, perhaps a we could do something simpler to get us part way towards the ultimate goal? ... add a final step to the jenkins build script that fails if "svn status | wc -l" returns non-zero? it wont't ensure no changes are made to src/, but it should ensure no changes are made to src/ unless explicitly allowed by an svn:ignore ... then we just have to (remove any existing svn:ignore under /src and) make sure we publicly shame anyone who adds svn:ignores to src/ because they wrote a sloppy test.
        Hide
        Robert Muir added a comment -

        add a final step to the jenkins build script that fails if "svn status | wc -l" returns non-zero?

        duh... what a simple solution. +1

        Show
        Robert Muir added a comment - add a final step to the jenkins build script that fails if "svn status | wc -l" returns non-zero? duh... what a simple solution. +1
        Hide
        Steve Rowe added a comment -

        add a final step to the jenkins build script that fails if "svn status | wc -l" returns non-zero?

        it wont't ensure no changes are made to src/, but it should ensure no changes are made to src/ unless explicitly allowed by an svn:ignore

        I don't remember which one(s), but I recall that at least one file is (was?) produced by a Solr test and svn:ignore'd. In order for Hoss's suggestion to be fully effective, we should stop svn:ignore'ing test outputs (outside of build/ or other already ignored directories). I'll see what's being svn:ignore'd now.

        Show
        Steve Rowe added a comment - add a final step to the jenkins build script that fails if "svn status | wc -l" returns non-zero? it wont't ensure no changes are made to src/, but it should ensure no changes are made to src/ unless explicitly allowed by an svn:ignore I don't remember which one(s), but I recall that at least one file is (was?) produced by a Solr test and svn:ignore'd. In order for Hoss's suggestion to be fully effective, we should stop svn:ignore'ing test outputs (outside of build/ or other already ignored directories). I'll see what's being svn:ignore'd now.
        Hide
        Hoss Man added a comment -

        i think you're talking about SOLR-3112 which was dealt with .. but even if there are others, we can start by adding this check now, and then file issues to fix & remove whatever is left.

        this isn't a silver bullet, it's certainly not as good as actually looking down src to fail on writes, but it will at least force people to be aware if/when they add a test that pollutes src/

        Show
        Hoss Man added a comment - i think you're talking about SOLR-3112 which was dealt with .. but even if there are others, we can start by adding this check now, and then file issues to fix & remove whatever is left. this isn't a silver bullet, it's certainly not as good as actually looking down src to fail on writes, but it will at least force people to be aware if/when they add a test that pollutes src/
        Hide
        Steve Rowe added a comment -

        i think you're talking about SOLR-3112

        I don't think so - I think I was remembering something under DIH test-files/.

        Here are the non-standard svn:ignore's I found under branch_3x/solr/ - are any of these problematic?:

        directory svn:ignore
        contrib/dataimporthandler/src/test-files dataimport.properties
        contrib/dataimporthandler/src/test-files/dih/solr/conf dataimport.properties
        contrib/dataimporthandler-extras/src/test-files/dihextras/solr/conf dataimport.properties
        core/src/test-files/solr data
        core/src/test-files/solr data
        example/example-DIH/solr/db data
        example/example-DIH/solr/mail data
        example/example-DIH/solr/db data
        example/example-DIH/solr/mail data
        example/example-DIH/solr/mail/lib *.jar
        example/example-DIH/solr/rss data
        example/example-DIH/solr/rss/conf dataimport.properties
        example/example-DIH/solr/tika data
        example/exampledocs post.jar
        example/multicore/core0 data
        example/multicore/core1 data
        example/solr data, lib, logs
        Show
        Steve Rowe added a comment - i think you're talking about SOLR-3112 I don't think so - I think I was remembering something under DIH test-files/ . Here are the non-standard svn:ignore 's I found under branch_3x/solr/ - are any of these problematic?: directory svn:ignore contrib/dataimporthandler/src/test-files dataimport.properties contrib/dataimporthandler/src/test-files/dih/solr/conf dataimport.properties contrib/dataimporthandler-extras/src/test-files/dihextras/solr/conf dataimport.properties core/src/test-files/solr data core/src/test-files/solr data example/example-DIH/solr/db data example/example-DIH/solr/mail data example/example-DIH/solr/db data example/example-DIH/solr/mail data example/example-DIH/solr/mail/lib *.jar example/example-DIH/solr/rss data example/example-DIH/solr/rss/conf dataimport.properties example/example-DIH/solr/tika data example/exampledocs post.jar example/multicore/core0 data example/multicore/core1 data example/solr data, lib, logs
        Hide
        Robert Muir added a comment -

        The ones under src/test-files is definitely problematic.
        What happens is that tests put indexes in there. And later tests go
        and 'update' those indexes. So if you make any change to the index format,
        two things can happen (both of which have happened to me):

        1. you see strange exceptions and think you broke something, digging into
          tests only to find out its this src/test-files stuff.
        2. after committing, other developers on the mailing list get confused
          and think something is broken for the same reason.

        As a temporary hack we have this in 'ant clean':

        clean:
        [echo] TODO: fix tests to not write files to 'core/src/test-files/solr/data'!

        But thats not a real solution.

        The svn:ignore just shoves the problem under the rug.

        Show
        Robert Muir added a comment - The ones under src/test-files is definitely problematic. What happens is that tests put indexes in there. And later tests go and 'update' those indexes. So if you make any change to the index format, two things can happen (both of which have happened to me): you see strange exceptions and think you broke something, digging into tests only to find out its this src/test-files stuff. after committing, other developers on the mailing list get confused and think something is broken for the same reason. As a temporary hack we have this in 'ant clean': clean: [echo] TODO: fix tests to not write files to 'core/src/test-files/solr/data'! But thats not a real solution. The svn:ignore just shoves the problem under the rug.
        Hide
        Robert Muir added a comment -

        its also possible the patch on this issue might solve that problem for individual runs
        (its still not as good as a test creating its own tempdir, guaranteeing it wont interfere with
        other tests in the same run though).

        Show
        Robert Muir added a comment - its also possible the patch on this issue might solve that problem for individual runs (its still not as good as a test creating its own tempdir, guaranteeing it wont interfere with other tests in the same run though).
        Hide
        Steve Rowe added a comment -

        Here's the list for trunk/solr/:

        directory svn:ignore
        contrib/dataimporthandler/src/test-files dataimport.properties
        contrib/dataimporthandler/src/test-files/dih/solr/conf dataimport.properties
        contrib/dataimporthandler-extras/src/test-files/dihextras/solr/conf dataimport.properties
        core/src/test-files/solr data
        example/example-DIH/solr/db data
        example/example-DIH/solr/db/conf dataimport.properties
        example/example-DIH/solr/mail data
        example/example-DIH/solr/mail/lib *.jar
        example/example-DIH/solr/rss data
        example/example-DIH/solr/rss/conf dataimport.properties
        example/example-DIH/solr/tika data
        example/exampledocs post.jar
        example/multicore/core0 data
        example/multicore/core1 data
        example/solr data, lib, logs, zoo_data
        Show
        Steve Rowe added a comment - Here's the list for trunk/solr/ : directory svn:ignore contrib/dataimporthandler/src/test-files dataimport.properties contrib/dataimporthandler/src/test-files/dih/solr/conf dataimport.properties contrib/dataimporthandler-extras/src/test-files/dihextras/solr/conf dataimport.properties core/src/test-files/solr data example/example-DIH/solr/db data example/example-DIH/solr/db/conf dataimport.properties example/example-DIH/solr/mail data example/example-DIH/solr/mail/lib *.jar example/example-DIH/solr/rss data example/example-DIH/solr/rss/conf dataimport.properties example/example-DIH/solr/tika data example/exampledocs post.jar example/multicore/core0 data example/multicore/core1 data example/solr data, lib, logs, zoo_data
        Hide
        Dawid Weiss added a comment -

        All of these are in .gitignore, Steven (and can be regenerated via dev-tools/scripts/gitignore-gen.sh.

        Show
        Dawid Weiss added a comment - All of these are in .gitignore, Steven (and can be regenerated via dev-tools/scripts/gitignore-gen.sh.
        Hide
        Robert Muir added a comment -

        I'm first testing if core/src/test-files/solr is still needed. Luca fixed a couple tests in SOLR-3112.
        Maybe its obselete and can be removed from svn:ignore, along with the ant clean hack.

        Show
        Robert Muir added a comment - I'm first testing if core/src/test-files/solr is still needed. Luca fixed a couple tests in SOLR-3112 . Maybe its obselete and can be removed from svn:ignore, along with the ant clean hack.
        Hide
        Robert Muir added a comment -

        Hmm definitely this still gets created (I un-svn-ignored it).

        Then i chmod 555'd core/src/test-file/solr just to hopefully make the offendor fail,
        but with this in place, instead the test hangs infinitely (looks like NoCacheHeaderTest)

        Show
        Robert Muir added a comment - Hmm definitely this still gets created (I un-svn-ignored it). Then i chmod 555'd core/src/test-file/solr just to hopefully make the offendor fail, but with this in place, instead the test hangs infinitely (looks like NoCacheHeaderTest)
        Hide
        Steve Rowe added a comment -

        All of these are in .gitignore, Steven (and can be regenerated via dev-tools/scripts/gitignore-gen.sh.

        Right, I'd forgotten about that (I'm not a git user).

        I rolled my own dirty one-liner script (newlines/indents added here for clarity):

        (for a in $(find . -type d | grep -v '\.svn\|/build') ; do
            b=`svn propget svn:ignore $a`
            if [[ -n $b ]]; then
                c=${b//$'\n'/, }
                echo "|$a|$c|"
            fi
        done) 2>/dev/null
        
        Show
        Steve Rowe added a comment - All of these are in .gitignore, Steven (and can be regenerated via dev-tools/scripts/gitignore-gen.sh. Right, I'd forgotten about that (I'm not a git user). I rolled my own dirty one-liner script (newlines/indents added here for clarity): (for a in $(find . -type d | grep -v '\.svn\|/build') ; do b=`svn propget svn:ignore $a` if [[ -n $b ]]; then c=${b//$'\n'/, } echo "|$a|$c|" fi done) 2>/dev/null
        Hide
        Robert Muir added a comment -

        If you apply the patch, the test won't create indexes under this directory any more.

        This is an improvement even though its not perfect. I'm gonna commit it.

        Show
        Robert Muir added a comment - If you apply the patch, the test won't create indexes under this directory any more. This is an improvement even though its not perfect. I'm gonna commit it.
        rmuir committed 1305585 (1 file)
        Reviews: none

        SOLR-3268: use sync for more safety

        rmuir committed 1305587 (3 files)
        Reviews: none

        SOLR-3268: use sync for more safety

        Hide
        Robert Muir added a comment -

        OK: i think the core/src/test-files/solr/data svn:ignore is nuked in trunk and branch_3x.

        As i said before its still not perfect, and I put comments in NoCacheHeaderTest indicating its broken,
        because if two different tests try to write to this sync'ed build/test-files directory at the same time,
        then it will cause strange fails.

        But its an improvement in the sense that we don't need the special ant clean, we don't dirty up the
        source tree, and sync task (called by compile-test) should nuke any leftover indexes created from
        the previous run.

        Show
        Robert Muir added a comment - OK: i think the core/src/test-files/solr/data svn:ignore is nuked in trunk and branch_3x. As i said before its still not perfect, and I put comments in NoCacheHeaderTest indicating its broken, because if two different tests try to write to this sync'ed build/test-files directory at the same time, then it will cause strange fails. But its an improvement in the sense that we don't need the special ant clean, we don't dirty up the source tree, and sync task (called by compile-test) should nuke any leftover indexes created from the previous run.
        Hide
        Robert Muir added a comment -

        OK the previous commit may or may not have also fixed the dataimporthandler problems.

        I need a beer run before touching DIH but ill investigate these next.

        Show
        Robert Muir added a comment - OK the previous commit may or may not have also fixed the dataimporthandler problems. I need a beer run before touching DIH but ill investigate these next.
        Hide
        Robert Muir added a comment -

        OK all those non-example/ svn:ignore's are gone.

        I think we can now add the 'svn status' check to jenkins nightly.

        Show
        Robert Muir added a comment - OK all those non-example/ svn:ignore's are gone. I think we can now add the 'svn status' check to jenkins nightly.
        Hide
        Robert Muir added a comment -

        I committed the svn status checker to nightly.

        Show
        Robert Muir added a comment - I committed the svn status checker to nightly.
        Robert Muir made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 3.6 [ 12319065 ]
        Resolution Fixed [ 1 ]
        rmuir committed 1308449 (6 files)
        Uwe Schindler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Robert Muir
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development