Skip to content
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.

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

fix minio storage iterator path #24691

Merged
merged 15 commits into from
May 13, 2023
Merged

Conversation

fuxiaohei
Copy link
Contributor

minio storage iterator shows different behavior with local fs iterator.

in local fs storage:

s.IterateObjects("prefix", func(path,obj)
     println(path) // show "prefix/xxx.file"
})

in minio storage:

s.IterateObjects("prefix", func(path,obj)
     println(path) // show "xxx.file"
})

I think local fs is correct, minio use wrong basePath to trim storage path prefix.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 13, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 13, 2023
@lunny
Copy link
Member

lunny commented May 13, 2023

Are there any features affected by this?

@lunny lunny added type/bug outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels May 13, 2023
@wxiaoguang
Copy link
Contributor

Interfaces should have the same behaviors.

@wxiaoguang
Copy link
Contributor

ps: I guess we need some common test code for different storages, to make sure they behave consistently

@fuxiaohei
Copy link
Contributor Author

Are there any features affected by this?

I think there are no affects. All are s.IterateObjects("")

@lunny lunny added this to the 1.20.0 milestone May 13, 2023
@lunny
Copy link
Member

lunny commented May 13, 2023

There is a test TestLocalStorageIterator but only for local disk, maybe we can have another test for minio with the same test data.

Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

tests needed.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 13, 2023
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 13, 2023
@wxiaoguang
Copy link
Contributor

How about using a common function to test all storages? Then it could guarantee that they have the consistent behavior.

ps: when rewriting the queue, I introduced a general test function, then all queues are tested by it, so all queues should have the same behavior now

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 13, 2023
Comment on lines 132 to 133
p = strings.TrimPrefix(p, "/") // url path prefix should not start with /
return util.PathJoinRelX(m.basePath, p)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think util.PathJoinRelX already removes the prefix slash. no need to trim it ahead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If p = "/" and m.base = "", it generates "./" which is invalid to minio api, shows Received unexpected error: Object name cannot be empty

Copy link
Contributor

@wxiaoguang wxiaoguang May 13, 2023

Choose a reason for hiding this comment

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

Nope, it never generates "./" here.

See the comment of "PathJoinRel" family functions, it only outputs "", "." or "foo/bar" path

Even if you trim "/" here, if the input p is "./" , you still get "." and later your code append another suffix slash.

So ideally, I think it should be:

p = util.PathJoinRelX(m.basePath, p)
if p == "." {  p = "" }   // minio doesn't use dot as relative path
return p

I guess there could be a test case for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if p == "." { p = "" } is ok

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 13, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 13, 2023
@silverwind silverwind enabled auto-merge (squash) May 13, 2023 21:33
@silverwind silverwind merged commit 61ad4c6 into go-gitea:main May 13, 2023
@GiteaBot
Copy link
Contributor

I was unable to create a backport for 1.19. @fuxiaohei, please send one manually. 🍵

go run ./contrib/backport 24691
...  // fix git conflicts if any
go run ./contrib/backport --continue

@GiteaBot GiteaBot added backport/manual No power to the bots! Create your backport yourself! and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels May 13, 2023
@fuxiaohei fuxiaohei deleted the fix/storage-iterator branch May 13, 2023 23:07
@lunny lunny removed the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label May 14, 2023
@lunny
Copy link
Member

lunny commented May 14, 2023

Since v1.19 hasn't prefix, it's unnecessary to backport.

