From f4710948c4a3d8475fb8b6b3c9c3ff214114d04e Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 2 Jan 2024 15:08:18 -0800 Subject: [PATCH] fix: an odd crash when deleting `null` DEL markers (#18727) fixes #18724 A regression was introduced in #18547, that attempted to file adding a missing `null` marker however we should not skip returning based on versionID instead it must be based on if we are being asked to create a DEL marker or not. The PR also has a side-affect for replicating `null` marker permanent delete, as it may end up adding a `null` marker while removing one. This PR should address both scenarios. --- .github/workflows/replication.yaml | 5 ++ Makefile | 4 ++ cmd/xl-storage-format-v2.go | 8 +-- docs/bucket/versioning/versioning-tests.sh | 81 ++++++++++++++++++++++ 4 files changed, 93 insertions(+), 5 deletions(-) create mode 100755 docs/bucket/versioning/versioning-tests.sh diff --git a/.github/workflows/replication.yaml b/.github/workflows/replication.yaml index 46b5434b6..e6a674f5a 100644 --- a/.github/workflows/replication.yaml +++ b/.github/workflows/replication.yaml @@ -54,3 +54,8 @@ jobs: sudo sysctl net.ipv6.conf.default.disable_ipv6=0 make test-site-replication-minio + - name: Test Versioning + run: | + sudo sysctl net.ipv6.conf.all.disable_ipv6=0 + sudo sysctl net.ipv6.conf.default.disable_ipv6=0 + make test-versioning diff --git a/Makefile b/Makefile index 22a05d488..099ed4db1 100644 --- a/Makefile +++ b/Makefile @@ -59,6 +59,10 @@ test-decom: install-race @env bash $(PWD)/docs/distributed/decom-encrypted-sse-s3.sh @env bash $(PWD)/docs/distributed/decom-compressed-sse-s3.sh +test-versioning: install-race + @echo "Running minio versioning tests" + @env bash $(PWD)/docs/bucket/versioning/versioning-tests.sh + test-configfile: install-race @env bash $(PWD)/docs/distributed/distributed-from-config-file.sh diff --git a/cmd/xl-storage-format-v2.go b/cmd/xl-storage-format-v2.go index b9b84068e..a84f829fd 100644 --- a/cmd/xl-storage-format-v2.go +++ b/cmd/xl-storage-format-v2.go @@ -1410,15 +1410,13 @@ func (x *xlMetaV2) DeleteVersion(fi FileInfo) (string, error) { err = x.setIdx(i, *ver) return "", err } - var err error x.versions = append(x.versions[:i], x.versions[i+1:]...) if fi.MarkDeleted && (fi.VersionPurgeStatus().Empty() || (fi.VersionPurgeStatus() != Complete)) { err = x.addVersion(ventry) + } else if fi.Deleted && uv.String() == emptyUUID { + return "", x.addVersion(ventry) } - // if we remove null version. we should try to add null version to top layer. - if uv.String() != emptyUUID { - return "", err - } + return "", err case ObjectType: if updateVersion && !fi.Deleted { ver, err := x.getIdx(i) diff --git a/docs/bucket/versioning/versioning-tests.sh b/docs/bucket/versioning/versioning-tests.sh new file mode 100755 index 000000000..584734b6f --- /dev/null +++ b/docs/bucket/versioning/versioning-tests.sh @@ -0,0 +1,81 @@ +#!/usr/bin/env bash + +if [ -n "$TEST_DEBUG" ]; then + set -x +fi + +trap 'catch $LINENO' ERR + +# shellcheck disable=SC2120 +catch() { + if [ $# -ne 0 ]; then + echo "error on line $1" + echo "$site server logs =========" + cat "/tmp/${site}_1.log" + echo "===========================" + cat "/tmp/${site}_2.log" + fi + + echo "Cleaning up instances of MinIO" + pkill minio + pkill -9 minio + rm -rf /tmp/multisitea +} + +catch + +set -e +export MINIO_CI_CD=1 +export MINIO_BROWSER=off +export MINIO_ROOT_USER="minio" +export MINIO_ROOT_PASSWORD="minio123" +export MINIO_KMS_AUTO_ENCRYPTION=off +export MINIO_PROMETHEUS_AUTH_TYPE=public +export MINIO_KMS_SECRET_KEY=my-minio-key:OSMM+vkKUTCvQs9YL/CVMIMt43HFhkUpqJxTmGl6rYw= +unset MINIO_KMS_KES_CERT_FILE +unset MINIO_KMS_KES_KEY_FILE +unset MINIO_KMS_KES_ENDPOINT +unset MINIO_KMS_KES_KEY_NAME + +if [ ! -f ./mc ]; then + wget -O mc https://dl.minio.io/client/mc/release/linux-amd64/mc && + chmod +x mc +fi + +minio server --address 127.0.0.1:9001 "http://127.0.0.1:9001/tmp/multisitea/data/disterasure/xl{1...4}" \ + "http://127.0.0.1:9002/tmp/multisitea/data/disterasure/xl{5...8}" >/tmp/sitea_1.log 2>&1 & +minio server --address 127.0.0.1:9002 "http://127.0.0.1:9001/tmp/multisitea/data/disterasure/xl{1...4}" \ + "http://127.0.0.1:9002/tmp/multisitea/data/disterasure/xl{5...8}" >/tmp/sitea_2.log 2>&1 & + +export MC_HOST_sitea=http://minio:minio123@127.0.0.1:9001 + +./mc mb sitea/delissue + +./mc version enable sitea/delissue + +echo hello | ./mc pipe sitea/delissue/hello + +./mc version suspend sitea/delissue + +./mc rm sitea/delissue/hello + +./mc version enable sitea/delissue + +echo hello | ./mc pipe sitea/delissue/hello + +./mc version suspend sitea/delissue + +./mc rm sitea/delissue/hello + +count=$(./mc ls --versions sitea/delissue | wc -l) + +if [ ${count} -ne 3 ]; then + echo "BUG: expected number of versions to be '3' found ${count}" + echo "===== DEBUG =====" + ./mc ls --versions sitea/delissue +fi + +echo "SUCCESS:" +./mc ls --versions sitea/delissue + +catch