Implement Docker healthcheck #4364

noisersup · 2024-06-14T12:44:14Z

Description

Closes #4246.

Readiness checklist

I added/updated unit tests (and they pass).
I added/updated integration/compatibility tests (and they pass).
I added/updated comments and checked rendering.
I made spot refactorings.
I updated user documentation.
I ran task all, and it passed.
I ensured that PR title is good enough for the changelog.
(for maintainers only) I set Reviewers (@FerretDB/core), Milestone (Next), Labels, Project and project's Sprint fields.
I marked all done items in this checklist.

codecov · 2024-06-14T12:48:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.04%. Comparing base (de509c5) to head (7c357b9).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4364      +/-   ##
==========================================
- Coverage   74.31%   74.04%   -0.28%     
==========================================
  Files         323      323              
  Lines       22364    22398      +34     
==========================================
- Hits        16620    16584      -36     
- Misses       4563     4607      +44     
- Partials     1181     1207      +26

see 12 files with indirect coverage changes

Flag	Coverage Δ
filter-true	`67.47% <ø> (-0.31%)`	⬇️
hana-1	`?`
integration	`67.47% <ø> (-0.31%)`	⬇️
mongodb-1	`5.05% <ø> (+0.09%)`	⬆️
postgresql-1	`44.01% <ø> (+0.06%)`	⬆️
postgresql-2	`44.89% <ø> (-0.02%)`	⬇️
postgresql-3	`42.99% <ø> (-0.05%)`	⬇️
postgresql-4	`40.27% <ø> (+0.01%)`	⬆️
postgresql-5	`43.77% <ø> (-0.30%)`	⬇️
sqlite-1	`43.19% <ø> (+0.06%)`	⬆️
sqlite-2	`44.03% <ø> (+0.03%)`	⬆️
sqlite-3	`42.22% <ø> (-0.11%)`	⬇️
sqlite-4	`39.42% <ø> (+0.05%)`	⬆️
sqlite-5	`43.01% <ø> (-0.04%)`	⬇️
unit	`33.04% <ø> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

AlekSi

There are merge conflicts now

mergify · 2024-06-19T08:37:07Z

@noisersup this pull request has merge conflicts.

chilagrow

See comments from @AlekSi.

rumyantseva

Left some thoughts.

rumyantseva · 2024-06-21T15:40:30Z

cmd/ferretdb/ping.go

+
+		client, err := mongo.Connect(ctx, options.Client().ApplyURI(u))
+		if err != nil {
+			l.Fatal(err)


There is a tricky thing about logs of Fatal level. Fatal causes os.Exit which means that defers won't be executed. In this particular case, let's say we have two items in the urls slice. Let's say the first item was connected successfully. Disconnect of this item is deferred. Let's say the second item's connection failed here. This defer from the first item will never be called.

So, we either shouldn't defer disconnect (e.g. we can disconnect right after ping) or we shouldn't use Fatal.

rumyantseva · 2024-06-21T15:41:03Z

cmd/ferretdb/ping.go

+
+		defer func() {
+			if err = client.Disconnect(ctx); err != nil {
+				l.Fatal(err)


Same here about defers. If more than one things were deferred, the first defer's execution will call os.Exit in case of failure, and the second one won't be executed.

AlekSi · 2024-06-26T17:24:22Z

cmd/ferretdb/main.go

-		logUUID = ""
-	}
-
+// It should be preffered for specific single commands/operations over setupLoggerWithStartupMsg.


That comment was ignored

AlekSi · 2024-06-26T18:11:24Z

cmd/ferretdb/ping.go

+		}
+
+		if err = client.Ping(ctx, nil); err != nil {
+			l.Fatal(err)


Unfortunately, process termination might not close all TCP connections properly in some configurations. We have to call client.Disconnect before checking ping result

AlekSi · 2024-06-26T18:11:50Z

cmd/ferretdb/ping.go

+
+		u := &url.URL{
+			Scheme: "mongodb",
+			Host:   fmt.Sprintf("%s:%s", host, port),


net.JoinHostPort

AlekSi · 2024-06-26T18:12:18Z

cmd/ferretdb/ping.go

+
+	var urls []string
+
+	if cli.Listen.Addr != "" {


What about cli.Listen.TLS?

How do we want to implement it? We don't store CA key anywhere to sign temporary client certificate.

Why do we need a temporary client certificate?.. Starting FerretDB with TLS requires passing all needed TLS files that we can use.
If your question is about "Extended Key Usage", I think we can safely ignore it, at least for now

AlekSi · 2024-06-26T18:14:37Z

cmd/ferretdb/ping.go

+			l.Fatal(err)
+		}
+
+		l.Infof("Ping to %s successful.", u)


We should not log secrets (such as passwords) on level higher than debug

I think there is one too many log I think it was commented before, at the moment see for listen addr
l.Debugf("--listen-addr flag is set. Ping to %s will be performed.", cli.Listen.Addr)
l.Debugf("Pinging %s...", u)
l.Infof("Ping to %s successful.", u)

Can we have one for starting such as Pinging blah blah... and one for success such as Ping success blah blah?

@chilagrow #4364 (comment)
I will send a PR codifying that

@AlekSi Oops, fixed

@chilagrow 2 of the lines you mentioned are only displayed in the debug mode. Normally (on the Info level) we only log the ping success, or when no ping is executed - the skip message.

Shouldn't we avoid displaying password even in debug?

No, everything is a fair game on debug level. First, all information is needed for debugging. Second, we log full requests and responses on debug level that are much more secret than anything else, as they contain actual user data.

chilagrow · 2024-06-27T01:11:25Z

cmd/ferretdb/ping.go

+			l.Fatal(err)
+		}
+
+		l.Infof("Ping to %s successful.", u)


I think there is one too many log I think it was commented before, at the moment see for listen addr
l.Debugf("--listen-addr flag is set. Ping to %s will be performed.", cli.Listen.Addr)
l.Debugf("Pinging %s...", u)
l.Infof("Ping to %s successful.", u)

Can we have one for starting such as Pinging blah blah... and one for success such as Ping success blah blah?

chilagrow · 2024-06-27T01:22:28Z

cmd/ferretdb/ping.go

+		}
+
+		if err = client.Ping(ctx, nil); err != nil {
+			l.Fatal(err)


Let's add a helpful message, logger.Fatal("Ping failed", zap.Error(err)). All other places too.

I don't think I got my idea across, see this PR noisersup#5 of what I mean.

chilagrow

I think I didn't get across what I meant about comments related to logging. Codified the suggestion noisersup#5

chilagrow · 2024-06-28T03:21:48Z

cmd/ferretdb/ping.go

+			l.Fatal(err)
+		}
+
+		l.Infof("Ping to %s successful.", u)


Shouldn't we avoid displaying password even in debug?

chilagrow · 2024-06-28T03:23:00Z

cmd/ferretdb/ping.go

+		}
+
+		if err = client.Ping(ctx, nil); err != nil {
+			l.Fatal(err)


I don't think I got my idea across, see this PR noisersup#5 of what I mean.

noisersup added 21 commits June 10, 2024 15:37

wip

0f36901

wip

9452deb

wip

608c2c1

wip

f4b07db

wip

ddccc71

wip

cac672b

wip

005004a

revert Taskfile

fc81108

simplify

3e515f0

wip

a4da112

wip

64fe2d2

wip

01feeff

wip

da8cdab

Merge branch 'main' into help-approach-2-4246

a652576

wip

aa0c553

wip

44fc844

wip

410d7e7

wip

64b7be7

wip

c5fd92d

wip

aa2d579

wip

1e4b5f9

mergify bot assigned noisersup Jun 14, 2024

noisersup mentioned this pull request Jun 14, 2024

Add stub for the Docker healthcheck #4355

Merged

9 tasks

noisersup added 4 commits June 19, 2024 02:46

merge

77bc798

wip

414255f

wip

ba98480

wip

1c3a9f8

AlekSi reviewed Jun 19, 2024

View reviewed changes

chilagrow reviewed Jun 20, 2024

View reviewed changes

noisersup and others added 4 commits June 21, 2024 13:11

move to ping.go

4fbb0d9

reuse code

2ac564f

be less verbose

e464a80

Merge branch 'main' into improve-ping-logging-4246

98b8ce5

noisersup requested review from AlekSi and chilagrow June 21, 2024 12:00

noisersup mentioned this pull request Jun 21, 2024

Provide Kubernetes probes #4306

Open

rumyantseva reviewed Jun 21, 2024

View reviewed changes

AlekSi added the do not merge PRs that should not be merged label Jun 24, 2024

noisersup and others added 2 commits June 26, 2024 13:52

rm defer

ee2084e

Merge branch 'main' into improve-ping-logging-4246

c7927f2

noisersup requested a review from rumyantseva June 26, 2024 13:08

Merge branch 'main' into improve-ping-logging-4246

f25b83a

AlekSi removed the do not merge PRs that should not be merged label Jun 26, 2024

AlekSi reviewed Jun 26, 2024

View reviewed changes

chilagrow reviewed Jun 27, 2024

View reviewed changes

noisersup added 8 commits June 27, 2024 14:26

move disconnect

994c869

wip

1f9f66e

wip

379fec6

wip

445797d

add tls

4dcdff5

add tls

2b922da

wip

dc9188a

wip

a52a981

noisersup requested review from AlekSi and chilagrow June 27, 2024 15:10

chilagrow reviewed Jun 28, 2024

View reviewed changes

wip

7c357b9

noisersup requested a review from chilagrow June 28, 2024 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Docker healthcheck #4364

Implement Docker healthcheck #4364

noisersup commented Jun 14, 2024 •

edited by AlekSi

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

AlekSi left a comment

mergify bot commented Jun 19, 2024

chilagrow left a comment

rumyantseva left a comment

rumyantseva Jun 21, 2024

rumyantseva Jun 21, 2024 •

edited

Loading

AlekSi Jun 26, 2024

AlekSi Jun 26, 2024

AlekSi Jun 26, 2024

AlekSi Jun 26, 2024

noisersup Jun 27, 2024

AlekSi Jun 27, 2024

AlekSi Jun 26, 2024

chilagrow Jun 27, 2024

AlekSi Jun 27, 2024

noisersup Jun 27, 2024

chilagrow Jun 28, 2024

AlekSi Jun 28, 2024

chilagrow Jun 27, 2024

chilagrow Jun 27, 2024

chilagrow Jun 28, 2024

chilagrow left a comment •

edited

Loading

chilagrow Jun 28, 2024

chilagrow Jun 28, 2024

Implement Docker healthcheck #4364

Are you sure you want to change the base?

Implement Docker healthcheck #4364

Conversation

noisersup commented Jun 14, 2024 • edited by AlekSi Loading

Description

Readiness checklist

codecov bot commented Jun 14, 2024 • edited Loading

Codecov Report

AlekSi left a comment

Choose a reason for hiding this comment

mergify bot commented Jun 19, 2024

chilagrow left a comment

Choose a reason for hiding this comment

rumyantseva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rumyantseva Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chilagrow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisersup commented Jun 14, 2024 •

edited by AlekSi

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

rumyantseva Jun 21, 2024 •

edited

Loading

chilagrow left a comment •

edited

Loading