boltdb shipper download failure handling and some refactorings #2156

sandeepsukhani · 2020-06-02T07:24:09Z

What this PR does / why we need it:
Previously boltdb shipper was using context from request for initial downloading of files which means if a request times out download operation would be cancelled as well and we will continue using whatever files were downloading until the context got cancelled. This would result in incorrect results until files get resynced. This PR takes care of it by passing the background context instead. This means files would continue to download even though request got timed out. We would anyways need those files for serving next requests.

This PR also does some code refactoring to make things clearer at some of the places and support the changes which handle the download failure issue.

codecov-commenter · 2020-06-02T08:14:19Z

Codecov Report

Merging #2156 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #2156      +/-   ##
==========================================
- Coverage   62.14%   62.13%   -0.02%     
==========================================
  Files         154      155       +1     
  Lines       12457    12509      +52     
==========================================
+ Hits         7741     7772      +31     
- Misses       4108     4131      +23     
+ Partials      608      606       -2

Impacted Files	Coverage Δ
pkg/storage/stores/local/shipper.go	`51.53% <40.62%> (-2.67%)`	⬇️
pkg/storage/stores/local/filescollection.go	`55.76% <55.76%> (ø)`
pkg/storage/stores/local/downloads.go	`64.51% <86.48%> (+1.31%)`	⬆️
pkg/storage/stores/local/metrics.go	`100.00% <100.00%> (ø)`
pkg/querier/queryrange/downstreamer.go	`95.87% <0.00%> (-2.07%)`	⬇️
pkg/logql/evaluator.go	`92.30% <0.00%> (-0.42%)`	⬇️
pkg/promtail/targets/filetarget.go	`70.48% <0.00%> (+1.80%)`	⬆️
pkg/promtail/targets/tailer.go	`78.40% <0.00%> (+4.54%)`	⬆️

pkg/storage/stores/local/shipper.go

pkg/storage/stores/local/filescollection.go

pkg/storage/stores/local/shipper.go

owen-d

I have a lot of questions about this PR. Some of them are due to my unfamiliarity with logic in this package, but others are concerns for some of the design choices taken. Here's a few high level things that help make code more intuitive, safe, and extensible in my opinion:

In general struct methods should be safe for use. If you're wrapping a state machine, the method shouldn't panic, etc, regardless of which state is held internally. You can return errors when needed, etc.

Example:

func (fc *filesCollection) cleanupFile(fileName string) error {
	boltdbFile := fc.files[fileName].boltdb
	filePath := boltdbFile.Path()

	if err := boltdbFile.Close(); err != nil {
		return err
	}

	delete(fc.files, fileName)

	return os.Remove(filePath)
}

This is dangerous because it depends on the file existing in the map and being non-nil. Furthermore it also depends on the embedded *bbolt.DB from being non-nil. If any of these are not true, it panics.

Locks should be as unobtrusive as possible and ideally be colocated with the data structures they protect. They should avoid forcing consumers of the library from maintaining the library structure's state. For instance, the lock embedded in the *filesCollection should be used by it's own methods and not expect consumers of the collection to manage it's state. Instead of

fc.Lock()
fc.iter(...)
fc.Unlock()

, the fc.iter method shouldl take care of the locking/unlocking logic.

Struct methods shouldn't require you to know the exact implementation internally. Instead, we should be able to read the exported functions/method names and start using it. This is important in order to maintain an extensible codebase. We want to make sure that adding functionality in one pkg shouldn't require that you read every dependency that pkg has (and every dependency those dependencies have, recursively) in order to work on it. We want to make adding to the codebase an O(1) or O(n log n) operation, not O(n) or O(n^2).

owen-d · 2020-06-05T11:49:08Z

pkg/storage/stores/local/filescollection.go

+		return nil, fc.err
+	}
+
+	return fc.files[fileName], nil


I think this should return an error if the file does not exist -- (nil, nil) seems like an antipattern here and the consumer would need to check both file & error on every call.

pkg/storage/stores/local/filescollection.go

owen-d · 2020-06-05T15:12:10Z

pkg/storage/stores/local/filescollection.go

