Use minio console access as readiness check #58
Conversation
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
saswatamcode
left a comment
saswatamcode
left a comment
There was a problem hiding this comment.
Thanks! Some suggestions.
| 200, | ||
| 200, | ||
| ), | ||
| Readiness: e2e.NewHTTPReadinessProbe(AccessPortName, "/minio/health/live", 200, 200), |
There was a problem hiding this comment.
This seems to be the liveness probe and not the readiness probe https://github.com/minio/minio/blob/master/docs/metrics/healthcheck/README.md#readiness-probe.
Maybe we can try switching to minio/health/ready and see if that works well? That way we can avoid having to load the full console. 🙂
There was a problem hiding this comment.
Liveness probe was giving false positives, so I don't trust much that a readiness probe won't give false positives too.
According to that documentation from MinIO, their liveness and readiness probes do the same thing:
- liveness:
Only fails if 'etcd' is configured and unreachable. - readiness:
Only fails if 'etcd' is configured and unreachable.
There was a problem hiding this comment.
We won't load the "full" console. As Javascript doesn't run, it's much more lightweight. It's also hacky, but it is what works 100% of the time.
There was a problem hiding this comment.
In fact, MinIO's liveness and readiness check are exactly the same:
There was a problem hiding this comment.
Oh wow! Ideally, this should be fixed on minio side.
But since we already have working "console"-based one then I think this is fine then.
Do you mind opening an issue on minio repo and linking here as a TODO, so that if this is ever fixed, we can update and remove hack here? 🙂
There was a problem hiding this comment.
I'll try to carve some time to create a snippet to reliably reproduce the issue and start a discussion over there. I prefer to not tie the creation of the issue there to merging work here, especially because their readiness probe "does what it says it does". It'll be more of a design question for them.
There was a problem hiding this comment.
Yeah, seems like they closed issue too. But IMO, readiness and liveness should mean diff things. Anyway, thanks for adding comment!
db/db_test.go
Outdated
| t.Cleanup(e.Close) | ||
|
|
||
| minio := NewMinio(e, "mintest", "bkt") | ||
| testutil.Ok(t, e2e.StartAndWaitReady(minio)) |
There was a problem hiding this comment.
Maybe we should do some operation here, to validate that StartAndWaitReady returning actually means that minio is ready to receive requests?
saswatamcode
left a comment
saswatamcode
left a comment
There was a problem hiding this comment.
LGTM minus test and TODO!
|
The build is drunk. It complains that minio/minio-go/v7 isn't in the go.sum files, but it's there: https://github.com/efficientgo/e2e/pull/58/files#diff-3295df7234525439d778f1b282d146a4f1ff6b415248aaac074e8042d9f42d63R167 |
| github.com/minio/minio-go/v7 v7.0.45 h1:g4IeM9M9pW/Lo8AGGNOjBZYlvmtlE1N5TQEYWXRWzIs= | ||
| github.com/minio/minio-go/v7 v7.0.45/go.mod h1:nCrRzjoSUQh8hgKKtu3Y708OLvRLtuASMg2/nvmbarw= |
There was a problem hiding this comment.
For some reason, the build tells me this doesn't exist.
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
* Use minio console access as readiness check Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Update etcd example lines Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Please the copyright linter * Test minio readiness by making a bucket * Rebuild Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> * Adding comment linking to issue about minio readiness check * Go fmt file * Update example go.sum file * Update mdox entry Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com> Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
2 participants
Signed-off-by: Douglas Camata 159076+douglascamata@users.noreply.github.com
This should fix #11. As my comment there mentions, I applied the same solution used at thanos-io/thanos#5615: using the MinIO console as readiness check. It has been working well there so far.
As an addition I added a small and basic test for the
NewMiniofunction.