Repository: accumulo Updated Branches: refs/heads/1.6.0-SNAPSHOT edbf231ca -> 4df3959b5
ACCUMULO-2520 made GC validate data read from !METADATA table Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/ca3f7789 Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/ca3f7789 Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/ca3f7789 Branch: refs/heads/1.6.0-SNAPSHOT Commit: ca3f77894466caddf71c89d78a98f9fee8fc7fd8 Parents: 4fdefd7 Author: Keith Turner <ktur...@apache.org> Authored: Fri Mar 21 14:58:05 2014 -0400 Committer: Keith Turner <ktur...@apache.org> Committed: Mon Mar 24 10:54:00 2014 -0400 ---------------------------------------------------------------------- .../server/gc/SimpleGarbageCollector.java | 48 ++++++++++++++++ .../server/test/GCLotsOfCandidatesTest.java | 2 +- .../server/gc/SimpleGarbageCollectorTest.java | 58 ++++++++++++++++++++ .../accumulo/server/gc/TestConfirmDeletes.java | 35 ++++++++++++ test/system/auto/simple/gc.py | 9 +++ 5 files changed, 151 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java ---------------------------------------------------------------------- diff --git a/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java b/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java index 22c3c0e..d09c557 100644 --- a/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java +++ b/src/server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java @@ -31,6 +31,7 @@ import java.util.TreeSet; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; import org.apache.accumulo.cloudtrace.instrument.CountSampler; import org.apache.accumulo.cloudtrace.instrument.Sampler; @@ -108,6 +109,9 @@ public class SimpleGarbageCollector implements Iface { private static final Logger log = Logger.getLogger(SimpleGarbageCollector.class); + private static final Pattern dirPattern = Pattern.compile("/[\\p{Punct}\\p{Alnum}&&[^/]]+/[\\p{Punct}\\p{Alnum}&&[^/]]+"); + private static final Pattern filePattern = Pattern.compile("/[\\p{Punct}\\p{Alnum}&&[^/]]+/[\\p{Punct}\\p{Alnum}&&[^/]]+/[\\p{Punct}\\p{Alnum}&&[^/]]+"); + private Instance instance; private AuthInfo credentials; private long gcStartDelay; @@ -419,6 +423,43 @@ public class SimpleGarbageCollector implements Iface { return new InetSocketAddress(Accumulo.getLocalAddress(new String[] {"--address", address}), port); } + // visible for testing + static boolean isValidFileDelete(String path) { + return filePattern.matcher(path).matches(); + } + + // visible for testing + static boolean isValidDirDelete(String path) { + return dirPattern.matcher(path).matches(); + } + + // visible for testing + static boolean isValidCandidate(String candidate) { + return isValidDirDelete(candidate) || isValidFileDelete(candidate); + } + + private void validateDir(String path) { + if (!isValidDirDelete(path)) { + throw new IllegalArgumentException("Invalid dir " + path); + } + } + + private void validateFile(String delete) { + if (!isValidFileDelete(delete)) { + throw new IllegalArgumentException("Invalid file " + delete); + } + } + + private void removeInvalidCandidates(SortedSet<String> candidates) { + for (Iterator<String> iterator = candidates.iterator(); iterator.hasNext();) { + String candidate = (String) iterator.next(); + if (!isValidCandidate(candidate)) { + log.error("Ingoring invalid deletion candidate " + candidate); + iterator.remove(); + } + } + } + /** * This method gets a set of candidates for deletion by scanning the METADATA table deleted flag keyspace */ @@ -441,6 +482,8 @@ public class SimpleGarbageCollector implements Iface { log.error("Unable to check the filesystem for offline candidates. Removing all candidates for deletion to be safe.", e); candidates.clear(); } + + removeInvalidCandidates(candidates); return candidates; } @@ -469,6 +512,7 @@ public class SimpleGarbageCollector implements Iface { } } + removeInvalidCandidates(candidates); return candidates; } @@ -513,6 +557,7 @@ public class SimpleGarbageCollector implements Iface { for (Entry<Key,Value> entry : scanner) { String blipPath = entry.getKey().getRow().toString().substring(Constants.METADATA_BLIP_FLAG_PREFIX.length()); + validateDir(blipPath); Iterator<String> tailIter = candidates.tailSet(blipPath).iterator(); int count = 0; while (tailIter.hasNext()) { @@ -556,15 +601,18 @@ public class SimpleGarbageCollector implements Iface { } // WARNING: This line is EXTREMELY IMPORTANT. // You MUST REMOVE candidates that are still in use + validateFile(delete); if (candidates.remove(delete)) log.debug("Candidate was still in use in the METADATA table: " + delete); String path = delete.substring(0, delete.lastIndexOf('/')); + validateDir(path); if (candidates.remove(path)) log.debug("Candidate was still in use in the METADATA table: " + path); } else if (Constants.METADATA_DIRECTORY_COLUMN.hasColumns(entry.getKey())) { String table = new String(KeyExtent.tableOfMetadataRow(entry.getKey().getRow())); String delete = "/" + table + entry.getValue().toString(); + validateDir(delete); if (candidates.remove(delete)) log.debug("Candidate was still in use in the METADATA table: " + delete); } else http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java ---------------------------------------------------------------------- diff --git a/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java b/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java index 722fd1a..c984a94 100644 --- a/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java +++ b/src/server/src/main/java/org/apache/accumulo/server/test/GCLotsOfCandidatesTest.java @@ -45,7 +45,7 @@ public class GCLotsOfCandidatesTest { for (int i = 0; i < 10000; ++i) { final Text emptyText = new Text(""); - Text row = new Text(String.format("%s%s%020d%s", Constants.METADATA_DELETE_FLAG_PREFIX, "/", i, + Text row = new Text(String.format("%s/%020d/%s", Constants.METADATA_DELETE_FLAG_PREFIX, i, "aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeeeffffffffffgggggggggghhhhhhhhhhiiiiiiiiiijjjjjjjjjj")); Mutation delFlag = new Mutation(row); delFlag.put(emptyText, emptyText, new Value(new byte[] {})); http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java ---------------------------------------------------------------------- diff --git a/src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java b/src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java new file mode 100644 index 0000000..c89aad3 --- /dev/null +++ b/src/server/src/test/java/org/apache/accumulo/server/gc/SimpleGarbageCollectorTest.java @@ -0,0 +1,58 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.accumulo.server.gc; + +import org.junit.Assert; +import org.junit.Test; + +public class SimpleGarbageCollectorTest { + @Test + public void testValidation() { + + Assert.assertTrue(SimpleGarbageCollector.isValidDirDelete("/a/b")); + Assert.assertTrue(SimpleGarbageCollector.isValidDirDelete("/45/t-00004")); + Assert.assertTrue(SimpleGarbageCollector.isValidDirDelete("/45/b-00006")); + + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/a")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("//a")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("//")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("////")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/45//t-00004")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("45///b-00006")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/45/b-00006/C00000.rf")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/ /b")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("/a/b/c")); + Assert.assertFalse(SimpleGarbageCollector.isValidDirDelete("zz/a/b")); + + Assert.assertTrue(SimpleGarbageCollector.isValidFileDelete("/45/t-000c4/C00000.rf")); + Assert.assertTrue(SimpleGarbageCollector.isValidFileDelete("/a7/b-00006/F0000a.rf")); + + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("")); + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("/")); + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("a7/b-00006/F0000a.rf")); + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("//a7/b-00006/F0000a.rf")); + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("///")); + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("//////")); + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("///F0000a.rf")); + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("hh/a7/b-00006/F0000a.rf")); + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("/a7/b-00006/")); + Assert.assertFalse(SimpleGarbageCollector.isValidFileDelete("/a7/b-00006")); + + } +} http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java ---------------------------------------------------------------------- diff --git a/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java b/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java index be444dd..16a1912 100644 --- a/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java +++ b/src/server/src/test/java/org/apache/accumulo/server/gc/TestConfirmDeletes.java @@ -53,6 +53,37 @@ public class TestConfirmDeletes { return result; } + @Test(expected = IllegalArgumentException.class) + public void testInvalidBlip() throws Exception { + String metadata[] = new String[] {"1636< file:/b-0001/someFile 10,100", "1636< last:3353986642a66eb 192.168.117.9:9997", + "1636< srv:dir /default_tablet", "1636< srv:flush 2", "1636< srv:lock tservers/192.168.117.9:9997/zlock-0000000000$3353986642a66eb", + "1636< srv:time M1328505870023", "1636< ~tab:~pr \0"}; + String deletes[] = {"~blip/", "~del/1636/b-0001/someFile"}; + + test1(metadata, deletes, 1, 0); + } + + @Test(expected = IllegalArgumentException.class) + public void testInvalidFile() throws Exception { + String metadata[] = new String[] {"1636< file:/b-0001/someFile/ooopppsss/garbage 10,100", "1636< last:3353986642a66eb 192.168.117.9:9997", + "1636< srv:dir /default_tablet", "1636< srv:flush 2", "1636< srv:lock tservers/192.168.117.9:9997/zlock-0000000000$3353986642a66eb", + "1636< srv:time M1328505870023", "1636< ~tab:~pr \0"}; + String deletes[] = {"~del/1636/b-0001/someFile"}; + + test1(metadata, deletes, 1, 0); + } + + @Test + public void testInvalidDeletes() throws Exception { + // a few invalid deletes, should be ignored + String metadata[] = new String[] {"1636< file:/default_tablet/someFile 10,100", "1636< last:3353986642a66eb 192.168.117.9:9997", + "1636< srv:dir /default_tablet", "1636< srv:flush 2", "1636< srv:lock tservers/192.168.117.9:9997/zlock-0000000000$3353986642a66eb", + "1636< srv:time M1328505870023", "1636< ~tab:~pr \0"}; + String deletes[] = {"~del", "~del/"}; + + test1(metadata, deletes, 0, 0); + } + @Test public void test() throws Exception { @@ -88,6 +119,10 @@ public class TestConfirmDeletes { deletes = new String[] {"~del/9/default_tablet", "~del/9/default_tablet/someFile"}; test1(metadata, deletes, 2, 0); + metadata = new String[] {"1636< file:/b-0001/I0000 10,100", "1636< last:3353986642a66eb 192.168.117.9:9997", "1636< srv:dir /default_tablet", + "1636< srv:flush 2", "1636< srv:lock tservers/192.168.117.9:9997/zlock-0000000000$3353986642a66eb", "1636< srv:time M1328505870023", + "1636< ~tab:~pr \0"}; + deletes = new String[] {"~blip/1636/b-0001", "~del/1636/b-0001/I0000"}; test1(metadata, deletes, 1, 0); } http://git-wip-us.apache.org/repos/asf/accumulo/blob/ca3f7789/test/system/auto/simple/gc.py ---------------------------------------------------------------------- diff --git a/test/system/auto/simple/gc.py b/test/system/auto/simple/gc.py index 4697246..2b7e2c4 100755 --- a/test/system/auto/simple/gc.py +++ b/test/system/auto/simple/gc.py @@ -60,6 +60,8 @@ class GCTest(SunnyDayTest): def runTest(self): self.waitForStop(self.ingester, 60) self.shell(self.masterHost(), 'flush -t test_ingest') + #following statemets caused GC to delete everything ACCUMULO-2520 + self.shell(self.masterHost(), 'grant Table.WRITE -u root -t !METADATA\ntable !METADATA\ninsert ~del testDel test valueTest\n') self.stop_gc(self.masterHost()) count = self.waitForFileCountToStabilize() @@ -74,6 +76,13 @@ class GCTest(SunnyDayTest): glob.glob(os.path.join(ACCUMULO_HOME,'logs',ID,'gc_*'))) out, err = handle.communicate() self.assert_(handle.returncode != 0) + + handle = self.runOn(self.masterHost(), + ['grep', '-q', 'Ingoring invalid deletion candidate'] + + glob.glob(os.path.join(ACCUMULO_HOME,'logs',ID,'gc_*'))) + out, err = handle.communicate() + self.assert_(handle.returncode == 0) + self.pkill(self.masterHost(), 'java.*Main gc$', signal.SIGHUP) self.wait(gc) log.info("Verifying Ingestion")
