Merge #27075 #27087

craig[bot] · marc · craig[bot] · commit b18abeb32a7a · 2018-07-05T08:40:55.000Z
27075: storage: use engine for os-level operations. r=mberhault a=mberhault

This is important when encryption is enabled as we need to make sure we
handle file metadata properly.
While `os.Remove` will leave cruft behind, `os.Link` won't copy the file
encryption settings from the original and will therefore make it
unreadable.

This fixes `RESTORE` which writes file to local disk using local
encryption settings then ingests them. The link bypassed the rocksdb Env
leaving the ingested file without any encryption settings attached (aka:
plaintext).

Release note: None

27087: roachtest: disable encryption on tests that can't handle it. r=mberhault a=mberhault

Two cases make encryption always fail:
- old versions (&lt;= 2.0)
- uses of debug commands that open the rocksdb instance (needs
  encryption flags)

Release note: None

Co-authored-by: marc &lt;marc@cockroachlabs.com&gt;
diff --git a/c-deps/libroach/batch.cc b/c-deps/libroach/batch.cc
@@ -535,6 +535,8 @@ DBStatus DBBatch::EnvDeleteFile(DBSlice path) { return FmtStatus("unsupported");
 
 DBStatus DBBatch::EnvDeleteDirAndFiles(DBSlice dir) { return FmtStatus("unsupported"); }
 
+DBStatus DBBatch::EnvLinkFile(DBSlice oldname, DBSlice newname) { return FmtStatus("unsupported"); }
+
 DBWriteOnlyBatch::DBWriteOnlyBatch(DBEngine* db) : DBEngine(db->rep, db->iters), updates(0) {}
 
 DBWriteOnlyBatch::~DBWriteOnlyBatch() {}
@@ -628,6 +630,8 @@ DBStatus DBWriteOnlyBatch::EnvDeleteFile(DBSlice path) { return FmtStatus("unsup
 
 DBStatus DBWriteOnlyBatch::EnvDeleteDirAndFiles(DBSlice dir) { return FmtStatus("unsupported"); }
 
+DBStatus DBWriteOnlyBatch::EnvLinkFile(DBSlice oldname, DBSlice newname) { return FmtStatus("unsupported"); }
+
 rocksdb::WriteBatch::Handler* GetDBBatchInserter(::rocksdb::WriteBatchBase* batch) {
   return new DBBatchInserter(batch);
 }
diff --git a/c-deps/libroach/batch.h b/c-deps/libroach/batch.h
@@ -49,6 +49,7 @@ struct DBBatch : public DBEngine {
   virtual DBStatus EnvCloseFile(rocksdb::WritableFile* file);
   virtual DBStatus EnvDeleteFile(DBSlice path);
   virtual DBStatus EnvDeleteDirAndFiles(DBSlice dir);
+  virtual DBStatus EnvLinkFile(DBSlice oldname, DBSlice newname);
 };
 
 struct DBWriteOnlyBatch : public DBEngine {
@@ -78,6 +79,7 @@ struct DBWriteOnlyBatch : public DBEngine {
   virtual DBStatus EnvCloseFile(rocksdb::WritableFile* file);
   virtual DBStatus EnvDeleteFile(DBSlice path);
   virtual DBStatus EnvDeleteDirAndFiles(DBSlice dir);
+  virtual DBStatus EnvLinkFile(DBSlice oldname, DBSlice newname);
 };
 
 // GetDBBatchInserter returns a WriteBatch::Handler that operates on a
diff --git a/c-deps/libroach/db.cc b/c-deps/libroach/db.cc
@@ -477,6 +477,8 @@ DBStatus DBEnvDeleteFile(DBEngine* db, DBSlice path) { return db->EnvDeleteFile(
 
 DBStatus DBEnvDeleteDirAndFiles(DBEngine* db, DBSlice dir) { return db->EnvDeleteDirAndFiles(dir); }
 
+DBStatus DBEnvLinkFile(DBEngine* db, DBSlice oldname, DBSlice newname) { return db->EnvLinkFile(oldname, newname); }
+
 DBIterator* DBNewIter(DBEngine* db, bool prefix, bool stats) {
   rocksdb::ReadOptions opts;
   opts.prefix_same_as_start = prefix;
diff --git a/c-deps/libroach/engine.cc b/c-deps/libroach/engine.cc
@@ -435,4 +435,9 @@ DBStatus DBImpl::EnvDeleteDirAndFiles(DBSlice dir) {
   return ToDBStatus(status);
 }
 
+// EnvLinkFile creates 'newname' as a hard link to 'oldname'.
+DBStatus DBImpl::EnvLinkFile(DBSlice oldname, DBSlice newname) {
+  return ToDBStatus(this->rep->GetEnv()->LinkFile(ToString(oldname), ToString(newname)));
+}
+
 }  // namespace cockroach
diff --git a/c-deps/libroach/engine.h b/c-deps/libroach/engine.h
@@ -50,6 +50,7 @@ struct DBEngine {
   virtual DBStatus EnvCloseFile(rocksdb::WritableFile* file) = 0;
   virtual DBStatus EnvDeleteFile(DBSlice path) = 0;
   virtual DBStatus EnvDeleteDirAndFiles(DBSlice dir) = 0;
+  virtual DBStatus EnvLinkFile(DBSlice oldname, DBSlice newname) = 0;
 
   DBSSTable* GetSSTables(int* n);
   DBString GetUserProperties();
@@ -94,6 +95,7 @@ struct DBImpl : public DBEngine {
   virtual DBStatus EnvCloseFile(rocksdb::WritableFile* file);
   virtual DBStatus EnvDeleteFile(DBSlice path);
   virtual DBStatus EnvDeleteDirAndFiles(DBSlice dir);
+  virtual DBStatus EnvLinkFile(DBSlice oldname, DBSlice newname);
 };
 
 }  // namespace cockroach
diff --git a/c-deps/libroach/include/libroach.h b/c-deps/libroach/include/libroach.h
@@ -402,6 +402,9 @@ DBStatus DBEnvDeleteFile(DBEngine* db, DBSlice path);
 // files it contains but not subdirectories in the given engine.
 DBStatus DBEnvDeleteDirAndFiles(DBEngine* db, DBSlice dir);
 
+// DBEnvLinkFile creates 'newname' as a hard link to 'oldname using the given engine.
+DBStatus DBEnvLinkFile(DBEngine* db, DBSlice oldname, DBSlice newname);
+
 // DBFileLock contains various parameters set during DBLockFile and required for DBUnlockFile.
 typedef void* DBFileLock;
 
diff --git a/c-deps/libroach/snapshot.cc b/c-deps/libroach/snapshot.cc
@@ -79,4 +79,6 @@ DBStatus DBSnapshot::EnvDeleteFile(DBSlice path) { return FmtStatus("unsupported
 
 DBStatus DBSnapshot::EnvDeleteDirAndFiles(DBSlice dir) { return FmtStatus("unsupported"); }
 
+DBStatus DBSnapshot::EnvLinkFile(DBSlice oldname, DBSlice newname) { return FmtStatus("unsupported"); }
+
 }  // namespace cockroach
diff --git a/c-deps/libroach/snapshot.h b/c-deps/libroach/snapshot.h
@@ -46,6 +46,7 @@ struct DBSnapshot : public DBEngine {
   virtual DBStatus EnvCloseFile(rocksdb::WritableFile* file);
   virtual DBStatus EnvDeleteFile(DBSlice path);
   virtual DBStatus EnvDeleteDirAndFiles(DBSlice dir);
+  virtual DBStatus EnvLinkFile(DBSlice oldname, DBSlice newname);
 };
 
 }  // namespace cockroach
diff --git a/pkg/cmd/roachtest/cdc.go b/pkg/cmd/roachtest/cdc.go
@@ -33,7 +33,9 @@ func registerCDC(r *registry) {
 
 		c.Put(ctx, cockroach, "./cockroach", crdbNodes)
 		c.Put(ctx, workload, "./workload", workloadNode)
-		c.Start(ctx, crdbNodes)
+		// Force encryption off as we do not have a good way to detect whether it is enabled
+		// for the `debug compact` call below.
+		c.Start(ctx, crdbNodes, startArgsDontEncrypt)
 
 		t.Status("loading initial data")
 		c.Run(ctx, workloadNode, fmt.Sprintf(
@@ -44,7 +46,7 @@ func registerCDC(r *registry) {
 		// fixed. See #26870
 		c.Stop(ctx, crdbNodes)
 		c.Run(ctx, crdbNodes, `./cockroach debug compact /mnt/data1/cockroach/`)
-		c.Start(ctx, crdbNodes)
+		c.Start(ctx, crdbNodes, startArgsDontEncrypt)
 
 		t.Status("installing kafka")
 		c.Run(ctx, kafkaNode, `curl https://packages.confluent.io/archive/4.0/confluent-oss-4.0.0-2.11.tar.gz | tar -xzv`)
diff --git a/pkg/cmd/roachtest/cluster.go b/pkg/cmd/roachtest/cluster.go
@@ -699,6 +699,11 @@ func startArgs(extraArgs ...string) option {
 	return roachprodArgOption(extraArgs)
 }
 
+// startArgsDontEncrypt will pass '--encrypt=false' to roachprod regardless of the
+// --encrypt flag on roachtest. This is useful for tests that cannot pass with
+// encryption enabled.
+var startArgsDontEncrypt = startArgs("--encrypt=false")
+
 // stopArgs specifies extra arguments that are passed to `roachprod` during `c.Stop`.
 func stopArgs(extraArgs ...string) option {
 	return roachprodArgOption(extraArgs)
diff --git a/pkg/cmd/roachtest/upgrade.go b/pkg/cmd/roachtest/upgrade.go
@@ -43,7 +43,9 @@ func registerUpgrade(r *registry) {
 		}
 
 		c.Put(ctx, b, "./cockroach", c.Range(1, nodes))
-		c.Start(ctx, c.Range(1, nodes))
+		// Force disable encryption.
+		// TODO(mberhault): allow it once oldVersion >= 2.1.
+		c.Start(ctx, c.Range(1, nodes), startArgsDontEncrypt)
 
 		const stageDuration = 30 * time.Second
 		const timeUntilStoreDead = 90 * time.Second
@@ -129,7 +131,7 @@ func registerUpgrade(r *registry) {
 				t.Fatal(err)
 			}
 			c.Put(ctx, cockroach, "./cockroach", c.Node(i))
-			c.Start(ctx, c.Node(i))
+			c.Start(ctx, c.Node(i), startArgsDontEncrypt)
 			if err := sleep(stageDuration); err != nil {
 				t.Fatal(err)
 			}
@@ -151,7 +153,7 @@ func registerUpgrade(r *registry) {
 			t.Fatal(err)
 		}
 		c.Put(ctx, cockroach, "./cockroach", c.Node(nodes))
-		c.Start(ctx, c.Node(nodes))
+		c.Start(ctx, c.Node(nodes), startArgsDontEncrypt)
 		if err := sleep(stageDuration); err != nil {
 			t.Fatal(err)
 		}
@@ -192,7 +194,7 @@ func registerUpgrade(r *registry) {
 		}
 
 		// Restart the previously stopped node.
-		c.Start(ctx, c.Node(nodes-1))
+		c.Start(ctx, c.Node(nodes-1), startArgsDontEncrypt)
 		if err := sleep(stageDuration); err != nil {
 			t.Fatal(err)
 		}
diff --git a/pkg/cmd/roachtest/version.go b/pkg/cmd/roachtest/version.go
@@ -48,7 +48,9 @@ func registerVersion(r *registry) {
 		c.Put(ctx, workload, "./workload", c.Node(nodes+1))
 
 		c.Put(ctx, b, "./cockroach", c.Range(1, nodes))
-		c.Start(ctx, c.Range(1, nodes))
+		// Force disable encryption.
+		// TODO(mberhault): allow it once version >= 2.1.
+		c.Start(ctx, c.Range(1, nodes), startArgsDontEncrypt)
 
 		stageDuration := 10 * time.Minute
 		buffer := 10 * time.Minute
@@ -167,7 +169,7 @@ func registerVersion(r *registry) {
 					return err
 				}
 				c.Put(ctx, cockroach, "./cockroach", c.Node(i))
-				c.Start(ctx, c.Node(i))
+				c.Start(ctx, c.Node(i), startArgsDontEncrypt)
 				if err := sleepAndCheck(); err != nil {
 					return err
 				}
@@ -188,7 +190,7 @@ func registerVersion(r *registry) {
 
 			// Do upgrade for the last node.
 			c.Put(ctx, cockroach, "./cockroach", c.Node(nodes))
-			c.Start(ctx, c.Node(nodes))
+			c.Start(ctx, c.Node(nodes), startArgsDontEncrypt)
 			if err := sleepAndCheck(); err != nil {
 				return err
 			}
@@ -200,7 +202,7 @@ func registerVersion(r *registry) {
 					return err
 				}
 				c.Put(ctx, b, "./cockroach", c.Node(i))
-				c.Start(ctx, c.Node(i))
+				c.Start(ctx, c.Node(i), startArgsDontEncrypt)
 				if err := sleepAndCheck(); err != nil {
 					return err
 				}
@@ -213,7 +215,7 @@ func registerVersion(r *registry) {
 					return err
 				}
 				c.Put(ctx, cockroach, "./cockroach", c.Node(i))
-				c.Start(ctx, c.Node(i))
+				c.Start(ctx, c.Node(i), startArgsDontEncrypt)
 				if err := sleepAndCheck(); err != nil {
 					return err
 				}
diff --git a/pkg/storage/engine/engine.go b/pkg/storage/engine/engine.go
@@ -290,6 +290,10 @@ type Engine interface {
 	// not subdirectories from this RocksDB's env. If dir does not exist,
 	// DeleteDirAndFiles returns nil (no error).
 	DeleteDirAndFiles(dir string) error
+	// LinkFile creates 'newname' as a hard link to 'oldname'. This is done using
+	// the engine implementation. For RocksDB, this means using the Env responsible for the file
+	// which may handle extra logic (eg: copy encryption settings for EncryptedEnv).
+	LinkFile(oldname, newname string) error
 }
 
 // WithSSTables extends the Engine interface with a method to get info
diff --git a/pkg/storage/engine/rocksdb.go b/pkg/storage/engine/rocksdb.go
@@ -2713,6 +2713,20 @@ func (r *RocksDB) DeleteDirAndFiles(dir string) error {
 	return nil
 }
 
+// LinkFile creates 'newname' as a hard link to 'oldname'. This use the Env responsible for the file
+// which may handle extra logic (eg: copy encryption settings for EncryptedEnv).
+func (r *RocksDB) LinkFile(oldname, newname string) error {
+	if err := statusToError(C.DBEnvLinkFile(r.rdb, goToCSlice([]byte(oldname)), goToCSlice([]byte(newname)))); err != nil {
+		return &os.LinkError{
+			Op:  "link",
+			Old: oldname,
+			New: newname,
+			Err: err,
+		}
+	}
+	return nil
+}
+
 // IsValidSplitKey returns whether the key is a valid split key. Certain key
 // ranges cannot be split (the meta1 span and the system DB span); split keys
 // chosen within any of these ranges are considered invalid. And a split key
diff --git a/pkg/storage/replica_proposal.go b/pkg/storage/replica_proposal.go
@@ -384,14 +384,14 @@ func addSSTablePreApply(
 			// If the fs supports it, make a hard-link for rocks to ingest. We cannot
 			// pass it the path in the sideload store as it deletes the passed path on
 			// success.
-			if linkErr := os.Link(path, ingestPath); linkErr == nil {
+			if linkErr := eng.LinkFile(path, ingestPath); linkErr == nil {
 				ingestErr := eng.IngestExternalFiles(ctx, []string{ingestPath}, noModify)
 				if ingestErr == nil {
 					// Adding without modification succeeded, no copy necessary.
 					log.Eventf(ctx, "ingested SSTable at index %d, term %d: %s", index, term, ingestPath)
 					return false
 				}
-				if rmErr := os.Remove(ingestPath); rmErr != nil {
+				if rmErr := eng.DeleteFile(ingestPath); rmErr != nil {
 					log.Fatalf(ctx, "failed to move ingest sst: %v", rmErr)
 				}
 				const seqNoMsg = "Global seqno is required, but disabled"

Original file line number	Diff line number	Diff line change
`@@ -435,4 +435,9 @@ DBStatus DBImpl::EnvDeleteDirAndFiles(DBSlice dir) {`
`435`	`435`	`return ToDBStatus(status);`
`436`	`436`	`}`
`437`	`437`
	`438`	`+// EnvLinkFile creates 'newname' as a hard link to 'oldname'.`
	`439`	`+DBStatus DBImpl::EnvLinkFile(DBSlice oldname, DBSlice newname) {`
	`440`	`+ return ToDBStatus(this->rep->GetEnv()->LinkFile(ToString(oldname), ToString(newname)));`
	`441`	`+}`
	`442`	`+`
`438`	`443`	`} // namespace cockroach`