Skip to content
Snippets Groups Projects
  1. Apr 13, 2023
    • Georges Racinet's avatar
      RHGitaly: `GitCommit` message building helper · 6b7c31ba
      Georges Racinet authored
      This parallels the Python implementation: returning a `GitCommit`, this
      time from a `ChangelogEntry` instead of Python `changectx` will be
      a common need.
      
      The user line parsing is implemented to match the Python implementation
      (module `stringutil`). We considered using a regular expression, but it
      is not so obvious to obtain the exact same result (actually first experiments
      where splitting the brackets with a regexp for quick validation of the
      first gRPC method that will use this).
      
      User line and timestamp parsing should find eventually their way into
      `hg-core`.
      6b7c31ba
    • Georges Racinet's avatar
      RHGitaly: helpers for server streaming gRPC methods involving repositories · 33189bb5
      Georges Racinet authored
      All future implementations of streaming methods with repositories will
      follow the same pattern, hence we provide a generic helper right away,
      taking care notably of repetitive error treatment (sending errors etc.).
      
      Detailed explanations of the constraints and how it will work are provided
      in the doc-comment of the `load_repo_and_stream` function. Concrete gRPC
      method implementations will consist mostly of calling it with the closure
      doing the actual work on the loaded repository.
      33189bb5
    • Georges Racinet's avatar
      RHGitaly: separation of repo errors into specification and load errors · a273ee46
      Georges Racinet authored
      As the Gitaly Comparison tests showed us in the Python case, the errors
      to return on wrong `Repository` specifications depend at least on the service,
      hence we need an added flexibility.
      
      For that, we need internally to make a difference between client-side errors
      (e.g., repo does not exist) and internal errors (failed to load repo, e.g., due
      to a filesystem issue).
      a273ee46
  2. Apr 12, 2023
  3. Mar 24, 2023
  4. Apr 02, 2023
    • Georges Racinet's avatar
      CI/CD: Rust lints (rustfmt, clippy and line widths) · f4e5c429
      Georges Racinet authored
      As we don't want to resort ot unstable features of rustfmt, we
      wrote a simple checker in Python.
      
      Also we have to run after the tests, because
      we need the protocol source files (e.g., `gitaly.rs` to be present).
      Without them, `cargo fmt` fails at the `use` statement.
      f4e5c429
    • Georges Racinet's avatar
      RHGitaly: fixed Clippy warnings · 555e544f
      Georges Racinet authored
      Some warnings have been fixed lower in the stack. However, the
      Camel Case rule for acronyms hurts my eyes in the case of TCP
      (and leads to plain wrong output when it ends up in startup logs
      through `fmt::Debug`).
      
      We had to change the way the generated code is included. This also
      has the nice effect of ignoring `gitaly.rs` in `rustfmt`, thus making
      `cargo fmt --check` work for us.
      
      The missing `Eq` impl when deriving `PartialEq` is new in Rust 1.63
      555e544f
  5. Mar 26, 2023
  6. Mar 23, 2023
    • Georges Racinet's avatar
      RHGitaly Comparison tests: comparison with HGitaly for ServerInfo · c7d863a5
      Georges Racinet authored
      We're defining a new fixture and `RpcHelper` for such comparisons.
      As expected, this is  much simpler than the one for Gitaly Comparison
      tests.
      
      The included test is actually useful: we are sure that we don't
      have version skew between RHGitaly and HGitaly. Given how it is
      implemented, this is not a surprise, but it'll have our back in
      case the implementation has to change.
      c7d863a5
    • Georges Racinet's avatar
      Comparison tests: cut off BaseRpcHelper from RpcHelper · 42fb2ee5
      Georges Racinet authored
      This new base class does not carry the specificities of
      the Gitaly comparison, but still has feature flags, default args,
      and calling primitives. It will be useful to introduce Comparison
      tests between HGitaly and RHGitaly with room for growth.
      42fb2ee5
    • Georges Racinet's avatar
      RHGitaly: proper version information · bd9e9d77
      Georges Racinet authored
      It was nice to use the purely stateless `ServerInfo` gRPC method
      (at least with the info that HGitaly provides) as a first example
      of an implementation using Tonic. Now it's time to provide the
      actual information, consistent with the Python implementation.
      
      We're using the `build_const` crate to inject the value at build time.
      This is actually importing from the `constants.rs` file in the
      relevant  build `out/` directory.
      
      Also the `..Default::default()` syntax (taught us by clippy) allows
      to avoid starting from the defaults to mutate a few fields.
      bd9e9d77
    • Georges Racinet's avatar
      CI/CD RHGitaly: using artifacts and cache · 9be2e5b3
      Georges Racinet authored
      This artifacts built by `tests-rust` will serve as precompilation
      for gitaly-comparison.
      
      For the cache, we follow the instructions of
      https://doc.rust-lang.org/nightly/cargo/guide/cargo-home.html#caching-the-cargo-home-in-ci
      but still exclude installed binaries information
      
      Hopefully, this will speed up the build a bit.
      9be2e5b3
    • Georges Racinet's avatar
      CI/CD: running pure Rust tests · 862e93be
      Georges Racinet authored
      A simple `cargo test`.
      
      Because of the dependencies, we had to provide a new base image
      for this (we could have used heptapod-gitaly, but it is unsatisfactory
      to rely on a huge image when much less is needed).
      862e93be
  7. Mar 19, 2023
    • Georges Racinet's avatar
      RHGitaly: Gitaly comparison tests · 69cb7195
      Georges Racinet authored
      We chose to compare RHGitaly with Gitaly rather than with HGitaly
      because it's not impossible that we stop at some point maintaining
      some methods in the Python code (time will tell).
      
      It is likely that we'll end up implementing some of the HGitaly-specific
      methods in RHGitaly (e.g. reading phases, topics and whatnot). In that
      case we'll need comparison between HGitaly and RHGitaly, but that
      will be some much simpler (no correspondence of Git content) that we
      will provide it directly.
      
      This fully hooked in CI/CD. We had to anyway: otherwise the new test would
      not pass.
      
      Implementation details
      ----------------------
      
      We add the capability to start RHGitaly, with a simple `cargo run`.
      This is validated independently by `test_rhgitaly_server` (homologous
      to `test_gitaly_server`).
      
      The `GitalyComparison` class (of which the `gitaly_comparison` fixture
      is an instance) now gains an optional `rhgitaly_channel`, which is set
      and exposed in the new `gitaly_rhgitaly_comparison` fixture.
      
      Finally, using `rhgitaly_channel` rather than `hgitaly_channel` is
      left to the `RPCHelper` class, with its `hg_server` argument.
      Since it is instantiated from inside the tests, it is easy to
      pass `hg_server` down from a parametrization, reaching the end goal: the
      very same test is run for HGitaly and RHGitaly, compared in both
      cases with Gitaly. This is illustrated in the test for `RepositoryExists`.
      
      Two annoyances had to be taken care of:
      
      - we need to ignore the `rust/` directory in `run-all-tests` because
        it makes pytest collection crazy (hangs forever) due at least in part
        to Mercurial full source being present by default in `rust/dependencies`
      - Tonic seems to add trailing metadata that Gitaly does not. We had
        to remove them in `assert_compare_errors()`.
      
      CI/CD assumptions
      -----------------
      
      The base image `heptapod-gitaly` is now expected to provide
      - the Rust toolchain
      - a clone of Mercurial sources. It can be daily or weekly, we
        make a share and update it
      - `rust-protobuf`: could be trimmed, as we only need
        the protocol files and don't need to update them for each build
      69cb7195
  8. Mar 18, 2023
    • Georges Racinet's avatar
      RHGitaly: configurable listen URL (TCP or Unix Domain) · d6853d85
      Georges Racinet authored
      We keep on configuring through environment variables, in order to
      focus on the task at hand: stop hardoding a TCP address and
      support Unix Domain sockets. The latter are the natural choice
      for integration tests, such as the Gitaly Comparison tests,
      as they come with strong guarantees not to be already in use.
      d6853d85
  9. Mar 17, 2023
    • Georges Racinet's avatar
      RHGitaly RepositoryExists implementation · 775265f6
      Georges Racinet authored
      Similarly to the Python implementation, this introduces a top level
      module to take care of logic that will be useful for several methods.
      
      These top-level modules don't know about actual method or services,
      but they handle protobuf messages, `Status` errors etc.
      
      Like the Python implementation, we support only one storage (named `default`)
      and configuration does its repositories root.
      775265f6
  10. Mar 28, 2023
    • Georges Racinet's avatar
      RHGitaly: logging with the tracing crate · 1bd1f685
      Georges Racinet authored
      This removes the (blocking) `println!` in favour of the standard `tracing`
      crate, which is precisely meant for asynchronous code.
      
      For now, we're exclusively logging to `stdout`, at the `INFO` level.
      Configurability will appear later.
      The `DEBUG` level is very verbose, as it includes lots of events from
      the tonic/h2 etc lower layers. These will certainly be interesting, yet
      not at this stage of developpement.
      
      Quoting from current documentation:
      
        The #[instrument] attribute provides an easy way to add tracing spans to
        functions. A function annotated with #[instrument] will create and enter a
        span with that function’s name every time the function is called, with
        arguments to that function will be recorded as fields using `fmt::Debug`.
      
      Hence we don't need to include the passed `request` in the call to `info!`.
      This also raises the question of the laziness of formatting, something that
      we will have to check later.
      
      Finally, Tonic's `Request` wrapper does include all invocation metadata, which
      is great to have `correlation_id` but won't be acceptable when we implement
      authentication, as it will also probably have the related secrets. It could
      be a simple matter of using an appropriate subscriber. We'll see.
      1bd1f685
  11. Mar 17, 2023
    • Georges Racinet's avatar
      RHGitaly: first service method · 9c1dd8a0
      Georges Racinet authored
      We implement the `ServerInfo` gRPC.
      
      This is still very much a PoC: hardcoded address, no config or any kind
      of shared state, no proper (async) logging and the `server_info` value
      is not even valid (yet helpful to distinguish with other attempts on the
      same workstation).
      
      It is a of course unfortunate for an example that the simplest service that
      we can implement is `ServerService`. This leads us to niceties such
      as `ServerServiceServer`. Compare to the HelloWorld tutorial where
      the service is called `Greeter` and leads to a `GreaterServer` module.
      
      We decided to provide the `server_server` factory function to keep
      imports from generated code out of `main.rs`. Subsequent ones will
      be clearer: `commit_server` etc.
      9c1dd8a0
  12. Mar 18, 2023
  13. Mar 17, 2023
    • Georges Racinet's avatar
      RHGitaly: initialized Cargo workspace and crate · 61d9b987
      Georges Racinet authored
      The idea is that we'll maintain a full clone of the Mercurial
      sources as `rust/dependencies/mercurial`, and the symlink
      will take the `hg-core` crate and anchor it to the workspace for
      RHGitaly, instead of the one from Mercurial sources.
      
      Because of this, we need to exclude the `rust` directory in
      `run-all-tests`. Otherwise, it makes pytest collection go crazy
      and hang forever.
      
      Also, we're versioning `Cargo.lock` because `rhgitaly` is a binary crate.
      61d9b987
  14. Apr 12, 2023
  15. Mar 31, 2023
  16. Mar 23, 2023
    • Georges Racinet's avatar
      Merged / adapted error treatment and Gitaly Comparison tests from stable · b872a3a9
      Georges Racinet authored
      It turns out that `CommitService` does not return the `INTERNAL`
      gRPC error any more in Gitaly 15.5 when storage is incorrect.
      Now that is `INVALID_ARGUMENT`, same as with other gRPC methods.
      b872a3a9
    • Georges Racinet's avatar
      Blob/Tree Gitaly Comparison test: switch to RpcHelper · 026fbefd
      Georges Racinet authored
      The tests for `TreeEntry` and `GetTreeEntries` predate the
      introduction of the `RpcHelper` class by a few months, and were
      probably important in defining what we wanted to mutualize.
      
      Switching these tests to `RpcHelper` makes them directly applicable to
      `RHGitaly` when the time comes. The Blob/Tree methods are natural
      candidates for oxidation, as it is obvious that they are too slow
      in the Python implementation.
      
      We had to overcome two challenges for this:
      
      - Gitaly chunking being a bit non-deterministic in `TreeEntry`,
        we needed a way to compare streaming requests without comparing
        response by response. This now done by `RpcHelper.assert_compare_aggregated`,
        which takes care of the small data fields (`oid` and `mode` in the case
        of `TreeEntry`) with the common Gitaly pattern of providing them in
        the first response only. Two new hooks are to be registered in `RpcHelper`
        for this: one for the concatenation, one to remove the main data so that
        the small fields can be compared.
      - To take pagination into account, we needed a way to expose the first
        response of each backend as they were before normalization removed
        the cursor information (which is backend-specific). Since the pagination
        subprotocol is meant to be used in several streaming gRPC methods,
        we'll probably provide this in `RpcHelper` itself in a second move.
      
      In the process we're doing a more thorough job of comparing the `oid`
      response fields, thanks to a normalizer that knows the mapping between
      HGitaly and Gitaly OIDs.
      026fbefd
  17. Mar 22, 2023
    • Georges Racinet's avatar
      CommitService.TreeEntry: fix discrepancy with Gitaly · eb5ffde1
      Georges Racinet authored
      We had a Gitaly Comparison test, but it was actually comparing
      HGitaly with itself (!), see the change in `assert_compare_tree_entry()`.
      
      Then of course, whether it was initially correct or not,
      we had discrepancies to fix:
      
      - a gRPC `NOT_FOUND` is expected for unknown paths and revisions
      - `.` is not a valid path *for this method*
      - chunking in Gitaly happens in chunks of 16 kiB, but it's not
        consistent: sometimes the last chunk can be up to 16 kiB + 1023B
        long, sometimes, we get exact chunks. It's not clear in Gitaly
        code base exactly what happens: added logs in `streamio.go` showed
        us that this is what gets passed to it (not recut, then, since
        it's meant to cut at 128kiB).
        For these reasons, we gave up on comparing chunk by chunk and
        test the metadata only.
      eb5ffde1
    • Georges Racinet's avatar
      CI/CD: hg-current run without Gitaly; comparison pushed to compat stage · 11825d33
      Georges Racinet authored
      Since we don't currently run tests for the `stable` and `default`
      Mercurial/Evolve revisions (see 1c187a5a5a0c), we are at risk of
      losing coverage without the Gitaly Comparison tests. Hence we
      introduce the `tests-hg-current` job.
      
      Because the `gitaly-comparison` job (new name) will become heavier with the
      introduction of RHGitaly, we're pushing it to the compat stage.
      We could also just run `tests_with_gitaly` and don't bother with
      coverage (since it is done in a harder way by `tests-hg-current`).
      11825d33
    • Georges Racinet's avatar
      CI/CD: comment was way outdated · 46645450
      Georges Racinet authored
      46645450
    • Georges Racinet's avatar
      Commit service: correct errors for unknown storage and empty repo arg · f8871512
      Georges Racinet authored
      Turns out Gitaly issues an `INTERNAL` error in that case. Probably not
      intended, but we're ready for further changes down the road.
      f8871512
    • Georges Racinet's avatar
      RefService: Gitaly Comparison test for missing repository arg · d2dc0502
      Georges Racinet authored
      It passes right away in that case.
      d2dc0502
    • Georges Racinet's avatar
      Missing repository argument: proper error codes. · 9d403f00
      Georges Racinet authored
      As it was uncovered by experimental implementation in RHGitaly of
      `RepositoryExists`, it's possible that the `repository` argument
      is entirely missing. In that case, we are now raising `ValueError`
      internally, and convert it to the gRPC code
      `HGitalyServicer.STATUS_CODE_STORAGE_NOT_FOUND`.
      9d403f00
    • Georges Racinet's avatar
      servicer.load_repo: completed error treatment. · 215369c1
      Georges Racinet authored
      Up to know, we were thinking that raising `KeyError` and setting
      code and details on the context was enough, but apparently, it
      is at least problematic with streaming gRPC methods.
      
      We're instead using the two-layer organisation that was suggested
      as comment in `service.repository`. As it turns out, the error
      code to set in case of unknown storage depends at least on the
      service. This was already visible in the error treatment of the
      full path methods of `RepositoryService`, which does not need
      to be specific any more.
      
      The new `STATUS_CODE_*`class attributes make it easy to tweak the
      error code in service implementations. Specific methods could override
      them with instance attributes if needed.
      
      In the process, we're basically giving up on providing the same
      `details` as Gitaly: these are highly dependent on implementation
      (it's just stderr if the implementation relies on a
      `git` subprocess) and chances that it may matter are low, given
      that GitLab these days pushes towards "structured errors", i.e. with
      programmatically usable details provided in metadata.
      As a result, the normalization of error details in `assert_compare_errors`
      is not used any more. We're keeping it in the code base, as it may
      turn useful for forthcoming tests were the details would not be a
      simple stderr echo.
      215369c1
  18. Mar 15, 2023
Loading