Merge #108485 #109036 #109260 #109288 #109292 #109324 #109340 #109349 #…

…109356

108485: github: code coverage workflows r=RaduBerinde a=RaduBerinde

This change adds two GitHub Action workflows which run on each PR. One generates unit test code coverage data, and one publishes that data to a GCS bucket from where Reviewable can access it.

We generate coverage data using `bazel coverage`, but we restrict it to only test the packages that have been modified by the PR.

Two workflows are required for security (the first workflow runs potentially malicious code from a fork); for more details, see https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Epic: none
Release note: None

109036: roachprod: better determination if scp -R flag can be used r=RaduBerinde a=RaduBerinde

When uploading a file to a cluster, we use the "tree dist" algorithm by default. This uploads the file to a single node, then we copy the file from that node to the other nodes (up to 10).

This only makes sense if the remote-to-remote transfers can happen directly, which only happens if we pass the `-R -A` flags to `scp`. Unfortunately older versions don't support these flags. Currently the flags are only passed if the OS is `darwin`.

This commits improves the determination - we run `ssh -V` (once) and check if the `SSL` major version is three. For reference, some examples of what `ssh -V` returns:
 - recent MacOSX: `OpenSSH_9.0p1, LibreSSL 3.3.6`
 - Ubuntu 22.04: `OpenSSH_8.9p1 Ubuntu-3ubuntu0.3, OpenSSL 3.0.2 15 Mar 2022`

In addition, if the version is not 3, we disable the use of "tree dist".

Epic: none
Release note: None

109260: sql stats: skip tests hitting combinedstmts and statements endpoints r=gtr a=gtr

Part of #109184.

This commit skips tests which hit the `combinedStmts` or `statements` endpoints which will sometimes timeout under stress as a result of recent backend changes. The test investigation is tracked by #109184.

Release note: None

109288: dev: error when trying to `dev test` a bazel tested target r=rickystewart a=liamgillies

Running `dev test` on these integration tests will always fail, so this PR adds a error when running the command on those files.

Fixes: #107813
Release note: None

109292: sql: fix expected batch count for edge case in copy test r=rharding6373 a=rharding6373

In TestLargeDynamicRows we test that 4 rows of data can fit in a batch size of at least 4 rows given default memory sizes. However, when we set the batch row size to the minimum value of 4, the test hook that counts batches counts an extra empty batch. This PR changes adjusts the minimum row size to 5 for the purposes of this test.

Epic: None
Fixes: #109134

Release note: None

109324: build: update bazel builder build docs r=rickystewart a=rail

Previously, the documentation described a manual build of the `bazelbuilder` docker image. The current approach is to use CI to build the image.

This PR updates the documentation to reflect the current process, including the FIPS image steps.

Epic: none
Release note: None

109340: changefeedccl: move node drain handling logic out of kvfeed r=miretskiy a=jayshrivastava

Previously, the kvfeed was responsible for monitoring for
node drains using a goroutine. This change moves this logic
into the change aggregator and removes the goroutine.
Overall, this change makes the code more organized and performant.

This change was inspired by work being done for #109167. The
work in that PR requires being able to restart the kvfeed.
Having drain logic intermingled with the kvfeed makes
restarts much more complex, hard to review, prone to bugs, etc.

Informs: #96953
Release note: None
Epic: None

109349: kv: wait on latches on each key in reverse acquisition order r=arulajmani,kvoli a=nvanbenschoten

This commit allocates latch IDs from the top of the uint64 space and in reverse order. This is done to order latches in the tree on a same key in reverse order of acquisition. Doing so ensures that when we iterate over the tree and see a key with many conflicting latches, we visit the latches on that key in the reverse order that they will be released. In doing so, we minimize the number of open channels that we wait on (calls to `waitForSignal`) and minimize the number of goroutine scheduling points. This is important to avoid spikes in runnable goroutine after each request completes, which can negatively affect node health.

See experiments below.

Epic: None
Release note (performance improvement): The impact of high concurrency blind writes to the same key on goroutine scheduling latency was reduced.

109356: build: explicitly set SKIP_LABEL_TEST_FAILURE in compose.sh r=rickystewart a=chrisseto

Previously, `SKIP_LABEL_TEST_FAILURE` was being set via a teamcity configuration. This change was quite opaque as the majority of CI configuration for Cockroach is stored as shell scripts within its repo. This commit follows that pattern by explicitly setting `SKIP_LABEL_TEST_FAILURE` in the script that runs `TestComposeCompare`.

Epic: None
Release note: None

Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: gtr <[email protected]>
Co-authored-by: Liam Gillies <[email protected]>
Co-authored-by: rharding6373 <[email protected]>
Co-authored-by: Rail Aliiev <[email protected]>
Co-authored-by: Jayant Shrivastava <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Chris Seto <[email protected]>

.github/workflows/code-cover-gen.yml

@@ -0,0 +1,100 @@
+name: PR code coverage (generate)
+on:
+  pull_request:
+    types: [ opened, reopened, synchronize ]
+    branches: [ master ]
+
+jobs:
+  code-cover-gen:
+    runs-on: ubuntu-latest
+    env:
+      PR: ${{ github.event.pull_request.number }}
+      HEAD_SHA: ${{ github.event.pull_request.head.sha }}
+      GH_TOKEN: ${{ github.token }}
+      FETCH_DEPTH: 15
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          # By default, checkout merges the PR into the current master.
+          # Instead, we want to check out the PR as is.
+          ref: ${{ github.event.pull_request.head.sha }}
+          # Fetching the entire history is much slower; we only fetch the last
+          # 15 commits. As such, we don't support PRs with 15 commits or more
+          # (we cannot get to the "base" commit).
+          fetch-depth: ${{ env.FETCH_DEPTH }}
+
+      - name: Set up Bazel cache
+        uses: actions/cache@v3
+        with:
+          path: |
+            ~/.cache/bazel
+          key: ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion', '.bazelrc', 'WORKSPACE', 'WORKSPACE.bazel', 'MODULE.bazel') }}
+          restore-keys: |
+            ${{ runner.os }}-bazel-    
+
+      - name: Get list of changed packages
+        shell: bash
+        run: |
+          set -euxo pipefail
+          
+          MAX_CHANGED_PKGS=20
+          FETCH_DEPTH=${{ env.FETCH_DEPTH }}
+          mkdir -p artifacts
+          
+          skip() {
+            echo "Skipping code coverage on PR #$PR: $1"
+            # Generate the json files with an error (which will show up in Reviewable).
+            jq -n --arg err "$1" '{error: $err}' > artifacts/cover-${PR}-${HEAD_SHA}.json 
+            if [ -n "${BASE_SHA:-}" ]; then
+              jq -n --arg err "$1" '{error: $err}' > artifacts/cover-${PR}-${BASE_SHA}.json 
+            fi
+            echo "SKIP=true" >> "${GITHUB_ENV}"
+            exit 0
+          }
+          
+          # To get the base commit, we get the number of commits in the PR.
+          # Note that github.event.pull_request.base.sha is not what we want,
+          # that is the tip of master and not necessarily the PR fork point.
+          NUM_COMMITS=$(gh pr view $PR --json commits --jq '.commits | length')
+          
+          # The number of commits bust be below the checkout fetch-depth.
+          if [ ${NUM_COMMITS} -ge ${FETCH_DEPTH} ]; then
+            skip "too many commits (${NUM_COMMITS})"
+          fi
+          BASE_SHA=$(git rev-parse HEAD~${NUM_COMMITS})
+          CHANGED_PKGS=$(build/ghactions/changed-go-pkgs.sh ${BASE_SHA} ${HEAD_SHA})
+          NUM_CHANGED_PKGS=$(echo "${CHANGED_PKGS}" | wc -w)
+          if [ ${NUM_CHANGED_PKGS} -gt ${MAX_CHANGED_PKGS} ]; then
+            skip "too many changed packages (${NUM_CHANGED_PKGS})"
+          fi
+          echo "BASE_SHA=${BASE_SHA}" >> "${GITHUB_ENV}"
+          echo "CHANGED_PKGS=${CHANGED_PKGS}" >> "${GITHUB_ENV}"
+
+      - name: Run "after" test coverage
+        if: env.SKIP != 'true'
+        shell: bash
+        run: |
+          set -euxo pipefail
+          CHANGED_PKGS='${{ env.CHANGED_PKGS }}'
+          # Make a copy of the script so that the "before" run below uses the
+          # same version.
+          cp build/ghactions/pr-codecov-run-tests.sh ${RUNNER_TEMP}/
+          ${RUNNER_TEMP}/pr-codecov-run-tests.sh artifacts/cover-${PR}-${HEAD_SHA}.json "${CHANGED_PKGS}"
+
+      - name: Run "before" test coverage
+        if: env.SKIP != 'true'
+        shell: bash
+        run: |
+          set -euxo pipefail
+          BASE_SHA='${{ env.BASE_SHA }}'
+          CHANGED_PKGS='${{ env.CHANGED_PKGS }}'
+          git checkout -f ${BASE_SHA}
+          ${RUNNER_TEMP}/pr-codecov-run-tests.sh artifacts/cover-${PR}-${BASE_SHA}.json "${CHANGED_PKGS}"
+
+      - name: Upload artifacts
+        # Note: we want to upload artifacts even if we skipped the steps above.
+        # See the skip function in the "Get list of changed packages" step.
+        uses: actions/upload-artifact@v2
+        with:
+          name: cover
+          path: artifacts/cover-*.json

.github/workflows/code-cover-publish.yaml

@@ -0,0 +1,58 @@
+name: PR code coverage (publish)
+
+on:
+  workflow_run:
+    workflows: [ "PR code coverage (generate)" ]
+    types: [ "completed" ]
+
+
+jobs:
+  # This job downloads the artifacts generated by the code-cover-gen job and
+  # uploads them to a GCS bucket, from where Reviewable can access them.
+  #
+  # Note that this workflow is not required for a PR to merge; a failure simply
+  # means that there won't be coverage data visible in Reviewable.
+  code-cover-publish:
+    runs-on: ubuntu-latest
+    if: >
+      github.event.workflow_run.event == 'pull_request' &&
+      github.event.workflow_run.conclusion == 'success'
+    steps:
+      - name: 'Download artifact'
+        uses: actions/[email protected]
+        with:
+          script: |
+            var artifacts = await github.actions.listWorkflowRunArtifacts({
+               owner: context.repo.owner,
+               repo: context.repo.repo,
+               run_id: ${{github.event.workflow_run.id }},
+            });
+            var matchArtifact = artifacts.data.artifacts.filter((artifact) => {
+              return artifact.name == "cover"
+            })[0];
+            var download = await github.actions.downloadArtifact({
+               owner: context.repo.owner,
+               repo: context.repo.repo,
+               artifact_id: matchArtifact.id,
+               archive_format: 'zip',
+            });
+            var fs = require('fs');
+            fs.writeFileSync('${{github.workspace}}/cover.zip', Buffer.from(download.data));
+
+      - run: |
+          mkdir -p cover
+          unzip cover.zip -d cover
+
+      - name: 'Authenticate to Google Cloud'
+        uses: 'google-github-actions/auth@v1'
+        with:
+          credentials_json: '${{ secrets.CODECOVER_SERVICE_ACCOUNT_KEY }}'
+
+      - name: 'Upload to GCS'
+        uses: 'google-github-actions/upload-cloud-storage@v1'
+        with:
+          path: 'cover'
+          glob: '**/cover-*.json'
+          parent: false
+          destination: 'crl-codecover-public/pr-cockroach/'
+          process_gcloudignore: false

build/README.md

@@ -137,20 +137,11 @@ Please follow the instructions above on updating the golang version, omitting th
 
 The `bazelbuilder` image is used exclusively for performing builds using Bazel. Only add dependencies to the image that are necessary for performing Bazel builds. (Since the Bazel build downloads most dependencies as needed, updates to the Bazel builder image should be very infrequent.) The `bazelbuilder` image is published both for `amd64` and `arm64` platforms. You can go through the process of publishing a new Bazel build
 
-- (One-time setup) Depending on how your Docker instance is configured, you may have to run `docker run --privileged --rm tonistiigi/binfmt --install all`. This will install `qemu` emulators on your system for platforms besides your native one.
 - Edit `build/bazelbuilder/Dockerfile` as desired.
