Summary:
For Lua hooks, we needed to know whether to run the hook per file, or per changeset. Rust hooks know this implicitly, as they're built-in to the server.
Stop having the tests set an unnecessary config
Reviewed By: krallin
Differential Revision: D22282799
fbshipit-source-id: c9f6f6325823d06d03341f04ecf7152999fcdbe7
Summary:
D21642461 (46d2b44c0e) converted Mononoke server to use the
`--mononoke-config-path` common argument style to select a config path.
Now that this change has been running for a while, remove the extra logic in
the server that allowed it to accept both the deprecated `--config_path / -P`
and the new arg.
Reviewed By: ikostia
Differential Revision: D22257386
fbshipit-source-id: 7da4ed4e0039d3659f8872693fa4940c58bae844
Summary:
`get_entry_with_small_repo_mapings` is a function that turns a `CommitEntry`
struct into `CommitEntryWithSmallReposMapped` struct - the idea being that this
function looks up hashes of commits into which the original commit from the
large repo got rewritten (in practice rewriting may have happened in the small
-> large direction, but it is not important for the purpose of this job). So it
establishes a mapping. Before this
diff, it actually established `Large<ChangesetId> ->
Option<(Small<ChangesetId>, Option<BookmarkName>)>` mapping, meaning that it
recorded into which bookmark large bookmark was rewritten. This was a useless
information (as evidenced by the fact that it was ignored by the
`prepare_entry` function, which turns `CommitEntryWithSmallReposMapped` into
`EntryPreparedForValidation`. It is useless because bookmarks are mutable and
it is impossible to do historic validation of the correctness of bookmark
renaming: bookmarks may have been correctly renamed when commits where pushes,
but they may be incorrectly renamed now and vice-versa. To deal with bookmarks,
we have a separate job, `bookmarks_validator`.
So this diff stops recording this useless information. As a bonus, this will
make migration onto `LiveCommitSyncConfig` easier.
Reviewed By: StanislavGlebik
Differential Revision: D22235389
fbshipit-source-id: c02b3f104a8cbd1aaf76100aa0930efeac475d42
Summary: We need to be able to query `synced_commit_mapping` to understand which `version_name` was used to sync commits. That `version_name` will be needed to produce `CommitSyncConfig` by utilizing upcoming `LiveCommitSyncConfig` APIs. And `CommitSyncConfig` is needed to create `CommitSyncer`. So let's extract this fn out of `CommitSyncer`, as it's an independent functionality really
Reviewed By: farnz
Differential Revision: D22244952
fbshipit-source-id: 53e55139efd423174176720c8bf7e3ecc0dcb0d7
Summary:
This diff migrates `backsyncer_cmd` (the thing that runs in the separate backsyncer job, as opposed to bakcsyncer, triggered from push-redirector) onto `LiveCommitSyncConfig`. Specifically, this means that on every iteration of the loop, which calls `backsync_latest` we reload `CommitSyncConfig` from configerator, build a new `CommitSyncer` from it, and then pass that `CommitSyncer` to `backsync_latest`.
One choice made here is to *not* create `CommitSyncer` on every iteration of the inner loop of `backsync_latest` and handle live configs outside. The reason for this is twofold:
- `backsync_latest` is called form `PushRedirector` methods, and `PushRedirector` is recreated on each `unbundle` using `LiveCommitSyncConfig`. That call provides an instance of `CommitSyncer` used to push-redirect a commit we want to backsync. It seems strictly incorrect to try and maybe use a different instance.
- because of some other consistency concerns (different jobs getting `CommitSyncConfig` updates at different times), any sync config change needs to go through the following loop:
- lock the repo
- land the change
- wait some time, until all the possible queues (x-repo sync and backsync) are drained
- unlock the repo
- this means that it's ok to have the config refreshed outside of `backsync_latest`
Reviewed By: farnz
Differential Revision: D22206992
fbshipit-source-id: 83206c3ebdcb2effad7b689597a4522f9fd8148a
Summary:
Before this diff only the main Mononoke server binary was able to use fs-based
`ConfigStore`, which is pretty useful in integration tests.
Reviewed By: farnz
Differential Revision: D22256618
fbshipit-source-id: 493a064a279250d01469c9ff7f747585581caf51
Summary: We designed the schema to make this simple to implement - it's literally a metadata read and a metadata write.
Reviewed By: ikostia
Differential Revision: D22233922
fbshipit-source-id: b392b4a3a23859c6106934f73ef60084cc4de62c
Summary:
Eventually, we want everything to be `async`/`await`; as a stepping stone in that direction, switch the remaining lobstore traits to new-style futures.
This just pushes the `.compat()` out to old-style futures, but it makes the move to non-'static lifetimes easier, as all the compile errors will relate to lifetime issues.
Reviewed By: krallin
Differential Revision: D22183228
fbshipit-source-id: 3fe3977f4469626f55cbf5636d17fff905039827
Summary:
This is to avoid passing `String` around. Will be useful in one of the next
diffs, where I add querying `LiveCommitSyncConfig` by versions.
Reviewed By: krallin
Differential Revision: D22243254
fbshipit-source-id: c3fa92b62ae32e06d7557ec486d211900ff3964f
Summary: I have previously moved the gitimport functionality (D22159880 (2cf5388835)) into a separate library, since repo_import shares similar behaviours. In this diff, I setup repo_import to be able to call gitimport to get the commits and changes. (Next steps include using Mover to set the paths of the files in the commits given by gitimport)
Reviewed By: StanislavGlebik
Differential Revision: D22233127
fbshipit-source-id: 4680c518943936f3e29d21c91a2bad60108e49dd
Summary:
Eventually, we want everything to be `async`/`await`; as a stepping stone in that direction, switch some of the blobstore interfaces to new-style `BoxFuture` with a `'static` lifetime.
This does not enable any fixes at this point, but does mean that `.compat()` moves to the places that need old-style futures instead of new. It also means that the work needed to make the transition fully complete is changed from a full conversion to new futures, to simply changing the lifetimes involved and fixing the resulting compile failures.
Reviewed By: krallin
Differential Revision: D22164315
fbshipit-source-id: dc655c36db4711d84d42d1e81b76e5dddd16f59d
Summary:
If the first client to send mutation data for a commit is only aware of partial
history for that commit, the primordial commit that is determined will be the
earliest of those commits. If another client comes along later with a longer
history, the new set of commits will be assigned a different primordial commit.
Make sure that when this happens, we still fetch the full history. We do this
by including the successor in the search-by-primordial case, which allows us
to join together disconnected histories at the cost of one extra round-trip to
the database.
Note that the fast path for addition of a single mutation will not fill in the
missing history. This is an acceptable trade-off for the faster performance
in the usual case.
Reviewed By: mitrandir77
Differential Revision: D22206317
fbshipit-source-id: 49141d38844d6cddc543b6388f0c31dbc70dcbc5
Summary:
By design, the mutation history of a commit should not have any cycles. However,
synthetic entries created by backfilling obsmarkers may inadvertently create
erroneous cycles, which must be correctly ignored by the mutation store.
The mutation store is affected by cycles in two ways:
* Self-referential entries (created by backfilling "revive" obsmarkers) must
be dropped early on, as these will overwrite any real mutation data for
that successor.
* Larger cycles will prevent determination of the primordial commit for
primordial optimization. Here we drop all entries that are part of the cycle.
These entries will not be shareable via the mutation store.
Note that it is still possible for cycles to form in the store if they are
added in multiple requests - the first request with a partial cycle will
allow determination of a primordial commit which is then used in subsequent
requests. That's ok, as client-side cycle detection will break the cycle in
these entries.
As we move away from history that has been backfilled from obsmarkers, this
will become less of a concern, as cycles in pure mutation data are impossible
to create.
Reviewed By: mitrandir77
Differential Revision: D22206318
fbshipit-source-id: a57f30a19c482c7cde01cbd26deac53b7bb5973f
Summary:
Push supported multiple bookmarks in theory, but in practice we never used it.
Since we want to start logging pushed commits in the next diffs we need to decide what to do with
bookmarks, since at the moment we can log only a single bookmark to scribe
let's just allow a single bookmark push
Reviewed By: farnz
Differential Revision: D22212674
fbshipit-source-id: 8191ee26337445ce2ef43adf1a6ded3e3832cc97
Summary:
In the next diffs it will be passed to unbundle processing so that we can use
scribe category to log pushed commits
Reviewed By: krallin
Differential Revision: D22212616
fbshipit-source-id: 17552bda11f102041a043f810125dc381e478611
Summary: That was like 50% of the point of this change, and somehow I forgot to do it.
Reviewed By: farnz
Differential Revision: D22231923
fbshipit-source-id: 4a4daaeaa844acd219680907c0b5a5fdacdf535c
Summary:
Similarly to how we have `PushRedirectorArgs`, we need `CommitSyncerArgs`: a struct, which a long-living process can own and periodically create a real `CommitSyncer` out of it, by consuming freshly reloaded `CommitSyncConfig`.
It is a little unfortunate that I am introducing yet another struct to `commit_rewriting/cross_repo_sync`, as it's already pretty confusing with `CommitSyncer` and `CommitSyncRepos`, but hopefully `CommitSyncerArgs`'s purpose is simple enough that it can be inferred from the name. Note that this struct does have a few convenience methods, as we need to access things like `target_repo` and various repo ids before we even create a real `CommitSyncer`. This makes it's purpose a little less singular, but still fine IMO.
Reviewed By: StanislavGlebik
Differential Revision: D22197123
fbshipit-source-id: e2d993e186075e33acec00200d2aab10fb893ffd
Summary:
This fn is not used anywhere except tests, and its only difference from
`backsync_all_latest` is in the fact that it accepts a limit. So let's rename
`backsync_all_latest` into `backsync_latest` and make it accept a limit arg.
I decided to use a custom enum instead of `Option` so that people don't have to
open fn definition to understand what `BacksyncLimit::Limit(2)` or
`BacksyncLimit::NoLimit` mean.
Reviewed By: StanislavGlebik
Differential Revision: D22187118
fbshipit-source-id: 6bd97bd6e6f3776e46c6031f775739ca6788ec8c
Summary:
This diff enables `unbundle` flow to start creating `push_redirector` structs from hot-reloaded `CommitSyncConfig` (by using the `LiveCommitSyncConfig` struct).
Using `LiveCommitSyncConfig` unfortunately means that we need to make sure those tests, which don't use standard fixtures, need to have both the `.toml` and the `.json` commit sync configs present, which is a little verbose. But it's not too horrible.
Reviewed By: StanislavGlebik
Differential Revision: D21962960
fbshipit-source-id: d355210b5dac50d1b3ad277f99af5bab56c9b62e
Summary:
`LiveCommitSyncConfig` is intended to be a fundamental struct, on which live push-redirection and commit sync config for push-redurector, x-repo sync job, backsyncer, commit and bookmark validators are based.
The struct wraps a few `ConfigStore` handles, which allows it to query latest values every time one of the public methods is called. Callers receive parsed structs/values (`true`/`false` for push redirection config, `CommitSyncConfig` for the rest), which they later need to use to build things like `Mover`, `BookmarkRenamer`, `CommitSyncer`, `CommitRepos` and so on. For now the idea is to rebuild these derived structs every time, but we can later add a memoization layer, if the overhead is going to be large.
Reviewed By: StanislavGlebik
Differential Revision: D22095975
fbshipit-source-id: 58e1f1d8effe921b0dc264fffa785593ef188665
Summary:
This diff does three things:
- moves existing `CommitSyncConfig` validation from `config.rs` into
`convert/commit_sync.rs`, so that any user of `impl Convert for
RawCommitSyncConfig` gets it for free
- adds another thing to validate `CommitSyncConfig` against (large repo is one
of the small repos)
- adds `RawCommitSyncConfig` validation for something that can be lost when
looking at `CommitSyncConfig` (no duplicate small repos).
Reviewed By: markbt
Differential Revision: D22211897
fbshipit-source-id: a9820cc8baf427da66ce7dfc943e25eb67e1fd6e
Summary: When running integration tests we should make the paths absolute, but kept it relative so far. This results it breaking the tests.
Reviewed By: krallin
Differential Revision: D22209498
fbshipit-source-id: 54ca3def84abf313db32aecfac503c7a42ed6576
Summary:
If we don't read the body for a response, then Hyper cannot return the
connection to the pool. So, let's do it automatically upon dropping. This will
typically happen when we send a request to upstream then don't read the
response.
I seem to remember this used to work fine at some point, but looking at the
code I think it's actually broken now and we don't reuse upstream connections
if we skip waiting for upstream in a batch request. So, let's fix it once and
for all with a more robust abstraction.
Reviewed By: HarveyHunt
Differential Revision: D22206742
fbshipit-source-id: 2da1c008556e1d964c1cc337d58f06f8d691a916
Summary:
This was old Tokio 0.1 code that needed channels for spawns, but in 0.2 that
actually is built-in to tokio::spawn, so let's use this.
Reviewed By: HarveyHunt
Differential Revision: D22206738
fbshipit-source-id: 8f89ca4f7afc8dd888fe289e8d597148976cc54c
Summary:
This fixes a bit of a tech debt item in the LFS Server. We've had this
discard_stream functon for a while, which was necessary because if you just
drop the data stream, you get an error on the sending end.
This makes the code more complex than it needs to be, since you need to always
explicitly discard data streams you don't want instead of just dropping them.
This fixes that by letting us support a sender that tolerates the receiver
being closed, and just ignores those errors.
Reviewed By: HarveyHunt
Differential Revision: D22206739
fbshipit-source-id: d209679b20a3724bcd2e082ebd0d2ce10e9ac481
Summary:
We have a lot of integration tests for LFS, but a handful of unit tests don't
hurt for some simpler changes. Let's make it easier to write those.
Reviewed By: HarveyHunt
Differential Revision: D22206741
fbshipit-source-id: abcb73b35c01f28dd54cc543cd0a746327d3787b
Summary:
This diff is probably going to sound weird ... but xavierd and I both think
this is the best approach for where we are right now. Here is why this is
necessary.
Consider the following scenario
- A client creates a LFS object. They upload it to Mononoke LFS, but not
upstream.
- The client shares this (e.g. with Sandcastle), and includes a LFS pointer.
- The client tries to push this commit
When this happens, the client might not actually have the object locally.
Indeed, the only pieces of data the client is guaranteed to have is
locally-authored data.
Even if the client does have the blob, that's going to be in the hgcache, and
uploading from the hgcache is a bit sketchy (because, well, it's a cache, so
it's not like it's normally guaranteed to just hold data there for us to push
it to the server).
The problem boils down to a mismatch of assumptions between client and server:
- The client assumes that if the data wasn't locally authored, then the server
must have it, and will never request this piece of data again.
- The server assumes that if the client offers a blob for upload, it can
request this blob from the client (and the client will send it).
Those assumptions are obviously not compatible, since we can serve
not-locally-authored data from LFS and yet want the client to upload it, either
because it is missing in upstream or locally.
This leaves us with a few options:
- Upload from the hg cache. As noted above, this isn't desirable, because the
data might not be there to begin with! Populating the cache on demand (from
the server) just to push data back to the server would be quite messy.
- Skip the upload entirely, either by having the server not request the upload
if the data is missing, by having the server report that the upload is
optional, or by having the client not offer LFS blobs it doens't have to the
server, or finally by having the client simply disobey the server if it
doesn't have the data the server is asking for.
So, why can we not just skip the upload? The answer is: for the same reason we
upload to upstream to begin with. Consider the following scenario:
- Misconfigured client produces a commit, and upload it to upstream.
- Misconfigured client shares the commit with Sandcastle, and includes a LFS
pointer.
- Sandcastle wants to push to master, so it goes to check if the blob is
present in LFS. It isn't (Mononoke LFS checks both upstream and internal, and
only finds the blob in upstream, so it requests that the client submit the
blob), but it's also not not locally authored, so we skip the push.
- The client tries to push to Mononoke
This push will fail, because it'll reference LFS data that is not present in
Mononoke (it's only in upstream).
As for how we fix this: the key guarantee made by our proxying mechanism is
that if you write to either LFS server, your data is readable in both (the way
we do this is that if you write to Mononoke LFS, we write it to upstream too,
and if you write to upstream, we can read it from Mononoke LFS too).
What does not matter there is where the data came from. So, when the client
uploads, we simply let it submit a zero-length blob, and if so, we take that to
mean that the client doesn't think it authored data (and thinks we have it), so
we try to figure out where the blob is on the server side.
Reviewed By: xavierd
Differential Revision: D22192005
fbshipit-source-id: bf67e33e2b7114dfa26d356f373b407f2d00dc70
Summary:
Due to Thrift design of "include" statements in fbcode the thrift structures has to be contained in folders that are identical to the folder layout inside fbcode.
This diff changes the folder layout on Cargp.toml files and in fbcode_builder, there will be a next diff that changes this for ShipIt as well.
Reviewed By: ikostia
Differential Revision: D22208707
fbshipit-source-id: 65f9cafed2f0fcc8398a3887dfa622de9e139f68
Summary:
If a commit changes modes (i.e. executable, symlink or regular) of a lot of files but
doesn't change their content then we don't need to put these filenodes to the
generated bundle. Mercurial stores mode in manifest, so changing the mode
doesn't change the filenode.
Reviewed By: ikostia
Differential Revision: D22206736
fbshipit-source-id: f64ee8a34281cd207c92653b927bf9109ccbe1b4
Summary:
I landed D22118926 (e288354caf) yesterday expecting those messages at about the same time
xavierd landed D21987918 (4d13ce1bcc), which removed them. This removes them from the
tests.
Reviewed By: StanislavGlebik
Differential Revision: D22204980
fbshipit-source-id: 6b1d696c93a07e942f86cd8df9a8e43037688728
Summary:
The Rust store code has been enabled everywhere for a few weeks now, let's
enable it by default in the code. Future changes will remove the config as well
as all the code associated with the non Rust store code.
The various tests changes are due to small difference between the Rust code and
the Python one, the biggest one being it's handling of corrupted packfiles. The
old code silently ignored them, while the new one errors out for local
packfiles. The test-lfs-bundle.t difference is just due to an ordering
difference between Python and Rust.
Reviewed By: kulshrax
Differential Revision: D21985744
fbshipit-source-id: 10410560193476bc303a72e7583f84924a6de820
Summary: Let's not heal 10000 blobs in parallel, that's a little too much data.
Reviewed By: farnz
Differential Revision: D22186543
fbshipit-source-id: 939fb5bc83b283090e979ac5fe3efc96191826d3
Summary:
If we're going to iterate through the whole manifest, we should probably
prefetch it. Otherwise, we might end up doing a whole lot of sequential
fetching. We saw this this week when a change landed in sparse profiles that
caused requests to Mononoke to increase 100-fold.
Unfortunately, I don't think we can selectively only fetch the things we are
missing, so this just goes ahead and fetches everything unconditionally. If
there is a better way to do this, I'm all ears.
Reviewed By: StanislavGlebik, xavierd
Differential Revision: D22118926
fbshipit-source-id: f809fa48a7ff7b449866b42b247bf1da30097caa
Summary: This got broken in D22115015 — this fixes it.
Reviewed By: farnz
Differential Revision: D22186138
fbshipit-source-id: 54c05466cdbd3be4f6887a852f099351ea5e891e
Summary: This will be used in the following diffs. It just adds commitsync fixtures in a single place, so that we can later play with them in integration tests.
Reviewed By: StanislavGlebik
Differential Revision: D21952665
fbshipit-source-id: 2933a9f7ea8343d5d52e6c3207e7d78a3ef0be25
Summary: `HgPhase` type is redundant and was adding dependency on mercurial in phases crate.
Reviewed By: farnz
Differential Revision: D22162716
fbshipit-source-id: 1c21841d34897d0072ff6fe5e4ac89adddeb3c68
Summary:
During expensive getbundle request traversing skiplist uses a lot of cpu, and
in fact it's blocking the whole CPU. krallin suggested to yield since it
should avoid blocking cpus, and that's what this diff is doing.
Reviewed By: krallin
Differential Revision: D22160477
fbshipit-source-id: 5bd881d5c50f4d8e64f2cc90389abc8568ec1df6
Summary: DangerousOverride is moved into a separate crate. Not only it is usually not needed but it was introducing dependencies on mercurial crate.
Reviewed By: StanislavGlebik
Differential Revision: D22115015
fbshipit-source-id: c9646896f906ea54d11aa83a8fbd8490a5b115ea
Summary: This change will ensure that cloning blobrepo is cheap, even if someone adds field that is expensive to clone. Plus it will result in just one arc-clone instead of cloning all the fields one by one.
Reviewed By: mitrandir77
Differential Revision: D22114066
fbshipit-source-id: ca0c3c78033b4c74872da314a32deb37c05b70ca
Summary: Globalrev does not have any dependencies on mercurial so it can be moved to mononoke_types since it is used in BlobRepo
Reviewed By: StanislavGlebik
Differential Revision: D22092491
fbshipit-source-id: 1dded88eb2ace08e8c6c3673e2d50ae1fbb9850d
Summary: Move all mercurial changeset generation logic to `blobrepo_hg`. This is preliminary step is required to decouples BlobRepo from mercurial, and in later stages it will be moved to derived data infra once blobrepo is free of mercurial.
Reviewed By: StanislavGlebik
Differential Revision: D22089677
fbshipit-source-id: bca28dedda499f80899e729e4142e373d8bec0b8
Summary: move HgMutationStore to attributes, and all related methods to BlobRepoHg
Reviewed By: StanislavGlebik
Differential Revision: D22089657
fbshipit-source-id: 8fe87418ccb8a7ad43828758844bdbd73dc0573d
Summary: Move `Filenodes` to `BlobRepo::attributes` as it is mercurial specific.
Reviewed By: ikostia
Differential Revision: D21662418
fbshipit-source-id: 87648a3e6fd7382437424df3ee60e1e582b6b958
Summary: This diff introduces `BlobRepoHg` extension trait for `BlobRepo` object. Which contains mercurial specific methods that were previously part of `BlobRepo`. This diff also stars moving some of the methods from BlobRepo to BlobRepoHg.
Reviewed By: ikostia
Differential Revision: D21659867
fbshipit-source-id: 1af992915a776f6f6e49b03e4156151741b2fca2
Summary:
This diff adds additional filed `BlobRepo::attributes` which can store attributes of arbitrary type. This will help store opaque types inside blobrepo without creating dependency on a crate which contains type definition for this attribute. This diff also moves `BonsaiHgMapping` inside attributes set.
- This work will allow to move mercurial changeset generation logic to derive data infrastructure
Reviewed By: ikostia
Differential Revision: D21640438
fbshipit-source-id: 3abd912e7227738a73ea9b17aabdda72a33059aa
Summary: Not all facebook-specific code was moved out of integration_runner_real.py, but removing part of the code that is left would made the code less readable, the rest of it will be removed while the integration_runner_real.py is made usable for OSS
Reviewed By: farnz
Differential Revision: D22114948
fbshipit-source-id: d9c532a6a9ea653de2b12cffc92fbf45826dad37