+	fc.lastUsedAt = time.Now()
+}
+
+func (fc *filesCollection) lastUsedTime() time.Time {


It seems a little weird to have private getters that reference private fields, but it doesn't affect functionality. Since private methods are pkg-scoped anyways, these functions could all be replaced via fc.lastUsedAt.

owen-d · 2020-06-05T15:23:20Z

pkg/storage/stores/local/shipper.go

@@ -260,20 +249,14 @@ func (s *Shipper) syncLocalWithStorage(ctx context.Context) (err error) {

 // deleteFileFromCache removes a file from cache.
 // It takes care of locking the filesCollection, closing the boltdb file and removing the file from cache
-func (s *Shipper) deleteFileFromCache(period, uploader string, fc *filesCollection) error {
+func (s *Shipper) deleteFileFromCache(uploader string, fc *filesCollection) error {


It seems like an antipattern to have this method just call lock/unlock then defer to the unexported cleanupFile method. Why not instead have the *filesCollection manage it's own mutex?

owen-d · 2020-06-05T15:29:45Z

pkg/storage/stores/local/shipper.go

 }

-func (s *Shipper) forEach(ctx context.Context, period string, callback func(db *bbolt.DB) error) error {
+func (s *Shipper) forEach(period string, callback func(db *bbolt.DB) error) error {
 	s.downloadedPeriodsMtx.RLock()


Can you explain what's happening here? It tries to see if the period exists twice in a row? Why do we check twice hoping it's changed?

It works in following manner:

Check with read-only lock whether we can find the entry.

If not with write lock check whether it got created by some other goroutine due to a race because multiple goroutines might be trying to acquire the write lock and create it.

If it is still not there create it.

It works in following manner:
Check with read-only lock whether we can find the entry.
If not with write lock check whether it got created by some other goroutine due to a race because multiple goroutines might be trying to acquire the write lock and create it.
If it is still not there create it.

Thanks. Would you mind adding this as a comment to the code here?

owen-d · 2020-06-05T15:34:03Z

pkg/storage/stores/local/shipper.go

+				defer s.downloadedPeriodsMtx.Unlock()
+
+				// cleaning up fc since it is in invalid state anyways.
+				delete(s.downloadedPeriods, period)


Instead of deleting it here, we should only add it to the downloadedPeriods after it successfully downloads.

This is done like this to hold other requests needing the same index which is not downloaded yet to wait on mutex to get unlocked. The first request which gets to hold a lock gets to download the files. Does it make sense?

pkg/storage/stores/local/shipper.go

pkg/storage/stores/local/downloads.go

owen-d · 2020-06-05T15:56:51Z

pkg/storage/stores/local/downloads.go

 			toDownload = append(toDownload, object)
 		}
 	}

-	for uploader := range fc.files {
+	err = fc.iter(func(uploader string, df *downloadedFile) error {
 		if _, isOK := listedUploaders[uploader]; !isOK {
 			toDelete = append(toDelete, uploader)


this means we'd delete when if uploader == s.uploader, is this correct?

…s suggested in PR review

owen-d

Thanks for all these changes. I have a few nits, but I think it's getting a lot closer. Nice work!

owen-d · 2020-06-11T18:52:02Z

pkg/storage/stores/local/downloads.go


+	toDownload, toDelete, err := fc.checkStorageForUpdates(ctx)
 	if err != nil {
 		return err
 	}

 	for _, storageObject := range toDownload {


I think there should be some bounded concurrency mechanism for downloads.

It is in my todo and I think it deserves a separate PR. What do you think?

owen-d · 2020-06-11T19:21:32Z

pkg/storage/stores/local/downloads.go

 	for _, uploader := range toDelete {
-		err := s.deleteFileFromCache(period, uploader, fc)
+		err := fc.cleanupFile(uploader)


I think this would be better expressed as fc.CleanupFiles(toDelete...), which could then be refactored like

func (fc *FilesCollection) CleanupAllFiles(files ...string)

This cleanup function already takes the lock in the exact same way, so we'd be able to increase code reuse and be more idiomatic.

owen-d · 2020-06-11T19:22:27Z

pkg/storage/stores/local/filescollection.go

+	return nil
+}
+
+func (fc *FilesCollection) CleanupAllFiles() error {


As expressed earlier, I think this can be changed to
func (fc *FilesCollection) CleanupFiles(files ...string)

owen-d · 2020-06-11T19:32:59Z

pkg/storage/stores/local/downloads.go

@@ -212,18 +209,18 @@ func (s *Shipper) downloadFilesForPeriod(ctx context.Context, period string, fc

 		totalFilesSize += stat.Size()



I think the lock can be held here rather than implicitly require the caller to do so.

I am taking the lock from the caller because this function is called in a goroutine. The goroutine could be waiting to be scheduled and some other goroutine could take the lock. While I am checking the ready channel in reads but it would be error-prone to rely only on that because someone else might forget to check the channel. The contract is simple now, all the public methods take care of locking.

owen-d · 2020-06-11T19:35:10Z

pkg/storage/stores/local/downloads.go

+
+			// cleaning up files due to error to avoid returning invalid results.
+			for fileName := range fc.files {
+				if err := fc.cleanupFile(fileName); err != nil {


Locks aren't obtained for cleanup (they technically are when this is called via NewFilesCollection, but I think it's better design to use the CleanupFIles(filename) method I proposed earlier).

Yes, as mentioned earlier, all the public methods take care of locking. I did it this way to do all the operations of initializing the filesCollection or cleaning up due to failures without releasing the locks. This way it is straightforward and less error-prone.

pkg/storage/stores/local/filescollection.go

owen-d · 2020-06-11T19:41:29Z

pkg/storage/stores/local/shipper.go

 }

-func (s *Shipper) forEach(ctx context.Context, period string, callback func(db *bbolt.DB) error) error {
+func (s *Shipper) forEach(period string, callback func(db *bbolt.DB) error) error {
 	s.downloadedPeriodsMtx.RLock()


It works in following manner:
Check with read-only lock whether we can find the entry.
If not with write lock check whether it got created by some other goroutine due to a race because multiple goroutines might be trying to acquire the write lock and create it.
If it is still not there create it.

Thanks. Would you mind adding this as a comment to the code here?

pkg/storage/stores/local/filescollection.go

slim-bean

Sandeep and I had a discussion around this PR, there are numerous other changes coming into this same space soon to chnage how index files are uploaded (no longer will we re-upload a file, instead only new files).

We agreed to merge this PR as is and address some of the remaining concerns with that work:

Add more testing, and add it up front when the development is done
Separate the upload/download functionality from the index type (shipper/receiver shouldn't care if it's boltdb or some other index)
Re-evaluate the locks in use to make sure API calls on a struct are side-effect free.

Thanks Cyril/Owen/Sandeep for the discussions and support around this PR!

pull-request-size bot added the size/L label Jun 2, 2020

sandeepsukhani force-pushed the boltdb-shipper-fixes branch from 598007e to b92a54a Compare June 2, 2020 08:02

sandeepsukhani requested review from cyriltovena and slim-bean June 4, 2020 08:41

cyriltovena reviewed Jun 5, 2020

View reviewed changes

pkg/storage/stores/local/shipper.go Outdated Show resolved Hide resolved

cyriltovena reviewed Jun 5, 2020

View reviewed changes

pkg/storage/stores/local/filescollection.go Outdated Show resolved Hide resolved

cyriltovena reviewed Jun 5, 2020

View reviewed changes

pkg/storage/stores/local/shipper.go Outdated Show resolved Hide resolved

cyriltovena reviewed Jun 5, 2020

View reviewed changes

pkg/storage/stores/local/shipper.go Outdated Show resolved Hide resolved

owen-d requested changes Jun 5, 2020

View reviewed changes

sandeepsukhani requested review from owen-d and cyriltovena June 10, 2020 05:44

sandeepsukhani added 2 commits June 10, 2020 16:02

boltdb shipper download failure handling and some refactorings

e74f62d

attempt to contain mutex handling in filescollection and other change…

a35c4cc

…s suggested in PR review

sandeepsukhani force-pushed the boltdb-shipper-fixes branch from b624e5f to a35c4cc Compare June 10, 2020 10:33

owen-d reviewed Jun 11, 2020

View reviewed changes

add a comment on checking for existence of files collection

5b0ba1f

slim-bean reviewed Jun 17, 2020

View reviewed changes

pkg/storage/stores/local/filescollection.go Outdated Show resolved Hide resolved

slim-bean reviewed Jun 17, 2020

View reviewed changes

pkg/storage/stores/local/filescollection.go Outdated Show resolved Hide resolved

slim-bean reviewed Jun 17, 2020

View reviewed changes

pkg/storage/stores/local/filescollection.go Outdated Show resolved Hide resolved

changes suggested from PR review

a764871

sandeepsukhani requested a review from slim-bean June 18, 2020 12:42

slim-bean approved these changes Jun 18, 2020

View reviewed changes

owen-d approved these changes Jun 18, 2020

View reviewed changes

sandeepsukhani merged commit 9ab3800 into grafana:master Jun 22, 2020

sandeepsukhani deleted the boltdb-shipper-fixes branch August 21, 2020 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boltdb shipper download failure handling and some refactorings #2156

boltdb shipper download failure handling and some refactorings #2156

sandeepsukhani commented Jun 2, 2020

codecov-commenter commented Jun 2, 2020 •

edited

Loading

owen-d left a comment

owen-d Jun 5, 2020

owen-d Jun 5, 2020

owen-d Jun 5, 2020

owen-d Jun 5, 2020

sandeepsukhani Jun 5, 2020

owen-d Jun 11, 2020 •

edited

Loading

owen-d Jun 5, 2020

sandeepsukhani Jun 5, 2020

owen-d Jun 5, 2020

owen-d left a comment

owen-d Jun 11, 2020

sandeepsukhani Jun 12, 2020

owen-d Jun 11, 2020

owen-d Jun 11, 2020

owen-d Jun 11, 2020

sandeepsukhani Jun 13, 2020

owen-d Jun 11, 2020

sandeepsukhani Jun 13, 2020

owen-d Jun 11, 2020 •

edited

Loading

slim-bean left a comment

		@@ -212,18 +209,18 @@ func (s *Shipper) downloadFilesForPeriod(ctx context.Context, period string, fc

		totalFilesSize += stat.Size()

boltdb shipper download failure handling and some refactorings #2156

boltdb shipper download failure handling and some refactorings #2156

Conversation

sandeepsukhani commented Jun 2, 2020

codecov-commenter commented Jun 2, 2020 • edited Loading

Codecov Report

owen-d left a 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

owen-d Jun 11, 2020 • 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

owen-d left a 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

owen-d Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

slim-bean left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 2, 2020 •

edited

Loading

owen-d Jun 11, 2020 •

edited

Loading

owen-d Jun 11, 2020 •

edited

Loading