storage: check error on Engine close #87232

jbowens · 2022-08-31T21:37:03Z

Panic if an error is encountered while closing the Engine. This ensures
unit tests and the like observe errors, especially related to leaked
iterators.

Close #71481.
Close #87507.

Release justification: low-risk bug fixes and non-production code changes
Release note: None

cockroach-teamcity · 2022-08-31T21:37:11Z

This change is

jbowens

There are a few unit tests that leak iterators that I haven't tracked down yet, so they're marked as exempt from this behavior for now. I'm suspicious that there might be a legitimate iterator leak somewhere in here. I'm going to create a GA blocker issue for identifying and resolving these remaining leaks.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)

erikgrinaker

I'm very +1 on the testing aspects of this. However, I am a bit worried that this can cause spurious node crashes when we close engines outside of node shutdown, such as here:

cockroach/pkg/kv/kvserver/store_snapshot.go

Lines 1315 to 1324 in 2dc2da8

    
           // Create an engine to use as a buffer for the empty snapshot. 
        
           eng, err := storage.Open( 
        
           	context.Background(), 
        
           	storage.InMemory(), 
        
           	storage.CacheSize(1<<20 /* 1 MiB */), 
        
           	storage.MaxSize(512<<20 /* 512 MiB */)) 
        
           if err != nil { 
        
           	return err 
        
           } 
        
           defer eng.Close()

Are these errors always severe enough that it warrants a node crash?

Alternatively, we can consider gating this on CrdbTestBuild, and logging an error otherwise.

erikgrinaker · 2022-09-05T19:23:09Z

pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go

+						// TODO(jackson): Track down the iterator leak and
+						// remove the storage.LeaksIteratorsTODO option.
+						engine := storage.NewDefaultInMemForTesting(storage.LeaksIteratorsTODO)


We should definitely track this down before 22.2, in case it's a new leak.

erikgrinaker · 2022-09-05T19:29:46Z

This also reminds me that we'll likely want to use log.Fatalf rather than panic for most crashes like these, since a panic may not cause the node to terminate (it may e.g. have been handled by a recover somewhere in the call stack), and instead limp along in some indeterminate state.

knz · 2022-09-06T06:12:26Z

Good point (also log.Fatal buys us sentry reporting)

jbowens

I'll gate it behind crdb-test for now, but I don't think we can swallow these errors in production either. Ideally, we'd propagate these errors to the call sites, but the Reader interface makes that a little difficult at the moment. Perhaps we can disentangle Close from Reader, so that Engine can still implement Reader but with a Close method that has an error return value.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @jbowens)

jbowens

Tracked down the remaining leaks—all were in test-code only

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)

erikgrinaker

I'll gate it behind crdb-test for now, but I don't think we can swallow these errors in production either.

I generally agree, but I'd like to build some confidence that we won't introduce spurious node crashes over relatively benign issues here. Happy to enable this by default (and consider an error return path) after some baking time.

Tracked down the remaining leaks—all were in test-code only

Nice! I guess we can remove the additional LeaksIteratorsTODO stuff then?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)

erikgrinaker

I generally agree, but I'd like to build some confidence that we won't introduce spurious node crashes over relatively benign issues here. Happy to enable this by default (and consider an error return path) after some baking time.

On second thought, maybe this would be a good point in the release cycle to weed this out by enabling it by default. We still have a couple of months left until the release. You call.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)

jbowens

I guess we can remove the additional LeaksIteratorsTODO stuff then?

I think I accidentally force pushed away my changes from another machine. Fixed.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)

erikgrinaker

Thanks for tracking down the leaks!

erikgrinaker · 2022-09-12T13:23:27Z

pkg/storage/pebble.go

