feat: Wait shutdown-grace-seconds to flush async db writes to disk #17094

chillyvee · 2023-07-23T12:39:55Z

Description

server/start.go by default exits the main() routine before databases have a chance to flush async writes to disk (such as pebbledb)

Since there is no master registry of all open databases that can be used for orderly flushing of all open databases, databases should be allowed to catch interrupt signals (like the main daemon) and be given time to flush to disk to avoid data loss.

Add Close() for snapshot manager
Add application.db and snapshots/metadata.db to BaseApp.Close()

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

server/start.go

yihuang · 2023-07-23T13:18:04Z

You can probably implement the app.Close method, which is called in graceful shutdown.

chillyvee · 2023-07-24T12:50:32Z

You can probably implement the app.Close method, which is called in graceful shutdown.

Thank you for the comment. Will try it.

chillyvee · 2023-07-24T14:15:14Z

Do you know where the database handle to data/evidence.db is held?

store/snapshots/manager.go

baseapp/baseapp.go

alexanderbez

Since the server package's signal capturing mechanism executes a callback function that performs all cleanup, instead the app should pass what it wants cleaned up to that call IMO

chillyvee · 2023-07-24T20:38:51Z

Since the server package's signal capturing mechanism executes a callback function that performs all cleanup, instead the app should pass what it wants cleaned up to that call IMO

Are you referring to cleanupFn from func startApp(svrCtx *Context, appCreator types.AppCreator, opts StartCmdOptions) (app types.Application, cleanupFn func(), err error) ?

A callback to add items to clean up makes sense, but since start.go/startApp opens db, should start.go clean itself up rather than relying on the chain app.go to ask?

cosmos-sdk/baseapp/baseapp.go creates the snapshotmanager, so is it better that cosmos-sdk clean that up rather than rely on a callback supplied by the application chain?

if app/app.go opens a new database, then cleanupFn should clean up that new database that cosmos-sdk does not manage

chillyvee · 2023-08-05T13:08:59Z

@alexanderbez Confirming whether you agree that:

it is okay for cosmos-sdk to close the database handles that it opens (rather than relying on the app.go chain logic)
app specific databases be closed by request from the app

alexanderbez · 2023-08-05T17:08:21Z

I would say this makes sense to me, yes!

chillyvee · 2023-08-06T01:28:36Z

Following guidance, cometbft closes databases it opens OnStop:
cometbft/cometbft#1210

chillyvee · 2023-08-06T01:30:40Z

github dependency test does not see the new snapshot manager Close() method

local go test succeeds with manager.Close() call

app.snapshotManager.Close undefined (type *snapshots.Manager has no field or method Close)

defined here:
https://github.com/cosmos/cosmos-sdk/pull/17094/files#diff-127e2148fdc244ca5bc1bf693f0d9d2020a46b6bf32cc1377801b6c49ceb9205R555

julienrbrt · 2023-08-06T07:35:45Z

github dependency test does not see the new snapshot manager Close() method

This is because store it is own go.mod. You could split the PR and scope one on store changes the the other one for integrating the changes (using then a pseudo version from main). That's better than adding replaces imho.

julienrbrt · 2023-08-06T07:36:27Z

store/snapshots/manager.go

@@ -550,3 +550,9 @@ func (m *Manager) snapshot(height int64) {
 		m.logger.Debug("pruned state snapshots", "pruned", pruned)
 	}
 }
