Summary: Add --readonly-storage option to cmdlib that will cause an error on any attempt to write to SQL metadata or Blobstore
Reviewed By: StanislavGlebik
Differential Revision: D18297959
fbshipit-source-id: e879183b74fb50abfb60d2424ea579708322963f
Summary:
This reworks our CoreContext a little bit to contain two fields instead of putting everything together in just one `Inner` struct. There's a few reasons why I'd like to make that change:
- First, with the `Inner` approach, everything has to be cloneable in `Inner`, even things that are largely static for a given session, because that's how we create new contexts to update their logger or Scuba sample. If we split things out, then we can clone the Logger & Scuba without cloning Inner.
- Second, this approach allows for better separation of concerns in the repo handler. Right now, it's a bit of a mess: many of our methods there aren't actually providing their command in Scuba for example, because they just use `self.ctx` and forget to update the method. It's just too error-prone. By separating things out, we have a data model that maps a little better to the state of the world (one session, multiple commands), and we make sure that we can't accidentally use a `ctx` without first tying it to a request. We had `prepared_ctx` as an attempt to do that, but since it wasn't mandatory it wasn't used everyhwere properly. The replacement `command_ctx` forces command code to pass their command in order to acquire a `CoreContext`.
Note that this diff is a lot of busywork here and there to update callsites accordingly. That said, there is one functional change in the commit cloud bookmarks filler, which was using a method it didn't need (and which never worked), so I took that out.
Relatedly, note that I removed `CoreContext` from the bundle2 parsing code, and passed just a `Logger` there. That code doesn't actually use `CoreContext`. This allowed for removing a few TODO's of dtolnay's when he introduced `FacebookInit`.
Reviewed By: StanislavGlebik
Differential Revision: D18352597
fbshipit-source-id: cd91042cef666c38b9cbd5f07518bc558e172aa2
Summary: This is to allow for cross-table transactions in integration tests.
Reviewed By: StanislavGlebik
Differential Revision: D18322477
fbshipit-source-id: 7bcc3ece8e2e75af65f008054c9461f77da43bc7
Summary:
Release notes: https://github.com/rust-lang/rust/blob/stable/RELEASES.md#version-1390-2019-11-07
Dist: https://dev-static.rust-lang.org/dist/2019-11-04/index.html
This diff includes fixes for new warnings inside of targets that set deny(warnings). I will fix any other new warnings in a separate diff to keep this one as small as possible.
- feature(bind_by_move_pattern_guards) has been stabilized
- feature(async_await) has been stabilized
- new warning on duplicate semicolon typos;;
- deprecation of std::mem::uninitialized in favor of MaybeUninit
- adjustment to how locals are captured in non-move async blocks
- soundness fix to disallow Send of fmt::Arguments
Reviewed By: jsgf
Differential Revision: D18358129
fbshipit-source-id: 718b6b7f3e35bf0f3d7098a07739191c862ac82d
Summary: Add a tool that can backsync one repo into another
Reviewed By: ikostia
Differential Revision: D17977534
fbshipit-source-id: 653fd9263ea05541184ebfb233ef7eb20aead02f
Summary:
This diff updates all license headers to use the new text and style.
Also, a few internal files were missing the header, but now they have it.
`fbcode/common/rust/netstring/` had the internal header, but now it has
GPLV2PLUS - since that goes to Mononoke's Github too.
Differential Revision: D17881539
fbshipit-source-id: b70d2ee41d2019fc7c2fe458627f0f7c01978186
Summary:
As suggested by yfeldblum in https://our.intern.facebook.com/intern/diff/D17544447/?transaction_id=2964022370277861.
> the `rust_` bit seems superfluous; the `_thrift` bit opens the door for confusion with apache thrift. Any issue using a module name like `fbthrift`?
---
```
$ fastmod '\brust_thrift\b' fbthrift -d ~/fbcode -e rs,toml
$ fastmod '\brust_thrift\b' fbthrift -d ~/fbcode -g '**/TARGETS'
$ hg revert experimental
$ arc lint -a
```
Reviewed By: bolinfest
Differential Revision: D17611123
fbshipit-source-id: b621a422480b00eb2e339ff7542cc66c3ca5b8ec
Summary:
This updates multiplexblob to avoid waiting for the SQL queue if writes have
succeeded in all the blobstores we are writing to.
It's worth noting that this might mean adding a new blobstore will require a
little more effort, since we no longer guarantee that the blobstore queue will
contain all writes that we made. That said, we weren't really in a position to
100% rely on this anyway (writes can in fact succeed even if the SQL queue
doesn't get written to), so in that sense it doesn't make a huge difference.
Reviewed By: farnz
Differential Revision: D17421208
fbshipit-source-id: 0f2ecbf22ba51531a5917baf912183d5309e1b63
Summary:
Change the multiplexed blobstore logging to log the blobstore key.
Also, fix the get() method to log errors to scuba (which it wasn't before!).
Further, refactor the code to remove env parsing for tupperware information and also switch
to using ScubaSampleBuilder. This means that the common server information can
be stored once and then cloned each time we log, rather than iterating through
a vec of server information each time we want to log.
Additionally, using ScubaSampleBuilder means that we don't need to pass in an Option<ScubaClient>,
cleaning up the code a little bit.
Reviewed By: StanislavGlebik
Differential Revision: D17368779
fbshipit-source-id: 0896962cdbd37912fc6f23a5e541e10cea90fa0e
Summary:
This diff moves initFacebook calls that used to happen just before FFI calls to instead happen at the beginning of main.
The basic assumption of initFacebook is that it happens at the beginning of main before there are additional threads. It must be allowed to modify process-global state like env vars or gflags without the possibility of a data race from other code concurrently reading those things. As such, the previous approach of calling initFacebook through `*fbinit::FACEBOOK` near FFI calls was prone to race conditions.
The new approach is based on attribute macros added in D17245802.
---
The primary remaining situations that still require `*fbinit::FACEBOOK` are when we don't directly control the function arguments surrounding the call to C++, such as in lazy_static:
lazy_static! {
static ref S: Ty = {
let _ = *fbinit::FACEBOOK;
/* call C++ */
};
}
and quickcheck:
quickcheck! {
fn f(/* args that impl Arbitrary */) {
let _ = *fbinit::FACEBOOK;
/* call C++ */
}
}
I will revisit these in a separate diff. They are a small fraction of total uses of fbinit.
Reviewed By: Imxset21
Differential Revision: D17328504
fbshipit-source-id: f80edb763e7f42b3216552dd32f1ea0e6cc8fd12
Summary: Add lease_type to memcache lease stats so that its clearer what types of leases are driving lease waits
Reviewed By: HarveyHunt
Differential Revision: D17344425
fbshipit-source-id: ea1ae44319428bc1705f502ad9fa1b2b0c44bf96
Summary:
We don't need to create double-indirection when accepting `>list` arguments.
This tends to force callers into creating new Vecs of references here and
there.
Since I was in here fixing D17286200, I figured I might as well do this too.
Reviewed By: farnz
Differential Revision: D17286608
fbshipit-source-id: 994f7d6da309b16b4e613d05faeaa3ae70ae70ab
Summary: I think these are left over from pre-2018 code where they may have been necessary. In 2018 edition, import paths in `use` always begin with a crate name or `crate`/`super`/`self`, so `use $ident;` always refers to a crate. Since extern crates are always in scope in every module, `use $ident` does nothing.
Reviewed By: Imxset21
Differential Revision: D17290473
fbshipit-source-id: 23d86e5d0dcd5c2d4e53c7a36b4267101dd4b45c
Summary:
That's something we'd like to do for a while - for each request track how many requests it
sends to our storages (i.e. manifold and xdb). That might make perf debugging easier.
There's a concern that it might increase cpu usage, and I'll run a canary to check
if it's the case.
Reviewed By: krallin
Differential Revision: D17091115
fbshipit-source-id: 27fea314241d883ced72d88d39f2e188716a1b9a
Summary: This makes it easier to measure performance differences across canary runs.
Reviewed By: StanislavGlebik
Differential Revision: D17225082
fbshipit-source-id: bfabfddaaaca711ed4f973bfbcdd93d618f90b33
Summary: If some blob is missing it becomes very hard to debug what is missing. This diff extends `LoadableError::Missing` with string argument.
Reviewed By: krallin
Differential Revision: D17132049
fbshipit-source-id: df1f60b1f739c8a837d1b1798edbaca58b1587f9
Summary:
Utility to backfill derived data:
- currently only support `unodes`
Reviewed By: krallin
Differential Revision: D16827897
fbshipit-source-id: aa0bb8ae4fe895233e3f11775b3432184afb37f9
Summary:
RedactedBlobstore is a part of `RepoBlobstore` which is the primary blobstore used in mononoke, it is cloned basically everywhere. This change make it clonable without allocations.
- redacted hash table was cloned on every clone of `RedactedBlostore`
- scuba_builder also contain hash table which was cloned on every clone or `RedactedBlobstore`
Reviewed By: krallin
Differential Revision: D16961607
fbshipit-source-id: 59fe56f0395b0184418ed8b709be8ecdefb9f4ce
Summary:
In order to remove struct Id hack in the next diff we need to implement
Manifest trait for ManifestUnode struct. However, there's a problem:
ManifestUnode::TreeId type is ManifestUnodeId which implements
MononokeId trait. And all MononokeId types automatically implement
Loadable<Value=Option<MononokeId>>. But Manifest trait requires TreeId that
implement Loadable<Value=Self> - note that there's no Option<...>, and so
ManifestUnodeId Loadable implementation doesn't match implementation required
by Manifest trait.
There are a few ways to fix it (see one of them in D16885043), and after some
discussion we decided that the best way to proceed is to add a special error
LoadableError that might be `Missing` meaning that id we are trying to fetch
doesn't exist
Reviewed By: aslpavel
Differential Revision: D16915387
fbshipit-source-id: df57ff4a7ab4ddbff5812ce5929a742d949726af
Summary:
This is a mechanical part of rename, does not change any commit messages in
tests, does not change the scuba table name/config setting. Those are more
complex.
Reviewed By: krallin
Differential Revision: D16890120
fbshipit-source-id: 966c0066f5e959631995a1abcc7123549f7495b6
Summary:
mononoke_types doesn't look like a good crate for it. Let's move to blobstore
crate instead
Reviewed By: krallin
Differential Revision: D16914142
fbshipit-source-id: 57602c5a644adc87beea0cbc92beeb0d901d5873
Summary:
This is the first diff in the stack that removes ugly hack `struct Id` that we
used in derived data (see more details about that later in the stack).
This diff reverses the dependency between blobstore crate and mononoke_types
crate. Previously blobstore depended on mononoke_types, and blobstore also had
Loadable/Storable traits. That means that we can't implement Loadable/Storable
traits for types from mononoke_types - we can't even import them without
creating cyclic dependency between crates! And we'll use Loadable in
mononoke_types later in the stack.
This diff reverses the dependency and moves Loadable/Storable in mononoke_types
Reviewed By: krallin
Differential Revision: D16884649
fbshipit-source-id: 20753f3e62980be7bb6a04ab9d19980bc459b40e
Summary:
This updates an earlier diff I had that introduced the typed fetch method on the Blobstore trait. Since we now have Storable and Loadable traits, it makes a little more sense to implement those for MononokeId and use that instead.
I was hoping this would mean we could move the implementation of those traits into the mononoke_types crate, but unfortunately it looks like the `where T: MononokeId` implementation for `Loadable` can't live there (Rust wants it next to the trait definition).
That said, this change does mean we can get rid of some manual BlobstoreBytes creation here and there, so that's still an upside.
In doing so, I updated the Loadable and Storable traits to require only a Blobstore reference, and to take ownership of what they're storing. This saves some clones, especially in the latter.
Reviewed By: aslpavel
Differential Revision: D16733733
fbshipit-source-id: 3ec3a97cef90d6850f2939ba3f5ec6e6e5b459fb
Summary: Use memcache leases to prevent simultaneous generation of derived data for the same changeset id.
Reviewed By: StanislavGlebik
Differential Revision: D16666659
fbshipit-source-id: 47e57449ab854e595a9dc860414c49afa966be01
Summary:
This resolves the list of files at a given commit to a list of `ContentId`s
and unblacklists all of those `ContentId`s.
Note 1: This means that these particular versions of files will be
unblacklisted in all commits which reference them.
Note 2: There's no auditing of this admin command invokation, if the
need for one arises, we should add it externally.
Reviewed By: krallin
Differential Revision: D16643436
fbshipit-source-id: 3af488e3e4d748027605587ab1e08c0abd0da109
Summary:
The healer is a blobstore-level operation, which is orthogonal to the concept of a repo; therefore, there should be no mention of repoid in any of the healer's structures or tables.
For now this leaves the schema unmodified, and fills the repoid with a dummy value (0). We can clean that up later.
Reviewed By: lukaspiatkowski, HarveyHunt
Differential Revision: D15051896
fbshipit-source-id: 438b4c6885f18934228f43d85cdb8bf2f0e542f1
Summary: RepositoryId shouldn't leak into the blobstore layer. This leaves repoid in the schema, but just populates it with a dummy value (0). We can clean up the schema and this code in a later diff.
Reviewed By: StanislavGlebik
Differential Revision: D15021285
fbshipit-source-id: 3ecb04a76ce74409ed0cced3d2a0217eacd0e2fb
Summary:
NOTE: This diff updates Thrift serialization for chunked files. Normally, this wouldn't be OK, but we never stored anything in this format, so that's fine.
This updates the Filestore to rebuild backmappings on read. The rationale for this is that since we store backmappings *after* writing FileContents, it is possible to have FileContents that logically exist but don't have backmappings. This should be a fairly exceptional case, but we can handle it by recomputing the missing backmappings on the fly.
As part of this change, I'm updating our Thrift serialization for chunked files and backmappings (as mentioned earlier, this is normally not a good idea, but it should be fine here):
- Chunked files now contain chunk lengths, which lets us derive offsets as well as total size for a chunked file.
- Backmappings don't contain the size anymore (since FileContents contains it now).
This was necessary because we need to have a file's size in order to recompute backmappings.
In this change, I also updated GitSha1 to stop embedding its blob type and length. This lets us reconstruct a strongly-typed GitSha1 object from just the hash (and was therefore necessary to avoid duplicating file size into backmappings), which seems sufficient at this stage (and if we do need the size, we can obtain it through the Filestore by querying the file).
Reviewed By: aslpavel
Differential Revision: D16440676
fbshipit-source-id: 23b66caf40fde2a2f756fef89af9fe0bb8bdadef
Summary:
This adds support for chunking in the Filestore. To do so, this reworks writes to be a 2 stage process:
- First, you prepare your write. This puts everything into place, but doesn't upload the aliases, backmappings, and the logical FileContents blob representing your file. If you're uploading a file that fits in a single chunk, preparing your writes effectively makes no blobstore changes. If you're uploading chunks, then you upload the individual chunks (those are uploaded essentially as if they were small files).
- Then, you commit your write. At that point, prepared gave you `FileContents` that represent your write, and a set of aliases. To commit your write, you write the aliases, then the file contents, and then a backmapping.
Note that we never create hierarchies when writing to the Filestore (i.e. chunked files always reference concrete chunks that contain file data), but that's not a guarantee we can rely on when reading. Indeed, since chunks and files are identical as far as blobstore storage is concerned, we have to handle the case where a chunked file references chunks that are themselves chunked (as I mentioned, we won't write it that way, but it could happen if we uploaded a file, then later on reduced the chunk size and wrote an identical file).
Note that this diff also updates the FileContents enum and adds unimplemented methods there. These are all going away later in this stack (in the diff where I incorporate the Filestore into the rest of Mononoke).
Reviewed By: StanislavGlebik
Differential Revision: D16440678
fbshipit-source-id: 07187aa154f4fdcbd3b3faab7c0cbcb1f8a91217
Summary:
This moves typed fetches from the blobrepo to the blobstore. The upshot is that this allows consumers of a blobstore to do typed fetches, instead of forcing them to get bytes then cast bytes to a blob, then cast the blob to the thing they want.
This required refactoring our crate hierarchy a little bit by moving BlobstoreBytes into Mononoke Types, since we now need the Blobstore crate to depend on Mononoke types, whereas it was the other way around before (since BlobstoreValue was already there, that seems reasonable)
Reviewed By: StanislavGlebik
Differential Revision: D16486774
fbshipit-source-id: 05751986ce3cb7273d68a8b4ebe9957bb892bcd6
Summary:
Any get request for a blacklisted file returns a magic string - not an error anymore - which replaces the actual content of the file. The magic string represents a sequence of 256 random characters.
Put still returns an error if there is an attempt to change a blacklisted file, but that's the normal behavior for this type of request.
Reviewed By: ikostia
Differential Revision: D16333820
fbshipit-source-id: 54e0189ebe2a71ea365979950d44dbb3fd1e5dc7
Summary:
This seems to be the standard way (e.g. `ManifoldBlob` does it :P) to report
fb303 counters for blobstores. Provided that we want XDB Blobstore to be a
full-featured one, we want this too.
Note: potentially it makes sense to just wrap every blobstore in the `CountedBlob` in `blobstore/factory`, I can work on that later. For now I want to be able to proceed with xdb blobstore and get its counters in the cheapest possible way.
Reviewed By: krallin
Differential Revision: D16441222
fbshipit-source-id: 04fffb4598a7929301f05faacd074d5c0c76848d
Summary:
Before this change, we would always include the shard id in our mysql-related fb303 counters. This is not perfect for two reasons:
- the the xdb blobstore we have 4K shards and 24 counters, so we were reporting 96K counters in total
- we rarely care about per-counter metrics anyway, since in most cases all queries are uniformly distributed
Therefore, let's change this approach to not use per-shard counters and use per-shardmap ones (when sharding is involved).
Reviewed By: krallin
Differential Revision: D16360591
fbshipit-source-id: b2df94a3ca9cacbf5c1f328b48e87b48cd18287e
Summary: This adds a few retries in create_raw_xdb_connection. This is intended as a first step towards solving some of the flakiness we've observed when connecting to MySQL through direct connections (sometimes, we fail to acquire certificates).
Reviewed By: farnz
Differential Revision: D16228401
fbshipit-source-id: 0804797aecfe0b917099191cd2a36ce4c077b949
Summary:
In earlier diffs in this stack, I updated the callsites that reference XDB tiers to use concrete &str types (which is what they were receiving until now ... but it wasn't spelled out as-is).
In this diff, I'm updating them to use owned `String` instead, which lets us hoist up `to_string()` and `clone()` calls in the stack, rather than pass down reference only to copy them later on.
This allows us to skip some unnecessary copies. Tt turns out we were doing quite a few "turn this String into a reference, pass it down the stack, then turn it back into a String".
Reviewed By: farnz
Differential Revision: D16260372
fbshipit-source-id: faec402a575833f6555130cccdc04e79ddb8cfef
Summary:
Instantiating a new DB connection may require remote calls to be made to e.g. Hipster to allocate a new certificate (this is only the case when connecting to MySQL).
Currently, our bindings to our underlying DB locator make a blocking call to pretend that this operaiton is synchronous: https://fburl.com/ytmljxkb
This isn't ideal, because this call might actually take time, and we might also occasionally want to retry it (we've had issues in our MySQL tests with acquiring certificates that retrying should resolve). Running this synchronously makes doing so inefficient.
This patch doesn't update that, but it fixes everything on the Rust side of things to stop expecting connections to return a `Result` (and to start expecting a Future instead).
In a follow up diff, I'll work on making the changes in common/rust/sql to start returning a Future here.
Reviewed By: StanislavGlebik
Differential Revision: D16221857
fbshipit-source-id: 263f9237ff9394477c65e455de91b19a9de24a20
Summary:
When the actual database schema was changed, this was left behind. Let's fix
this.
Reviewed By: StanislavGlebik
Differential Revision: D16254128
fbshipit-source-id: 9a5a0d453f8cc846e9f5f963aadb3a9e9b705ef8
Summary:
This patch adds optimized implementations of is_present() for FileBlob (which
we use in tests), and ManifoldBlob (which AFAIK, we don't use: I believe we use
ManifoldThriftBlob instead).
The underlying goal is to make sure we don't perform accidental reads of LFS
blobs when all we meant was to check a blob existed when committing a
changeset.
For ManifoldBlob, this is implemented by querying for an empty range, which
appears to be explicitly supported as per the Rust client's source code (it
sets `?withPayload=0` as a query parameter).
Reviewed By: StanislavGlebik
Differential Revision: D16262273
fbshipit-source-id: c055f63d114d40be2c448b8c8e34eb82d0e641b5
Summary:
This diff does two things:
- resolves a problem with dropping censorship information when calling
`in_memory_writes_READ_DOC_COMMENT`
- prevents someone from accidentally creating a `BlobRepo` where internal blobstore's prefix is different from the `repoid`. While prefix is conceptually unrelated to a blobstore, we do care that existing blobstores continue to work, so we need this safeguard.
Reviewed By: farnz
Differential Revision: D16163225
fbshipit-source-id: fc1c9d4dc32f6958b4b0e2e61026c1f3fe5f3b17
Summary: This is generic implementation of tree manifest. It is going to be used to replace current hg manifest generation logic, and to implement any tree manifest bonsai derived structures like UNodes.
Reviewed By: StanislavGlebik
Differential Revision: D16048964
fbshipit-source-id: 6ffbdabca8788f22d87a61daba412cd1f9447d57
Summary:
This diff sets two Rust lints to warn in fbcode:
```
[rust]
warn_lints = bare_trait_objects, ellipsis_inclusive_range_patterns
```
and fixes occurrences of those warnings within common/rust, hg, and mononoke.
Both of these lints are set to warn by default starting with rustc 1.37. Enabling them early avoids writing even more new code that needs to be fixed when we pull in 1.37 in six weeks.
Upstream tracking issue: https://github.com/rust-lang/rust/issues/54910
Reviewed By: Imxset21
Differential Revision: D16200291
fbshipit-source-id: aca11a7a944e9fa95f94e226b52f6f053b97ec74