Summary:
- Move existing test under a `testify` Suite as `baseRedisWithServerTestSuite`
- Those tests require real redis server.
- Add `go.uber.org/mock/mockgen@latest` as dependency
- as a tool (Makefile).
- in the `go.mod` file.
- Mock redis client lives under a `mock` directory under the queue module.
- That mock module has an extra hand-written mock in-memory redis-like struct.
- Add tests using the mock redis client.
- Changed the logic around queue provider creation.
- Now the `getNewQueue` returns a Queue provider directly, not an init
function to create it.
The whole Queue module is close to impossible to test properly because
everything is private, everything goes through a struct route. Because
of that, we can't test for example what keys are used for given queue.
To overcome this, as a first step I removed one step from that hard
route by allowing custom calls to create new queue provider. To achieve
this, I moved the creation logic into the `getNewQueue` (previously it
was `getNewQueueFn`). That changes nothing on that side, everything goes
as before, except the `newXXX` call happens directly in that function
and not outside that.
That made it possible to add extra provider specific parameters to those
function (`newXXX`). For example a client on redis. Calling it through
the `getNewQueue` function, it gets `nil`.
- If the provided client is not `nil`, it will use that instead of the
connection string.
- If it's `nil` (default behaviour), it creates a new redis client as it
did before, no changes to that.
The rest of the provider code is unchanged. All these changes were
required to make it possible to generate mock clients for providers and
use them.
For the tests, the existing two test cases are good with redis server,
and they need some extra helpers, for example to start a new redis
server if required, or waiting on a redis server to be ready to use.
These helpers are only required for test cases using real redis server.
For better isolation, moved existing test under a testify Suite, and
moved them into a new test file called `base_redis_with_server_test.go`
because, well they test the code with server. These tests do exactly the
same as before, calling the same sub-tests the same way as before, the
only change is the structure of the test (remove repetition, scope
server related helper functions).
Finally, we can create unit tests without redis server. The main focus of
this group of tests are higher level overview of operations. With the
mock redis client we can set up expectations about used queue names,
received values, return value to simulate faulty state.
These new unit test functions don't test all functionality, at least
it's not aimed for it now. It's more about the possibility of doing that
and add extra tests around parts we couldn't test before, for example
key.
What extra features can test the new unit test group:
- What is the received key for given queue? For example using `prefix`,
or if all the `SXxx` calls are expected to use `queue_unique` if
it's a unique queue.
- If it's not a unique queue, no `SXxx` functions are called, because
those sets are used only to check if a value is unique or not.
- `HasItem` return `false` always if it's a non-unique queue.
- All functions are called exactly `N` times, and we don't have any
unexpected calls to redis from the code.
Signed-off-by: Victoria Nadasdi <victoria@efertone.me>
For security reasons, scoping access to a redis server via ACL rules is
a good practice. Some parts of the codebase handles prefix like cache[^1]
and session[^2], but the queue module doesn't.
This patch adds this missing functionality to the queue module.
Note about relevant test:
I tried to keep the PR as small as possible (and reasonable), and not
change how the test runs. Updated the existing test to use the same
redis address and basically duplicated the test with the extra flag. It
does NOT test if the keys are correct, it ensures only it works as
expected. To make assertions about the keys, the whole test has to be
updated as the general wrapper doesn't allow the main test to check
anything provider (redis) specific property. That's not something I
wanted to take on now.
[^1]: e4c3c039be/modules/cache/cache_redis.go (L139-L150)
[^2]: e4c3c039be/modules/session/redis.go (L122-L129)
Signed-off-by: Victoria Nadasdi <victoria@efertone.me>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3836
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Victoria Nadasdi <victoria@efertone.me>
Co-committed-by: Victoria Nadasdi <victoria@efertone.me>
(cherry picked from commit e1bbfa36197ebab97954e8195f7d36adf7c85d56)
(cherry picked from commit 91245ca9179a46047a351247dacecdace557111d)
(cherry picked from commit 705d0558be2c90d06e9e5b883044fd0b275b1113)
(cherry picked from commit 9247594970c9db109e3e6ca3fd87485450df921c)
(cherry picked from commit 9db1158a487e00e588810459fe402cc2ccea43f7)
(cherry picked from commit 3b36b77d87a90fbea03fc16638657e19328ccedc)
(cherry picked from commit 162fa1d8ae3753dd8ee51698555e495f2c63d925)
(cherry picked from commit d03d0afbb565c8bc8b723e10c8c70b69e7af7b80)
(cherry picked from commit 7b8f92f7871b838bc2eefa34e7dc48bcd141d1d5)
(cherry picked from commit 035abca9691d33e319062325dae402da66683c43)
(cherry picked from commit a8fbf6bb56046665cb2cde0ffcc753f56b2f0f2d)
(cherry picked from commit 3be681d037b07880236cae1aa70245e5eb4d1497)
(cherry picked from commit 7e5d471c832ee3fea378ecc97835b038bd55a8e1)
(cherry picked from commit 323801d935fec2c6d460192b62fa12b5204da76e)
(cherry picked from commit 3fdfe4bfea623111f1f97e50b71b98a63c8b38e7)
(cherry picked from commit 58a07421a4508ca298c1c3a45d33d49737ee98d8)
(cherry picked from commit dbb71a4c8502b640857d3500dda12ab4b5d74b29)
(cherry picked from commit d442113520d21149e155d1e62bbeb6a35a6aec08)
(cherry picked from commit d3329f01f8c7145c422b159509f544ec83604a51)
(cherry picked from commit 069a1d68b856898e2913d1d4456deb7f1e976a6c)
(cherry picked from commit 14919e609a4dd9ae9ca19880ffc459def8bea273)
(cherry picked from commit 49b76be1068d1f83169956bb141116481a7e6a3c)
(cherry picked from commit 0fe9f257d2bd277f5cd620fe04e4b80b5abcd585)
(cherry picked from commit b583bebeab3d0b182df6b5d087522a4fb89ba3e9)
(cherry picked from commit 5c616e43a64451d607b6ee24400708d2704fd4db)
(cherry picked from commit 854bcea9051dc615cfd6d3e8cb03986e9058fd65)
(cherry picked from commit c2acb181c57e6ffef37df1a3a3b1b63c326cdd43)
(cherry picked from commit 1cb07e71d14118871ae40a82adabcde851a3e172)
(cherry picked from commit 5d3f09e6351614a8db979995299ac1b94ebf08ee)
(cherry picked from commit f8bf1c8d42be0eb40f4d4fdc72e7e4cefa842e52)
(cherry picked from commit a471ed4576607a4e13cac980016c9e2a702d9fd5)
(cherry picked from commit 95c755f4e34bb753a0f94f87f02a17256d7d1619)
(cherry picked from commit 1d8bc5215f6918e11d8beb1f7e252b04d9c15bb6)
(cherry picked from commit 45c1e7b8d0920db98556ecfdf0d1111c2ffcb66e)
(cherry picked from commit 2eb4d93af7b5679228dc38578a746242250e5d92)
(cherry picked from commit 98dbce5e147432194d6f177133dcaabe04309712)
(cherry picked from commit fbe2fb5861c90fcc292f357f45f804ee87594b6e)
Replaces #24641
Currently, unit tests fail when run locally (unless users have minio
instance running). This PR only requires redis unit tests if in CI.
- Only run redis unit tests when `CI` env variable is set
- Add minio as a service in unit tests actions
# ⚠️ Breaking
Many deprecated queue config options are removed (actually, they should
have been removed in 1.18/1.19).
If you see the fatal message when starting Gitea: "Please update your
app.ini to remove deprecated config options", please follow the error
messages to remove these options from your app.ini.
Example:
```
2023/05/06 19:39:22 [E] Removed queue option: `[indexer].ISSUE_INDEXER_QUEUE_TYPE`. Use new options in `[queue.issue_indexer]`
2023/05/06 19:39:22 [E] Removed queue option: `[indexer].UPDATE_BUFFER_LEN`. Use new options in `[queue.issue_indexer]`
2023/05/06 19:39:22 [F] Please update your app.ini to remove deprecated config options
```
Many options in `[queue]` are are dropped, including:
`WRAP_IF_NECESSARY`, `MAX_ATTEMPTS`, `TIMEOUT`, `WORKERS`,
`BLOCK_TIMEOUT`, `BOOST_TIMEOUT`, `BOOST_WORKERS`, they can be removed
from app.ini.
# The problem
The old queue package has some legacy problems:
* complexity: I doubt few people could tell how it works.
* maintainability: Too many channels and mutex/cond are mixed together,
too many different structs/interfaces depends each other.
* stability: due to the complexity & maintainability, sometimes there
are strange bugs and difficult to debug, and some code doesn't have test
(indeed some code is difficult to test because a lot of things are mixed
together).
* general applicability: although it is called "queue", its behavior is
not a well-known queue.
* scalability: it doesn't seem easy to make it work with a cluster
without breaking its behaviors.
It came from some very old code to "avoid breaking", however, its
technical debt is too heavy now. It's a good time to introduce a better
"queue" package.
# The new queue package
It keeps using old config and concept as much as possible.
* It only contains two major kinds of concepts:
* The "base queue": channel, levelqueue, redis
* They have the same abstraction, the same interface, and they are
tested by the same testing code.
* The "WokerPoolQueue", it uses the "base queue" to provide "worker
pool" function, calls the "handler" to process the data in the base
queue.
* The new code doesn't do "PushBack"
* Think about a queue with many workers, the "PushBack" can't guarantee
the order for re-queued unhandled items, so in new code it just does
"normal push"
* The new code doesn't do "pause/resume"
* The "pause/resume" was designed to handle some handler's failure: eg:
document indexer (elasticsearch) is down
* If a queue is paused for long time, either the producers blocks or the
new items are dropped.
* The new code doesn't do such "pause/resume" trick, it's not a common
queue's behavior and it doesn't help much.
* If there are unhandled items, the "push" function just blocks for a
few seconds and then re-queue them and retry.
* The new code doesn't do "worker booster"
* Gitea's queue's handlers are light functions, the cost is only the
go-routine, so it doesn't make sense to "boost" them.
* The new code only use "max worker number" to limit the concurrent
workers.
* The new "Push" never blocks forever
* Instead of creating more and more blocking goroutines, return an error
is more friendly to the server and to the end user.
There are more details in code comments: eg: the "Flush" problem, the
strange "code.index" hanging problem, the "immediate" queue problem.
Almost ready for review.
TODO:
* [x] add some necessary comments during review
* [x] add some more tests if necessary
* [x] update documents and config options
* [x] test max worker / active worker
* [x] re-run the CI tasks to see whether any test is flaky
* [x] improve the `handleOldLengthConfiguration` to provide more
friendly messages
* [x] fine tune default config values (eg: length?)
## Code coverage:
Victoria Nadasdi
Earl Warren
Kyle D
wxiaoguang