+
+// close snapshot database
+func (m *Manager) Close() {


Lets add a changelog under store/CHANGELOG.md for this.

chillyvee · 2023-08-06T10:56:48Z

snapshot manager Close() split into #17294

chillyvee · 2023-08-06T11:01:36Z

This PR is rebased on #17294

pseudo version for go.mod after #17294 merge?

julienrbrt · 2023-08-06T13:27:20Z

This PR is rebased on #17294

pseudo version for go.mod after #17294 merge?

👍
Yes, pseudo version from main.

…disk

chillyvee · 2023-08-23T21:03:00Z

Hi! Could you fix the conflicts and look at the last comment?

Conflict resolved.

Last comment from yihuang?

julienrbrt · 2023-08-24T07:52:27Z

Hi! Could you fix the conflicts and look at the last comment?

Conflict resolved.

Last comment from yihuang?

Yes

chillyvee · 2023-08-24T08:22:12Z

Hi! Could you fix the conflicts and look at the last comment?

Conflict resolved.
Last comment from yihuang?

Yes

Let's wait for an update from @yihuang to see how soon a root cause solution is available.

If a solution isn't available soon, it might be good to use the grace period now and reduce data loss on program exit (this is a current problem). We can remove the grace feature later.

baseapp/baseapp.go

yihuang · 2023-09-05T03:11:36Z

Hi! Could you fix the conflicts and look at the last comment?

Conflict resolved.
Last comment from yihuang?

Yes

Let's wait for an update from @yihuang to see how soon a root cause solution is available.

If a solution isn't available soon, it might be good to use the grace period now and reduce data loss on program exit (this is a current problem). We can remove the grace feature later.

sorry for the delay, our chain binary still uses sdk 0.46, which is known to be not good at graceful shutdown.
but I just tested with simd in latest cosmos-sdk, it's very stable that app.Close is always called when kill the node, I also added several seconds of time.Sleep in it, and it'll wait for it to return, there's big cleanup regarding the graceful shutdown has done in recent sdk.

chillyvee · 2023-09-05T13:15:40Z

I just tested with simd in latest cosmos-sdk, it's very stable that app.Close is always called when kill the node, I also added several seconds of time.Sleep in it, and it'll wait for it to return, there's big cleanup regarding the graceful shutdown has done in recent sdk.

@yihuang Do you think it's better to remove the new grace-seconds and only keep the DB close calls in baseapp/baseapp.go Close() ?

Is there any way to backport the Close()/Cleanup so that previous SDK versions can shutdown properly? Or must all chains upgrade to 0.49/0.50 to shutdown properly?

yihuang · 2023-09-05T13:57:09Z

I just tested with simd in latest cosmos-sdk, it's very stable that app.Close is always called when kill the node, I also added several seconds of time.Sleep in it, and it'll wait for it to return, there's big cleanup regarding the graceful shutdown has done in recent sdk.

@yihuang Do you think it's better to remove the new grace-seconds and only keep the DB close calls in baseapp/baseapp.go Close() ?

I think so, the issue probably don't reproduce in main branch, but I guess we can add this in 0.46 backport if is reproduce there?

Is there any way to backport the Close()/Cleanup so that previous SDK versions can shutdown properly? Or must all chains upgrade to 0.49/0.50 to shutdown properly?

I did an try before, but there are too many API breaking changes to be accepted: #16203

chillyvee · 2023-09-09T12:10:53Z

I just tested with simd in latest cosmos-sdk, it's very stable that app.Close is always called when kill the node, I also added several seconds of time.Sleep in it, and it'll wait for it to return, there's big cleanup regarding the graceful shutdown has done in recent sdk.

@yihuang Do you think it's better to remove the new grace-seconds and only keep the DB close calls in baseapp/baseapp.go Close() ?

I think so, the issue probably don't reproduce in main branch, but I guess we can add this in 0.46 backport if is reproduce there?

How about we move the db close calls in baseapp.go to a new PR to close the databases on exit.

This PR can be just for the shutdown-grace backport for versions that don't shutdown gracefully like 0.45/0.46/0.47(juno)

yihuang · 2023-09-09T15:09:53Z

I just tested with simd in latest cosmos-sdk, it's very stable that app.Close is always called when kill the node, I also added several seconds of time.Sleep in it, and it'll wait for it to return, there's big cleanup regarding the graceful shutdown has done in recent sdk.

@yihuang Do you think it's better to remove the new grace-seconds and only keep the DB close calls in baseapp/baseapp.go Close() ?

I think so, the issue probably don't reproduce in main branch, but I guess we can add this in 0.46 backport if is reproduce there?

How about we move the db close calls in baseapp.go to a new PR to close the databases on exit.

This PR can be just for the shutdown-grace backport for versions that don't shutdown gracefully like 0.45/0.46/0.47(juno)

Yeah, sounds good to me.

chillyvee · 2023-09-10T06:16:23Z

How about we move the db close calls in baseapp.go to a new PR to close the databases on exit.
This PR can be just for the shutdown-grace backport for versions that don't shutdown gracefully like 0.45/0.46/0.47(juno)

Yeah, sounds good to me.

Moved database close calls to #17667

chillyvee · 2023-09-10T06:19:49Z

This PR can be just for the shutdown-grace backport for versions that don't shutdown gracefully like 0.45/0.46/0.47(juno)

server/start.go

CHANGELOG.md

julienrbrt · 2023-11-10T09:16:47Z

@Mergifyio backport release/v0.50.x

mergify · 2023-11-10T09:16:52Z

backport release/v0.50.x

✅ Backports have been created

#18431 feat: Wait shutdown-grace-seconds to flush async db writes to disk (backport #17094) has been created for branch release/v0.50.x but encountered conflicts

…17094) Co-authored-by: Julien Robert <julien@rbrt.fr> (cherry picked from commit c92b257) # Conflicts: # CHANGELOG.md

