smengcl commented on code in PR #11829:
URL: https://github.com/apache/ozone/pull/11829#discussion_r4418391549
##########
hadoop-ozone/dist/src/main/compose/common/s3a-test.sh:
##########
@@ -17,16 +17,38 @@
# This script runs S3A contract tests against various bucket types on
# a Docker Compose-based Ozone cluster.
-# Requires HADOOP_AWS_DIR to point the directory containing hadoop-aws sources.
+#
+# If you have Hadoop sources locally, set HADOOP_AWS_DIR to the directory
containing `hadoop-aws`.
+# Otherwise it will try to download Hadoop ${HADOOP_VERSION}, defaults to
${hadoop.version}.
-if [[ -z ${HADOOP_AWS_DIR} ]] || [[ ! -e ${HADOOP_AWS_DIR} ]]; then
- echo "Skipping S3A tests due to missing HADOOP_AWS_DIR (directory with
hadoop-aws sources)" >&2
- exit
+if [[ -z "${HADOOP_AWS_DIR:-}" ]]; then
+ export HADOOP_AWS_DIR="${TEMP_DIR:-"/tmp/hadoop-aws-${HADOOP_VERSION}"}"
fi
# shellcheck source=/dev/null
source "$COMPOSE_DIR/../testlib.sh"
Review Comment:
`HADOOP_VERSION` is used here before `testlib.sh` sets its default. Both
`test-s3a.sh` scripts use `set -u`, so a local run without `HADOOP_VERSION` or
`TEMP_DIR` set fails with `HADOOP_VERSION: unbound variable`. CI passes only
because `check.yml` exports it.
Also, if `TEMP_DIR` is set, `HADOOP_AWS_DIR` becomes `TEMP_DIR` itself, and
`hadoop-aws` is extracted into it directly.
How about setting the default after sourcing `testlib.sh`?
```suggestion
# shellcheck source=/dev/null
source "$COMPOSE_DIR/../testlib.sh"
: "${HADOOP_AWS_DIR:=${TEMP_DIR:-/tmp}/hadoop-aws-${HADOOP_VERSION}}"
export HADOOP_AWS_DIR
```
##########
hadoop-ozone/dist/src/main/compose/testlib.sh:
##########
@@ -27,12 +27,21 @@ RESULT_DIR=${RESULT_DIR:-"$COMPOSE_DIR/result"}
source ${_testlib_dir}/compose_v2_compatibility.sh
source "${_testlib_dir}/../smoketest/testlib.sh"
+: ${DOWNLOAD_DIR:="${HOME}/.cache/ozone"}
: ${OZONE_COMPOSE_RUNNING:=false}
: "${OZONE_VOLUME_OWNER:=}"
: ${SECURITY_ENABLED:=false}
: ${SCM:=scm}
: ${SKIP_APACHE_VERIFY_DOWNLOAD:=${CI:-false}}
+if [[ -z ${HADOOP_VERSION:-} ]]; then
+ export HADOOP_VERSION="${hadoop.version}"
Review Comment:
nit: `HADOOP_VERSION` is exported here and via `GITHUB_ENV` in `check.yml`.
Docker Compose prefers shell environment over `.env`, so this overrides
`HADOOP_VERSION=${hadoop.version}${docker.hadoop.image.flavor}` in ozonesecure,
ozonesecure-ha, upgrade and xcompat. The value is the same with the default
empty flavor, but a non-empty `docker.hadoop.image.flavor` would be dropped
from the Hadoop image tag.
Perhaps use a separate variable for the download version, e.g.
`HADOOP_DOWNLOAD_VERSION`?
##########
hadoop-ozone/dist/src/main/compose/testlib.sh:
##########
@@ -696,23 +705,67 @@ download_if_not_exists() {
if [[ -e "${f}" ]]; then
echo "${f} already downloaded"
- else
- echo "Downloading ${f} from ${url}"
- curl --fail --location --output "${f}" --show-error --silent "${url}" ||
rm -fv "${f}"
+ elif ! download_with_retry "${url}" "${f}"; then
+ rm -fv "${f}"
+ return 1
fi
}
+import_keys() {
+ local url="$1"
+
+ keys=$(mktemp)
+ download_with_retry "$url" "$keys"
+ gpg --import "$keys"
+ rm -f "$keys"
+}
+
download_and_verify_apache_release() {
local remote_path="$1"
local f="$(basename "${remote_path}")"
local
base_url="${APACHE_MIRROR_URL:-https://www.apache.org/dyn/closer.lua?action=download&filename=}";
local
checksum_base_url="${APACHE_OFFICIAL_URL:-https://downloads.apache.org/}";
- local download_dir="${DOWNLOAD_DIR:-/tmp}"
- download_if_not_exists "${base_url}${remote_path}" "${download_dir}/${f}"
+ download_if_not_exists "${base_url}${remote_path}" "${DOWNLOAD_DIR}/${f}"
>&2 || return 1
if [[ "${SKIP_APACHE_VERIFY_DOWNLOAD}" != "true" ]]; then
- download_if_not_exists "${checksum_base_url}${remote_path}.asc"
"${download_dir}/${f}.asc"
- gpg --verify "${download_dir}/${f}.asc" "${download_dir}/${f}" || exit 1
+ download_if_not_exists "${checksum_base_url}${remote_path}.asc"
"${DOWNLOAD_DIR}/${f}.asc" >&2
+ gpg --verify "${DOWNLOAD_DIR}/${f}.asc" "${DOWNLOAD_DIR}/${f}" >&2 ||
return 1
Review Comment:
`DOWNLOAD_DIR` is now `~/.cache/ozone` instead of `/tmp`, so files persist.
If verification fails, or an earlier download was interrupted, the file stays,
later runs report "already downloaded", and verification fails again until the
file is deleted manually.
Maybe delete the files when verification fails, so that the next run
downloads them again?
```suggestion
gpg --verify "${DOWNLOAD_DIR}/${f}.asc" "${DOWNLOAD_DIR}/${f}" >&2 || {
rm -fv "${DOWNLOAD_DIR}/${f}" "${DOWNLOAD_DIR}/${f}.asc" >&2; return 1; }
```
##########
.github/workflows/populate-cache.yml:
##########
@@ -59,30 +63,77 @@ jobs:
key: maven-repo-${{ hashFiles('**/pom.xml') }}
- name: Setup Java
- if: steps.restore-cache.outputs.cache-hit != 'true'
uses: actions/setup-java@v6
with:
distribution: 'temurin'
java-version: ${{ env.TEST_JAVA_VERSION }}
- - name: Get NodeJS version
- id: nodejs-version
- if: steps.restore-cache.outputs.cache-hit != 'true'
- run: echo "nodejs-version=$(mvn help:evaluate
-Dexpression=nodejs.version -q -DforceStdout -Dscan=false)" >> $GITHUB_OUTPUT
+ - name: Get versions
+ id: versions
+ run: |
+ NODEJS_VERSION=$(mvn help:evaluate -N -Dexpression=nodejs.version -q
-DforceStdout -Dscan=false)
+ echo "NODEJS_VERSION=$NODEJS_VERSION" >> $GITHUB_ENV
+ HADOOP_VERSION=$(mvn help:evaluate -N -Dexpression=hadoop.version -q
-DforceStdout -Dscan=false)
+ echo "HADOOP_VERSION=$HADOOP_VERSION" >> $GITHUB_ENV
+ RANGER_VERSION=$(mvn help:evaluate -N -Dexpression=ranger.version -q
-DforceStdout -Dscan=false)
+ echo "RANGER_VERSION=$RANGER_VERSION" >> $GITHUB_ENV
+
+ - name: Restore Hadoop
+ id: restore-hadoop
+ uses: apache/infrastructure-actions/stash/restore@restore/v1
+ with:
+ clean: true
+ path: ~/.cache/ozone
+ key: hadoop-${{ env.HADOOP_VERSION }}
+
+ - name: Get Hadoop
+ if: steps.restore-hadoop.outputs.stash-hit != 'true'
+ run: |
+ source hadoop-ozone/dist/src/main/compose/testlib.sh
+ import_keys https://downloads.apache.org/hadoop/common/KEYS
+ download_hadoop
+
+ - name: Stash Hadoop
+ if: steps.restore-hadoop.outputs.stash-hit != 'true'
+ uses: apache/infrastructure-actions/stash/save@save/v1
+ with:
+ path: ~/.cache/ozone
+ key: hadoop-${{ env.HADOOP_VERSION }}
+
+ - name: Restore Ranger
+ id: restore-ranger
+ uses: apache/infrastructure-actions/stash/restore@restore/v1
+ with:
+ clean: true
+ path: ~/.cache/ozone
+ key: ranger-${{ env.RANGER_VERSION }}
+
+ - name: Get Ranger
+ if: steps.restore-ranger.outputs.stash-hit != 'true'
+ run: |
Review Comment:
`clean: true` removes the directory only when a stash is found. On a Ranger
miss, `~/.cache/ozone` still contains the Hadoop tarball and `.asc`, so they
are saved in the Ranger stash too (6 files, 116 MB for `ranger-2.9.0` in your
run). Ranger and STS splits then download the Hadoop tarball as well.
Hadoop is already stashed by this step, so I think we can clear the
directory first:
```suggestion
run: |
rm -rf ~/.cache/ozone
```
##########
.github/workflows/populate-cache.yml:
##########
@@ -59,30 +63,77 @@ jobs:
key: maven-repo-${{ hashFiles('**/pom.xml') }}
- name: Setup Java
- if: steps.restore-cache.outputs.cache-hit != 'true'
uses: actions/setup-java@v6
with:
distribution: 'temurin'
java-version: ${{ env.TEST_JAVA_VERSION }}
- - name: Get NodeJS version
- id: nodejs-version
- if: steps.restore-cache.outputs.cache-hit != 'true'
- run: echo "nodejs-version=$(mvn help:evaluate
-Dexpression=nodejs.version -q -DforceStdout -Dscan=false)" >> $GITHUB_OUTPUT
+ - name: Get versions
+ id: versions
+ run: |
+ NODEJS_VERSION=$(mvn help:evaluate -N -Dexpression=nodejs.version -q
-DforceStdout -Dscan=false)
+ echo "NODEJS_VERSION=$NODEJS_VERSION" >> $GITHUB_ENV
+ HADOOP_VERSION=$(mvn help:evaluate -N -Dexpression=hadoop.version -q
-DforceStdout -Dscan=false)
+ echo "HADOOP_VERSION=$HADOOP_VERSION" >> $GITHUB_ENV
+ RANGER_VERSION=$(mvn help:evaluate -N -Dexpression=ranger.version -q
-DforceStdout -Dscan=false)
+ echo "RANGER_VERSION=$RANGER_VERSION" >> $GITHUB_ENV
+
+ - name: Restore Hadoop
+ id: restore-hadoop
+ uses: apache/infrastructure-actions/stash/restore@restore/v1
+ with:
+ clean: true
+ path: ~/.cache/ozone
+ key: hadoop-${{ env.HADOOP_VERSION }}
+
+ - name: Get Hadoop
+ if: steps.restore-hadoop.outputs.stash-hit != 'true'
+ run: |
+ source hadoop-ozone/dist/src/main/compose/testlib.sh
+ import_keys https://downloads.apache.org/hadoop/common/KEYS
+ download_hadoop
+
+ - name: Stash Hadoop
+ if: steps.restore-hadoop.outputs.stash-hit != 'true'
+ uses: apache/infrastructure-actions/stash/save@save/v1
+ with:
+ path: ~/.cache/ozone
+ key: hadoop-${{ env.HADOOP_VERSION }}
Review Comment:
`stash/save` uses `retention-days: 5` by default, and the save is skipped on
a hit, so the stash is not renewed. On master the daily scheduled run recreates
it after expiry. On `ozone-2.1` and `ozone-2.2` it is recreated only on a
`pom.xml` push, and restore does not fall back to master's stash, so most runs
on those branches will download from apache.org.
Could we set a longer retention here and in `Stash Ranger`?
```suggestion
key: hadoop-${{ env.HADOOP_VERSION }}
retention-days: 30
```
##########
hadoop-ozone/dist/src/test/shell/compose_testlib.bats:
##########
@@ -1,50 +0,0 @@
-#!/usr/bin/env bash
-# Licensed to the Apache Software Foundation (ASF) under one or more
-# contributor license agreements. See the NOTICE file distributed with
-# this work for additional information regarding copyright ownership.
-# The ASF licenses this file to You under the Apache License, Version 2.0
-# (the "License"); you may not use this file except in compliance with
-# the License. You may obtain a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-
-load ../../main/compose/testlib.sh
Review Comment:
nit: `failing1/`, `test1/`, `test2/`, `test3/` and `test4/` under
`src/test/shell` are only used by this test, so I think they can be removed as
well:
```bash
git rm -r hadoop-ozone/dist/src/test/shell/{failing1,test1,test2,test3,test4}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