-- Build the image for both platforms and publish the cross-platform manifest. Note that the non-native build for your image will be very slow since it will have to emulate.
-```
-    TAG=$(date +%Y%m%d-%H%M%S)
-    docker build --platform linux/amd64 -t cockroachdb/bazel:amd64-$TAG build/bazelbuilder
-    docker push cockroachdb/bazel:amd64-$TAG
-    docker build --platform linux/arm64 -t cockroachdb/bazel:arm64-$TAG build/bazelbuilder
-    docker push cockroachdb/bazel:arm64-$TAG
-    docker manifest rm cockroachdb/bazel:$TAG
-    docker manifest create cockroachdb/bazel:$TAG cockroachdb/bazel:amd64-$TAG cockroachdb/bazel:arm64-$TAG
-    docker manifest push cockroachdb/bazel:$TAG
-```
-- Then, update `build/.bazelbuilderversion` with the new tag and commit all your changes.
+- Build the image by triggering the `Build and Push Bazel Builder Image` build in TeamCity. The generated image will be published to https://hub.docker.com/r/cockroachdb/bazel.
+- Update `build/.bazelbuilderversion` with the new tag and commit all your changes.
+- Build the FIPS image by triggering the `Build and Push FIPS Bazel Builder Image` build in TeamCity. The generated image will be published to https://hub.docker.com/r/cockroachdb/bazel-fips.
+- Update `build/.bazelbuilderversion-fips` with the new tag and commit all your changes.
 - Ensure the "Bazel CI" job passes on your PR before merging.
 
 #  Dependencies

build/ghactions/changed-go-pkgs.sh

@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+
+BASE_SHA="$1"
+HEAD_SHA="$2"
+
+if [ -z "$HEAD_SHA" ];then
+    echo "Usage: $0 <base-sha> <head-sha>"
+    exit 1
+fi
+
+git diff --name-only "${BASE_SHA}..${HEAD_SHA}" -- "pkg/**/*.go" ":!*/testdata/*" ":!pkg/acceptance/compose/gss/psql/**" \
+  | xargs -rn1 dirname \
+  | sort -u \
+  | { while read path; do if ls "$path"/*.go &>/dev/null; then echo -n "$path "; fi; done; }

build/ghactions/pr-codecov-run-tests.sh

@@ -0,0 +1,49 @@
+#!/usr/bin/env bash
+
+set -xeuo pipefail
+
+output_json_file="$1"
+packages="$2"
+
+if [ -z "${packages}" ]; then
+  echo "No packages; skipping"
+  touch "${output_json_file}"
+  exit 0
+fi
+
+
+# Find the targets. We need to convert from, e.g.
+#   pkg/util/log/logpb pkg/util/quotapool
+# to
+#   //pkg/util/log/logpb:* + //pkg/util/quotapool:*
+
+paths=""
+sep=""
+for p in ${packages}; do
+  paths="${paths}${sep}//$p:*"
+  sep=" + "
+done
+
+targets=$(bazel query "kind(\".*_test\", ${paths})")
+
+if [[ -z "${targets}" ]]; then
+  echo "No test targets found"
+  exit 0
+fi
+
+echo "Running tests"
+
+# TODO(radu): do we need --strip=never?
+bazel coverage \
+  --@io_bazel_rules_go//go/config:cover_format=lcov --combined_report=lcov \
+  --instrumentation_filter="//pkg/..." \
+  ${targets}
+
+lcov_file="$(bazel info output_path)/_coverage/_coverage_report.dat"
+if [ ! -f "${lcov_file}" ]; then
+  echo "Coverage file ${lcov_file} does not exist"
+  exit 1
+fi
+
+echo "Converting coverage file"
+bazel run @go_sdk//:bin/go -- run github.com/cockroachdb/code-cov-utils/[email protected] "${lcov_file}" "${output_json_file}"

build/teamcity/cockroach/nightlies/compose.sh

@@ -6,6 +6,12 @@ dir="$(dirname $(dirname $(dirname $(dirname "${0}"))))"
 source "$dir/teamcity-support.sh"
 source "$dir/teamcity-bazel-support.sh"
 
+# The test failures generated by TestComposeCompare are not necessarily
+# failures per se. They're cases of behavioral divergences from Postgres. While
+# our compatibility guarantees are not 100%, it's better to treat failures as
+# information to occasionally review.
+export SKIP_LABEL_TEST_FAILURE=1
+
 tc_start_block "Run compose tests"
 
 bazel build //pkg/cmd/bazci --config=ci