…ackport #17094) (#18431) Co-authored-by: Chill Validation <92176880+chillyvee@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>

chillyvee requested a review from a team as a code owner July 23, 2023 12:39

github-advanced-security bot found potential problems Jul 23, 2023

View reviewed changes

server/start.go Fixed Show fixed Hide fixed

github-actions bot added the C:Store label Jul 24, 2023

github-advanced-security bot found potential problems Jul 24, 2023

View reviewed changes

store/snapshots/manager.go Fixed Show fixed Hide fixed

baseapp/baseapp.go Fixed Show fixed Hide fixed

chillyvee force-pushed the cv_servergraceful branch from 0c754e7 to 864ef99 Compare July 24, 2023 14:33

alexanderbez reviewed Jul 24, 2023

View reviewed changes

julienrbrt reviewed Aug 6, 2023

View reviewed changes

chillyvee force-pushed the cv_servergraceful branch from a83f171 to 593f240 Compare August 6, 2023 11:00

chillyvee force-pushed the cv_servergraceful branch 2 times, most recently from 6a91d0f to 398b357 Compare August 7, 2023 12:40

chillyvee added 7 commits August 7, 2023 14:47

CV Wait shutdown-grace-seconds for async database writes to flush to …

6079dc0

…disk

CV app.Close closes application and snapshot db

e3904e8

CV Add shutdown-grace-seconds changelog and update comments

b579c37

CV new start call format

be648d6

CV discard snapshot manager error

f543ae9

CV document database opens for baseapp.Close

79c2b97

CV updated store snapshot manager close

5a73107

chillyvee force-pushed the cv_servergraceful branch from 398b357 to 5a73107 Compare August 7, 2023 12:48

alexanderbez reviewed Aug 24, 2023

View reviewed changes

baseapp/baseapp.go Outdated Show resolved Hide resolved

baseapp/baseapp.go Outdated Show resolved Hide resolved

chillyvee added 3 commits September 4, 2023 12:37

CV use cockroachdb errors join

2d23528

Merge remote-tracking branch 'origin/main' into cv_servergraceful

688b8bf

CV baseapp shutdown-grace changelog update

1cdf3b9

chillyvee mentioned this pull request Sep 10, 2023

feat: CV Close databases opened by cosmos-sdk during BaseApp shutdown #17667

Merged

20 tasks

CV remove db close calls moved to PR 17667

71b62eb

julienrbrt added this pull request to the merge queue Sep 15, 2023

julienrbrt removed this pull request from the merge queue due to a manual request Sep 15, 2023

julienrbrt reviewed Sep 15, 2023

View reviewed changes

server/start.go Outdated Show resolved Hide resolved

server/start.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

Apply suggestions from code review

942d5f4

julienrbrt enabled auto-merge September 15, 2023 07:49

julienrbrt added this pull request to the merge queue Sep 15, 2023

Merged via the queue into cosmos:main with commit c92b257 Sep 15, 2023
44 of 45 checks passed

julienrbrt removed the TODO:backport-v0.50.1 label Nov 10, 2023