+		if buildutil.CrdbTestBuild {
+			p.logger.Fatalf("error during engine close: %s", err)
+		} else {
+			p.logger.Infof("error during engine close: %s", err)


I'd promote this to an Errorf.

the pebble Logger interface doesn't have an Errorf—updated to stash the logging context on the engine and use log.Errorf

When the crdb_test build flag is provided, fatal the process if an engine Close returns an error. This ensures unit tests and the like observe errors, especially related to leaked iterators. Close cockroachdb#71481. Release justification: low-risk bug fixes and non-production code changes Release note: None

jbowens · 2022-09-13T20:20:13Z

TFTR!

jbowens · 2022-09-14T18:04:35Z

bors r=erikgrinaker

craig · 2022-09-14T19:49:13Z

Build succeeded:

Bazel Essential CI (Cockroach)

jbowens force-pushed the clean-db-close branch 5 times, most recently from 3eeb747 to 630f2fa Compare September 1, 2022 14:03

jbowens marked this pull request as ready for review September 1, 2022 20:53

jbowens requested review from a team as code owners September 1, 2022 20:53

jbowens requested a review from erikgrinaker September 1, 2022 20:53

jbowens force-pushed the clean-db-close branch from 630f2fa to ba7c083 Compare September 1, 2022 20:59

jbowens changed the title ~~storage: check error on engine close~~ storage: check error on Engine close Sep 1, 2022

jbowens commented Sep 1, 2022

View reviewed changes

erikgrinaker reviewed Sep 5, 2022

View reviewed changes

jbowens mentioned this pull request Sep 7, 2022

*: unidentified iterator leaks of engine iterators #87507

Closed

jbowens force-pushed the clean-db-close branch from ba7c083 to c3e9601 Compare September 7, 2022 16:42

jbowens commented Sep 7, 2022

View reviewed changes

jbowens force-pushed the clean-db-close branch from c3e9601 to b9c6321 Compare September 8, 2022 14:48

jbowens commented Sep 8, 2022

View reviewed changes

jbowens added the backport-22.2.x label Sep 8, 2022

jbowens force-pushed the clean-db-close branch from b9c6321 to dba2e27 Compare September 8, 2022 18:13

erikgrinaker reviewed Sep 12, 2022

View reviewed changes

jbowens force-pushed the clean-db-close branch from dba2e27 to 185450e Compare September 12, 2022 13:13

jbowens commented Sep 12, 2022

View reviewed changes

erikgrinaker approved these changes Sep 12, 2022

View reviewed changes

jbowens force-pushed the clean-db-close branch from 185450e to e4a8a43 Compare September 13, 2022 20:18

craig bot merged commit 5eeb769 into cockroachdb:master Sep 14, 2022

blathers-crl bot mentioned this pull request Sep 14, 2022

release-22.2: storage: check error on Engine close #87964

Merged

erikgrinaker mentioned this pull request Sep 27, 2022

kvserver: sync checksum computation with long poll #86591

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: check error on Engine close #87232

storage: check error on Engine close #87232

jbowens commented Aug 31, 2022 •

edited

Loading

cockroach-teamcity commented Aug 31, 2022

jbowens left a comment

erikgrinaker left a comment

erikgrinaker Sep 5, 2022

erikgrinaker commented Sep 5, 2022

knz commented Sep 6, 2022

jbowens left a comment

jbowens left a comment

erikgrinaker left a comment

erikgrinaker left a comment

jbowens left a comment

erikgrinaker left a comment

erikgrinaker Sep 12, 2022

jbowens Sep 13, 2022

jbowens commented Sep 13, 2022

jbowens commented Sep 14, 2022

craig bot commented Sep 14, 2022

	// Create an engine to use as a buffer for the empty snapshot.
	eng, err := storage.Open(
	context.Background(),
	storage.InMemory(),
	storage.CacheSize(1<<20 /* 1 MiB */),
	storage.MaxSize(512<<20 /* 512 MiB */))
	if err != nil {
	return err
	}
	defer eng.Close()

storage: check error on Engine close #87232

storage: check error on Engine close #87232

Conversation

jbowens commented Aug 31, 2022 • edited Loading

cockroach-teamcity commented Aug 31, 2022

jbowens left a comment

Choose a reason for hiding this comment

erikgrinaker left a comment

Choose a reason for hiding this comment

erikgrinaker Sep 5, 2022

Choose a reason for hiding this comment

erikgrinaker commented Sep 5, 2022

knz commented Sep 6, 2022

jbowens left a comment

Choose a reason for hiding this comment

jbowens left a comment

Choose a reason for hiding this comment

erikgrinaker left a comment

Choose a reason for hiding this comment

erikgrinaker left a comment

Choose a reason for hiding this comment

jbowens left a comment

Choose a reason for hiding this comment

erikgrinaker left a comment

Choose a reason for hiding this comment

erikgrinaker Sep 12, 2022

Choose a reason for hiding this comment

jbowens Sep 13, 2022

Choose a reason for hiding this comment

jbowens commented Sep 13, 2022

jbowens commented Sep 14, 2022

craig bot commented Sep 14, 2022

jbowens commented Aug 31, 2022 •

edited

Loading