zjjhot added a commit to zjjhot/gitea that referenced this pull request May 15, 2023
* upstream/main: (30 commits)
  Don't filter action runs based on state (go-gitea#24711)
  Add Go package registry (go-gitea#24687)
  Fix flash of unstyled content in action view page (go-gitea#24712)
  Clean up various avatar dimensions (go-gitea#24701)
  Remove the parallelizing when loading repo for dashboard (go-gitea#24705)
  Optimize actions list by removing an unnecessary `git` call (go-gitea#24710)
  Update cron-translations.yml (go-gitea#24708)
  Fix run list broken when trigger user deleted (go-gitea#24706)
  Remove Fomantic comment module (go-gitea#24703)
  Update to Alpine 3.18 (go-gitea#24700)
  fix minio storage iterator path (go-gitea#24691)
  Add status indicator on main home screen for each repo (go-gitea#24638)
  Add test for api team orgnization (go-gitea#24699)
  Improve button-ghost, remove tertiary button (go-gitea#24692)
  Add icon support for safari (go-gitea#24697)
  Improve avatar uploading / resizing / compressing, remove Fomantic card module (go-gitea#24653)
  Fix docs documenting invalid `@every` for `OLDER_THAN` cron settings (go-gitea#24695)
  Fix `organization` field being `null` in `GET /api/v1/teams/{id}` (go-gitea#24694)
  Use standard HTTP library to serve files (go-gitea#24693)
  Add `eslint-plugin-eslint-comments` (go-gitea#24690)
  ...
@DmitryFrolovTri
Copy link
Contributor

DmitryFrolovTri commented May 17, 2023

@fuxiaohei @lunny I think this broke local tests at least for me - when they are run locally here is what I get consistently when I run make test tried with go1.19, go1.19.9, go 1.20.2
Does anything in gitea actually start the http://127.0.0.1:9000/gitea/ for those tests?

Can we modify the standard testing that it avoids running minio test if no minio? May be to run this test
we define an env variable minio sort of like with sqlite? What is your view? Or may be nominio
or add some code that verifies that minio is actually running and if not simply skips the test?

2023/05/17 08:27:10 ...les/storage/minio.go:101:NewMinioStorage() [I] Creating Minio storage at 127.0.0.1:9000:gitea with base path 
--- FAIL: TestMinioStorageIterator (4.98s)
    storage_test.go:15: 
        	Error Trace:	/home/puni/sources/mine/gitea-sapk-fork-test/modules/storage/storage_test.go:15
        	            				/home/puni/sources/mine/gitea-sapk-fork-test/modules/storage/minio_test.go:11
        	Error:      	Received unexpected error:
        	            	Put "http://127.0.0.1:9000/gitea/": dial tcp 127.0.0.1:9000: connect: connection refused
        	Test:       	TestMinioStorageIterator
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0xa35537]

goroutine 47 [running]:
testing.tRunner.func1.2({0xb7f5e0, 0x10d6330})
	/home/puni/.gvm/gos/go1.20.2/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/home/puni/.gvm/gos/go1.20.2/src/testing/testing.go:1529 +0x39f
panic({0xb7f5e0, 0x10d6330})
	/home/puni/.gvm/gos/go1.20.2/src/runtime/panic.go:884 +0x213
code.gitea.io/gitea/modules/storage.testStorageIterator(0xc00036c4e0, {0xc16a8a?, 0x5?}, {0xbec960?, 0xc00036e080?})
	/home/puni/sources/mine/gitea-sapk-fork-test/modules/storage/storage_test.go:27 +0x3d7
code.gitea.io/gitea/modules/storage.TestMinioStorageIterator(0x0?)
	/home/puni/sources/mine/gitea-sapk-fork-test/modules/storage/minio_test.go:11 +0x87
testing.tRunner(0xc00036c4e0, 0xc739d8)
	/home/puni/.gvm/gos/go1.20.2/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
	/home/puni/.gvm/gos/go1.20.2/src/testing/testing.go:1629 +0x3ea
FAIL	code.gitea.io/gitea/modules/storage	5.635s

to run tests I clone a fresh copy of repo and then:

TAGS="bindata sqlite sqlite_unlock_notify" make build
TAGS="bindata sqlite sqlite_unlock_notify" make test

silverwind added a commit that referenced this pull request May 17, 2023
Fix #24691 (comment)

---------

Co-authored-by: silverwind <me@silverwind.io>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/manual No power to the bots! Create your backport yourself! lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants