Skip to content

test(fileservice): isolate external object storage tests #28291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gouhongshen wants to merge 1 commit into
base: main
Choose a base branch
from
+108 −35
Open

test(fileservice): isolate external object storage tests #28291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 79 additions & 27 deletions pkg/fileservice/object_storage_arguments_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"testing"
"time"

"github.com/matrixorigin/matrixone/pkg/logutil"
"github.com/stretchr/testify/assert"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
Expand Down Expand Up @@ -68,14 +67,31 @@ func TestObjectStorageArguments(t *testing.T) {
})
}

func objectStorageArgumentsForTest(defaultName string, t *testing.T) (ret []ObjectStorageArguments) {
const runExternalObjectStorageTestsEnv = "MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS"

@XuPeng-SH XuPeng-SH Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Wire this opt-in to an automated external-storage workflow before landing. On the current CI default branch, the only jobs receiving TEST_S3FS_ALIYUN/TEST_S3FS_QCLOUD run go test -short, so shouldRunExternalObjectStorageTests always returns false. There is no workflow setting this new variable or selecting the new test entry points; the earlier paired CI#416 / matrixone#26612 rollout was closed unmerged. Consequently every automated run now reports success while skipping Aliyun/QCloud, turning the original flaky coverage into no provider coverage. Please pair this with a trusted exact-head, non-short workflow that sets this flag, runs providers in isolated jobs (pre-merge for relevant paths and scheduled/manual), and removes the credentials from short coverage.

@gouhongshen gouhongshen Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed valid. Current matrixorigin/CI coverage-ut.yaml exports TEST_S3FS_ALIYUN and TEST_S3FS_QCLOUD only to go test -short; CI main has no object-storage integration workflow, and MatrixOne has no caller. Therefore this PR alone would remove provider coverage rather than isolate it.

The required replacement needs a paired matrixorigin/CI change that owns trusted runner/secrets: remove these credentials from coverage, add provider-isolated non-short exact-head jobs with MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS=1, and retain path-filtered plus scheduled/manual triggers. The closed CI#416 and matrixone#26612 show the intended split, but neither landed. That cross-repository CI/secret work is outside the current MatrixOne-only PR authority, so I am leaving this thread unresolved and holding this PR from landing until the paired CI path is authorized and available.

@gouhongshen gouhongshen Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reconfirmed on exact head 3fa1d644eb13c142c30e863210ee688bec35c479: “the previous automated-coverage rollout blocker is still open.” Current matrixorigin/CI@main still exports TEST_S3FS_ALIYUN and TEST_S3FS_QCLOUD in coverage-ut.yaml and runs go test ... -short; it has no MO_RUN_EXTERNAL_OBJECT_STORAGE_TESTS consumer or object-storage workflow, and this MatrixOne branch has no caller. The P1 is valid and not stale.

The required fix is a coordinated matrixorigin/CI change that owns trusted runners/secrets: remove provider credentials from short coverage and add exact-head, non-short, provider-isolated opt-in jobs with relevant path, scheduled, and manual triggers. That external CI/secret administration is outside this MatrixOne-only PR authority. The PR body states the same landing condition, so I am leaving this thread unresolved and holding the PR until an authorized paired change can be linked.


// disk
ret = append(ret, ObjectStorageArguments{
func localObjectStorageArgumentsForTest(defaultName string, t *testing.T) []ObjectStorageArguments {
return []ObjectStorageArguments{{
Name: defaultName,
Endpoint: "disk",
Bucket: t.TempDir(),
})
}}
}

func shouldRunExternalObjectStorageTests(short bool, enabled string) bool {
return !short && enabled == "1"
}

func requireExternalObjectStorageTests(t *testing.T) {
t.Helper()
if shouldRunExternalObjectStorageTests(testing.Short(), os.Getenv(runExternalObjectStorageTestsEnv)) {
return
}
t.Skipf("external object storage tests require -short=false and %s=1", runExternalObjectStorageTestsEnv)
}

func externalObjectStorageArgumentsForTest(defaultName string, t *testing.T) (ret []ObjectStorageArguments) {
t.Helper()
requireExternalObjectStorageTests(t)

// s3.json
content, err := os.ReadFile("s3.json")
Expand All @@ -90,19 +106,22 @@ func objectStorageArgumentsForTest(defaultName string, t *testing.T) (ret []Obje
RoleARN string `json:"role-arn"`
}

if err := json.Unmarshal(content, &config); err == nil {
ret = append(ret, ObjectStorageArguments{
Name: "s3.json " + defaultName,
Endpoint: config.Endpoint,
Region: config.Region,
KeyID: config.APIKey,
KeySecret: config.APISecret,
Bucket: config.Bucket,
RoleARN: config.RoleARN,
KeyPrefix: fmt.Sprintf("%v", rand.Int64()),
})
if err := json.Unmarshal(content, &config); err != nil {
t.Fatalf("parse s3.json: %v", err)
}
ret = append(ret, ObjectStorageArguments{
Name: "s3.json " + defaultName,
Endpoint: config.Endpoint,
Region: config.Region,
KeyID: config.APIKey,
KeySecret: config.APISecret,
Bucket: config.Bucket,
RoleARN: config.RoleARN,
KeyPrefix: fmt.Sprintf("%v", rand.Int64()),
})
}
} else if !os.IsNotExist(err) {
t.Fatalf("read s3.json: %v", err)
}

// s3_fs_test_new.xml
Expand All @@ -112,15 +131,18 @@ func objectStorageArgumentsForTest(defaultName string, t *testing.T) (ret []Obje
XMLName xml.Name `xml:"Spec"`
Cases []S3CredentialTestCase `xml:"Case"`
}
if err := xml.Unmarshal(content, &spec); err == nil {
for _, kase := range spec.Cases {
if kase.Skip {
continue
}
kase.KeyPrefix = fmt.Sprintf("%v", rand.Int64())
ret = append(ret, kase.ObjectStorageArguments)
if err := xml.Unmarshal(content, &spec); err != nil {
t.Fatalf("parse s3_fs_test_new.xml: %v", err)
}
for _, kase := range spec.Cases {
if kase.Skip {
continue
}
kase.KeyPrefix = fmt.Sprintf("%v", rand.Int64())
ret = append(ret, kase.ObjectStorageArguments)
}
} else if !os.IsNotExist(err) {
t.Fatalf("read s3_fs_test_new.xml: %v", err)
}

// envs
Expand All @@ -141,22 +163,52 @@ func objectStorageArgumentsForTest(defaultName string, t *testing.T) (ret []Obje
reader := csv.NewReader(strings.NewReader(value))
argStrs, err := reader.Read()
if err != nil {
logutil.Warn("bad S3FS test spec", zap.Any("spec", value))
continue
t.Fatalf("parse %s: %v", name, err)
}
var args ObjectStorageArguments
if err := args.SetFromString(argStrs); err != nil {
logutil.Warn("bad S3FS test spec", zap.Any("spec", value))
continue
t.Fatalf("parse %s: %v", name, err)
}
args.KeyPrefix = fmt.Sprintf("%v", rand.Int64())

ret = append(ret, args)
}
if len(ret) == 0 {
t.Fatalf("%s=1 but no external object storage test configuration was found", runExternalObjectStorageTestsEnv)
}

return ret
}

func TestShouldRunExternalObjectStorageTests(t *testing.T) {
testCases := []struct {
name string
short bool
enabled string
want bool
}{
{name: "explicitly enabled", enabled: "1", want: true},
{name: "disabled by default"},
{name: "short mode wins", short: true, enabled: "1"},
{name: "invalid value", enabled: "true"},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
assert.Equal(t, testCase.want, shouldRunExternalObjectStorageTests(testCase.short, testCase.enabled))
})
}
}

func TestLocalObjectStorageArgumentsIgnoreExternalConfiguration(t *testing.T) {
t.Setenv("TEST_S3FS_ALIYUN", "name=aliyun,endpoint=https://127.0.0.1:1,bucket=test,key-id=id,key-secret=secret")
args := localObjectStorageArgumentsForTest("test", t)
if !assert.Len(t, args, 1) {
return
}
assert.Equal(t, "test", args[0].Name)
assert.Equal(t, "disk", args[0].Endpoint)
}

func TestQCloudRegion(t *testing.T) {
args := ObjectStorageArguments{
Endpoint: "http://cos.foobar.myqcloud.com",
Expand Down
11 changes: 10 additions & 1 deletion pkg/fileservice/object_storage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,16 @@ func testObjectStorageWithParentContext[T ObjectStorage](
}

func TestObjectStorages(t *testing.T) {
for _, args := range objectStorageArgumentsForTest("test", t) {
testObjectStorages(t, localObjectStorageArgumentsForTest("test", t))
}

func TestObjectStoragesExternal(t *testing.T) {
testObjectStorages(t, externalObjectStorageArgumentsForTest("test", t))
}

func testObjectStorages(t *testing.T, testArguments []ObjectStorageArguments) {
t.Helper()
for _, args := range testArguments {

t.Run(args.Name, func(t *testing.T) {
specCtx, cancel := context.WithTimeout(t.Context(), 3*time.Minute)
Expand Down
5 changes: 3 additions & 2 deletions pkg/fileservice/qcloud_sdk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@ import (
)

func TestQCloudSDK(t *testing.T) {
testArguments := externalObjectStorageArgumentsForTest("test", t)

t.Run("object storage", func(t *testing.T) {
for _, args := range objectStorageArgumentsForTest("test", t) {
for _, args := range testArguments {
if !strings.Contains(args.Endpoint, "myqcloud") {
continue
}
Expand All @@ -62,7 +63,7 @@ func TestQCloudSDK(t *testing.T) {
})

t.Run("file service", func(t *testing.T) {
for _, args := range objectStorageArgumentsForTest("test", t) {
for _, args := range testArguments {
if !strings.Contains(args.Endpoint, "myqcloud") {
continue
}
Expand Down
21 changes: 16 additions & 5 deletions pkg/fileservice/s3_fs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2636,26 +2636,29 @@ type S3CredentialTestCase struct {
ObjectStorageArguments
}

var s3CredentialTestCases = func() []S3CredentialTestCase {
func s3CredentialTestCasesForTest(t *testing.T) []S3CredentialTestCase {
t.Helper()
content, err := os.ReadFile("s3_fs_test_new.xml")
if os.IsNotExist(err) {
return nil
}
if err != nil {
panic(err)
t.Fatal(err)
}
var spec struct {
XMLName xml.Name `xml:"Spec"`
Cases []S3CredentialTestCase `xml:"Case"`
}
err = xml.Unmarshal(content, &spec)
if err != nil {
panic(err)
t.Fatal(err)
}
return spec.Cases
}()
}

func TestNewS3FSFromSpec(t *testing.T) {
requireExternalObjectStorageTests(t)
s3CredentialTestCases := s3CredentialTestCasesForTest(t)
if len(s3CredentialTestCases) == 0 {
t.Skip("no case")
}
Expand Down Expand Up @@ -2930,8 +2933,16 @@ func BenchmarkS3FSAllocateCacheDataHighCardinality(b *testing.B) {
}

func TestS3FSFromSpecs(t *testing.T) {
testS3FSFromSpecs(t, localObjectStorageArgumentsForTest("test", t))
}

func TestS3FSFromExternalSpecs(t *testing.T) {
testS3FSFromSpecs(t, externalObjectStorageArgumentsForTest("test", t))
}

for _, args := range objectStorageArgumentsForTest("test", t) {
func testS3FSFromSpecs(t *testing.T, testArguments []ObjectStorageArguments) {
t.Helper()
for _, args := range testArguments {

t.Run(args.Name, func(t *testing.T) {

Expand Down
Loading