Skip to content
Snippets Groups Projects
  1. Apr 12, 2023
  2. Mar 31, 2023
  3. 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
  4. 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
  5. Mar 15, 2023
  6. Mar 13, 2023
  7. Mar 12, 2023
  8. Mar 10, 2023
  9. Mar 08, 2023
  10. Feb 20, 2023
    • Georges Racinet's avatar
      Moving interceptors to hgitaly.service · 826957b9
      Georges Racinet authored
      The end goal is to allow configuring logging in one shot, both for
      sporadic logs from service methods and the systematic request logging
      done in interceptors. Such loags require to tie a format string
      using `correlation_id` to the relevant loggers, and can now be done
      for the `hgitaly.service` only.
      
      Of course it is lacking in consistency, as interceptors are not
      services. Hence the docstring proposing to introduce an intermediate
      subpackage. That would be a chore for another day, though.
      826957b9
    • Georges Racinet's avatar
      Sytematic requests logging bumped to INFO · 86ad9098
      Georges Racinet authored
      This will now be our primary source of informatiopn, with
      the supported `correlation_id`, provided the format string uses is.
      
      Converserly, logging the loading operation on repos themselves is
      generally speaking redundant, being useful only to investigate problems
      before the start of a request and the related load, hence we demote it
      to `DEBUG`
      86ad9098
    • Georges Racinet's avatar
      hgitaly.service: logging correlation_id systematically. · f70b3c51
      Georges Racinet authored
      More generally, we make use of the logger adapter that leverages
      information from the gRPC context in the whole `hgitaly.service`
      package, and we won't from other packages.
      
      The intention and the constraints that lead to it are explained
      in the new REAdME section.
      f70b3c51
    • Georges Racinet's avatar
      hgitaly.logging: new module to log correlation_id · 1d29e1b9
      Georges Racinet authored
      This provide san adapter to extract `correlation_id` from
      the gRPC context and add it to the logging `extra`, so
      that it may be used in a format, e.g.:
      `[%(asctime)s] [%(correlation_id)s] [%(levelname)s] [%(name)s] %(message)s`
      
      with the drawback that all loggers using this format will have to use
      the adapter.
      
      Minimal usage is added (`RequestLoggerInterceptor`) and support in
      `Service` fixture to get coverage with actual `ServicerContext` objects.
      
      Closes #122
      1d29e1b9
    • Georges Racinet's avatar
      c5a8ff21
    • Georges Racinet's avatar
      Logs: remove redundant · 6944f894
      Georges Racinet authored
      These were remnant of times when we didn't have the systematic
      request logging interceptor.
      6944f894
Loading