Skip to content
Snippets Groups Projects
This project is mirrored from https://gitlab.com/gitlab-org/gitaly.git. Pull mirroring updated .
  1. Jun 07, 2022
  2. Jun 06, 2022
  3. Jun 05, 2022
  4. Jun 03, 2022
    • Patrick Steinhardt's avatar
      Merge branch 'pks-catfile-request-queue-isdirty-deadlock' into 'master' · a31bd1be
      Patrick Steinhardt authored
      catfile: Fix deadlock between reading a new object and accessing it
      
      See merge request gitlab-org/gitaly!4590
      a31bd1be
    • Patrick Steinhardt's avatar
      catfile: Keep request queue dirty if reading object info fails · 49ccf030
      Patrick Steinhardt authored
      When reading the object info fails we're currently returning the request
      queue back to the state where it pretends to not be reading an object
      anymore. This means that it is now not considered dirty anymore and may
      be used for additional requests, or even be returned to the catfile
      cache. This is dangerous though: we do not know why reading the object
      info has failed, and chances are high that the error is not recoverable.
      And in that case we certainly don't want to return the catfile process
      to the cache.
      
      Fix this isssue by not unsetting `isReadingObject` when `ReadObject()`
      fails. The only exception is when we got a `NotFound` error, which is a
      graceful failure and doesn't dirty the git-cat-file(1) process.
      49ccf030
    • Sami Hiltunen's avatar
      Simplify request queue dirtiness tracking · 18f838be
      Sami Hiltunen authored
      The dirtiness tracking  in request queue is somewhat hard to understand
      as it bounces between multiple types. The Object type is tracking
      the number of bytes read. The queue retains a reference to the latest
      read object to check whether it has been fully read or not. This back
      and forth can be simplified by managing the dirtiness state completely
      in the queue. This commit simplifies the tracking by marking the queue
      dirty when an object is returned, and composing the readers in a manner
      that flicks the dirtiness bit off once the reading has been completed.
      This moves all of the dirtiness logic to the queue, making it easier to
      understand.
      
      The object is reading from the stdout of a cat-file process. Closing the
      cat-file process also closes the underlying reader the Object is using.
      There's no need to have a separate closing mechanism for the Object so
      the close and isClosed is removed as part of this change. They were only
      called from the queue and it is no longer tracking the currentObject so
      it can't close the object separately.
      
      The behavior to return os.ErrClosed on reading when the queue is closed is
      still retained as some tests rely on it. A later commit can adjust the tests
      to remove the assertions and the behavior. The tests incorrectly assert
      that nothing could be read after the process is closed. The io is buffered
      and it is possible that something can still be read from the buffer even after
      the underlying process is already closed.
      18f838be
    • John Cai's avatar
      Merge branch 'wc-capture-panic' into 'master' · 7c2fcde2
      John Cai authored
      ci: Capture panic stack traces
      
      See merge request gitlab-org/gitaly!4593
      7c2fcde2
  5. Jun 01, 2022
  6. May 31, 2022
    • Will Chandler's avatar
      ci: Capture panic stack traces · eaf3df45
      Will Chandler authored
      The `short` formatting option for `gotestsum` will usually suppress
      panics and their stack traces, making it difficult to understand why a
      panicking test failed.
      
      Using `standard-verbose` will display the full trace, but this also
      gives us the full output of `go test -v`, removing the readability
      improvements that `gotestsum` provides.
      
      To work around this, we use the `--jsonfile` option to write out the
      full output of `go test -json`, then search for panics in an
      `after_script` section with results in a pre-collapsed section.
      
      To avoid unused log files piling up in local testing, by default the
      full job output is written to `/dev/null`, with this setting overriden
      in the CI config.
      eaf3df45
    • Will Chandler's avatar
      Merge branch 'pks-postgres-with-praefect-flakiness' into 'master' · e3aa6272
      Will Chandler authored
      ci: Hopefully decrease flakiness in `test-with-praefect`
      
      See merge request gitlab-org/gitaly!4583
      e3aa6272
    • Sami Hiltunen's avatar
      Merge branch 'pks-streamcache-fix-flaky-test' into 'master' · 0e162656
      Sami Hiltunen authored
      streamcache: Unlock waiters after cache keys have been pruned
      
      See merge request gitlab-org/gitaly!4589
      0e162656
    • Patrick Steinhardt's avatar
      Merge branch 'pks-ci-fix-test-reports' into 'master' · 398923d8
      Patrick Steinhardt authored
      ci: Fix test reports not being uploaded
      
      See merge request gitlab-org/gitaly!4594
      398923d8
    • Patrick Steinhardt's avatar
      ci: Fix test reports not being uploaded · aefb907a
      Patrick Steinhardt authored
      With 1c112bb7 (ci: Move test reports into temporary directory,
      2022-01-13), we have moved our test reports into a temporary directory
      to prepare for running tests as an unprivileged user. This change broke
      our ability to upload these test reports as artifacts though because
      GitLab only supports artifacts which are relative to the build root.
      
      Fix this issue by instead writing test reports into a new directory in
      our build root that's writeable by the unprivileged test user. While at
      it, pull out the test user ID into a separate variable so that it can be
      documented better.
      aefb907a
  7. May 30, 2022
    • Patrick Steinhardt's avatar
      ci: Increase connection limit for Postgres · a326bfb3
      Patrick Steinhardt authored
      We regularly get CI failures when running with Praefect, which is most
      likely caused by an exhaustion of the database's connection pool.
      Increase the limit so that we can hopefully get to a more stable state.
      
      Note that we cannot set `PGOPTIONS` here as that causes the Postgres
      services to not come up. Instead, we work around this by manually adding
      the configuration to the executed command.
      a326bfb3
    • Patrick Steinhardt's avatar
      catfile: Fix dirtiness check when current object has been fully read · 4b83e599
      Patrick Steinhardt authored
      We always return the currently pending object's dirty state in case the
      request queue has an object set. In case the object has been fully read
      without yet having been closed though this won't account for the queue's
      outstanding requests. So ultimately, the queue may now says its clean
      even though it still got requests pending.
      
      Fix this issue by not returning early in case there is an object. This
      causes us to correctly discard any such processes instead of returning
      them to the cache with still-pending requests. This is an issue we have
      indeed been observing in RPCs which have limits, like ListLFSPointers.
      Amend one of our tests to enable cache reuse of catfile processes, which
      does surface the issue previous to this fix.
      
      Changelog: fixed
      4b83e599
    • Patrick Steinhardt's avatar
      Merge branch 'pks-lfs-smudge-refactor' into 'master' · 469ff7a3
      Patrick Steinhardt authored
      gitaly-lfs-smudge: Refactor code to be more readily extensible
      
      See merge request gitlab-org/gitaly!4587
      469ff7a3
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Remove unused parameter · 21d27406
      Patrick Steinhardt authored
      Remove the unused `to io.Writer` parameter from `handleSmudge()`. We
      alreay return an `io.ReadCloser` that the contents can be read from, so
      there is no need to receive a writer.
      21d27406
    • Patrick Steinhardt's avatar
      repository: Convert GetArchive to use new smudge configuration · ad32b7e7
      Patrick Steinhardt authored
      The `GetArchive()` RPC can optionally convert LFS pointers to their
      actual contents via the gitaly-lfs-smudge filter. The configuration of
      this filter happens via a set of environment variables, which we have
      replaced with a single structure in this commit series.
      
      Convert `GetArchive()` to use the new `smudge.Config` to inject the
      environment.
      ad32b7e7
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Move configuration-code into separate package · 0bc5c6dd
      Patrick Steinhardt authored
      Move the configuration-related code of gitaly-lfs-smudge into a separate
      package so that we can access and inject it in Gitaly's services.
      0bc5c6dd
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Allow passing configuration as a single envvar · 5a252a7b
      Patrick Steinhardt authored
      The gitaly-lfs-smudge binary accepts its configuration as a set of
      environment variables, which is required so that we can pass it through
      Git. Using separate environment variables doesn't scale though: there is
      no easily accessible documentation about what is accepted and what is
      not, and furthermore every new configuration needs another environment
      variable.
      
      Refactor our code to accept a single `GITALY_LFS_SMUDGE_CONFIG` variable
      that contains everything needed by gitaly-lfs-smudge to operate. The old
      environment variables still exist for backwards-compatibility reasons,
      but are overridden in case the new environment variable exists.
      5a252a7b
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Refactor config-handling to have a central struct · 2dd003d8
      Patrick Steinhardt authored
      The gitaly-lfs-smudge binary accepts a set of environment variables to
      configure it. Because of this the configuration of the binary is kind of
      hard to extend: for every piece of information that is required, we need
      to add a new environment variable. Ultimately, this leads to a design
      where we have lots of magic environment variables injected with no clear
      indicator about which ones are used and which ones aren't, similar to
      the case we had with gitaly-hooks before introducing the hooks payload.
      
      Refactor the code and create a central `Config` struct which holds all
      the configuration required. This prepares us for a fix where we can do
      the same like we did for the hooks payload where we only inject a single
      JSON-encoded environment variable that can then be parsed into this
      struct.
      2dd003d8
    • Patrick Steinhardt's avatar
      env: Add helper to extract environment variables from an array · 56dad545
      Patrick Steinhardt authored
      Add a helper function that can extract environment variables by their
      key from an array of environment variables.
      56dad545
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Move config-related code into its own file · a7ed6ea1
      Patrick Steinhardt authored
      We're about to refactor the way the configuration is handled, which also
      includes moving the configuration code into a separate package. Split it
      out into a separate file for now to prepare for this move.
      a7ed6ea1
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Split out a new `run()` function · b62d7112
      Patrick Steinhardt authored
      We're about to move loading the configuration out of `smudge()` so that
      it doesn't need to depend on any global state anymore. Prepare for this
      by pulling out a separate `run()` function so that we can continue to
      log any errors and exit with an error code in a single location, only.
      b62d7112
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Simplify error handling and logging · 96bbfa25
      Patrick Steinhardt authored
      The way `smudge()` handles errors is a bit convoluted right now: in many
      paths it will both log the error and return it to the caller, where the
      caller should then use it as an indicator to decide whether to exit with
      an error code or whether it should exit successfully.
      
      Refactor the code so that we only log any such errors in our `main()`
      function to simplify this logic and make it more Go-like. Also, now that
      logging is mostly done in the `main()` function, move the initialization
      of the logging system close to it.
      96bbfa25
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Do not test logging in unit tests · 86b607c1
      Patrick Steinhardt authored
      We're currently verifying that some of our low-level functions log
      certain events into the logfile. This makes it hard to refactor the
      code, and ultimately we really should only exercise the system under
      test instead of also verifying that some other components like logging
      work as expected. As another hurdle, this is effectively testing global
      state as the loggers are configured globally.
      
      Remove these assertions from the lower-level unit tests to allow for
      easier refactoring of the code. Logging is tested already via our new
      high-level tests which verify that executing the binary does the right
      thing.
      86b607c1
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Fix panic when log config is missing · fb40241b
      Patrick Steinhardt authored
      Because gitaly-lfs-smudge is executed by Git instead of by us we can't
      directly log to either stdout or stderr. Instead, we're currently forced
      to log into a separate file. The location of that file is controlled by
      an environment variable: if present we set up logging, if not we skip
      over the configuration.
      
      There's a bug here though: we return a `nil` closer in case we skip
      configuring the log and don't initialize the logger at all. First, this
      leads to a panic because we unconditionally try to close the closer. And
      second, even if this worked, because we don't configure the logger we'd
      now log to stdout directly. The result is that the logs would end up in
      the final blob's contents as seen by Git.
      
      Fix this bug by always initializing the logger. In case no file is
      given, we ask it to simply write to `io.Discard`.
      fb40241b
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Add tests which exercise binary on a high-level · e1af944e
      Patrick Steinhardt authored
      While we do have tests which verify that parts of the gitaly-lfs-smudge
      command run as expected, we don't have any which test the final binary.
      This makes it hard to test for some specific scenarios and doesn't give
      us an easy way to verify that it works as expected.
      
      Add such tests to improve our test coverage.
      e1af944e
    • Patrick Steinhardt's avatar
      gitaly-lfs-smudge: Move test setup functions into separate file · 1d4c4f66
      Patrick Steinhardt authored
      Move functions required for the test setup into a separate file
      according to our coding style.
      1d4c4f66
  8. May 26, 2022
  9. May 25, 2022
    • Sami Hiltunen's avatar
      Merge branch 'smh-pipeline-dependencies' into 'master' · ac989862
      Sami Hiltunen authored
      Parallelize CI jobs
      
      Closes #3123
      
      See merge request gitlab-org/gitaly!4571
      ac989862
    • Sami Hiltunen's avatar
      Remove build dependency on verify step · 21d391f4
      Sami Hiltunen authored
      The verify step currently depends on the build step as it will fail
      if the Go modules are not downloaded prior to the job executing. If
      the modules are not present, golang-ci will fail with a timeout as
      it exceeds the timeout while downloading the dependencies. This commit
      removes the dependency on build and instead downloads the modules
      prior to executing golang-ci. In the general case when the cache is
      available, this will allow the verify step to execute in parallel with
      the build steps.
      21d391f4