mergify bot pushed a commit that referenced this pull request Nov 10, 2023

feat: Wait shutdown-grace-seconds to flush async db writes to disk (#…

a6e09f0

…17094) Co-authored-by: Julien Robert <julien@rbrt.fr> (cherry picked from commit c92b257) # Conflicts: # CHANGELOG.md

mergify bot mentioned this pull request Nov 10, 2023

feat: Wait shutdown-grace-seconds to flush async db writes to disk (backport #17094) #18431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Wait shutdown-grace-seconds to flush async db writes to disk #17094

feat: Wait shutdown-grace-seconds to flush async db writes to disk #17094

chillyvee commented Jul 23, 2023 •

edited

Loading

yihuang commented Jul 23, 2023

chillyvee commented Jul 24, 2023

chillyvee commented Jul 24, 2023

alexanderbez left a comment

chillyvee commented Jul 24, 2023 •

edited

Loading

chillyvee commented Aug 5, 2023

alexanderbez commented Aug 5, 2023

chillyvee commented Aug 6, 2023

chillyvee commented Aug 6, 2023 •

edited

Loading

julienrbrt commented Aug 6, 2023

julienrbrt Aug 6, 2023

chillyvee Aug 6, 2023

chillyvee commented Aug 6, 2023

chillyvee commented Aug 6, 2023 •

edited

Loading

julienrbrt commented Aug 6, 2023

chillyvee commented Aug 23, 2023

julienrbrt commented Aug 24, 2023

chillyvee commented Aug 24, 2023

yihuang commented Sep 5, 2023 •

edited

Loading

chillyvee commented Sep 5, 2023 •

edited

Loading

yihuang commented Sep 5, 2023 •

edited

Loading

chillyvee commented Sep 9, 2023

yihuang commented Sep 9, 2023

chillyvee commented Sep 10, 2023

chillyvee commented Sep 10, 2023

julienrbrt commented Nov 10, 2023

mergify bot commented Nov 10, 2023 •

edited

Loading

feat: Wait shutdown-grace-seconds to flush async db writes to disk #17094

feat: Wait shutdown-grace-seconds to flush async db writes to disk #17094

Conversation

chillyvee commented Jul 23, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

yihuang commented Jul 23, 2023

chillyvee commented Jul 24, 2023

chillyvee commented Jul 24, 2023

alexanderbez left a comment

Choose a reason for hiding this comment

chillyvee commented Jul 24, 2023 • edited Loading

chillyvee commented Aug 5, 2023

alexanderbez commented Aug 5, 2023

chillyvee commented Aug 6, 2023

chillyvee commented Aug 6, 2023 • edited Loading

julienrbrt commented Aug 6, 2023

julienrbrt Aug 6, 2023

Choose a reason for hiding this comment

chillyvee Aug 6, 2023

Choose a reason for hiding this comment

chillyvee commented Aug 6, 2023

chillyvee commented Aug 6, 2023 • edited Loading

julienrbrt commented Aug 6, 2023

chillyvee commented Aug 23, 2023

julienrbrt commented Aug 24, 2023

chillyvee commented Aug 24, 2023

yihuang commented Sep 5, 2023 • edited Loading

chillyvee commented Sep 5, 2023 • edited Loading

yihuang commented Sep 5, 2023 • edited Loading

chillyvee commented Sep 9, 2023

yihuang commented Sep 9, 2023

chillyvee commented Sep 10, 2023

chillyvee commented Sep 10, 2023

julienrbrt commented Nov 10, 2023

mergify bot commented Nov 10, 2023 • edited Loading

✅ Backports have been created

chillyvee commented Jul 23, 2023 •

edited

Loading

chillyvee commented Jul 24, 2023 •

edited

Loading

chillyvee commented Aug 6, 2023 •

edited

Loading

chillyvee commented Aug 6, 2023 •

edited

Loading

yihuang commented Sep 5, 2023 •

edited

Loading

chillyvee commented Sep 5, 2023 •

edited

Loading

yihuang commented Sep 5, 2023 •

edited

Loading

mergify bot commented Nov 10, 2023 •

edited

Loading