cloud: Added support to all ExternalStorage ReadFile methods, to raise a sentinel ErrFileDoesNotExist error

[adityamaru]

The ReadFile interface method is responsible for returning a Reader
which can be used to stream data from an external storage source.
There are also instances where we use this method to solely check for
the existence (or absence) of a particular file/object, by attempting
to open a stream. eg: checking for BackupManifest/BackupManifestCheckpoint
before exporting data. Previously, we would treat any error returned
by the storage vendor API as a signal for the file/object not existing.

This change adds logic to catch the native "file does not exist"
errors for each storage provider, and throw a sentinel error to users
of the ReadFile method. This allows for more careful error handling.

Relevant unit tests have also been added.

Release note: None

[cockroach-teamcity]

This change is 

[adityamaru]

I'm still waiting on Azure credentials to ensure I am catching all the relevant 404 errors. The remaining storage providers have been tested.

[miretskiy]

Reviewed 4 of 11 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @miretskiy, and @pbardea)

---

pkg/ccl/backupccl/manifest_handling.go, line 108 at r1 (raw file):

	if err != nil {
		if errors.Is(err, cloud.ErrFileDoesNotExist) {
			//nolint:returnerrcheck

do you know what this linter does?

---

pkg/ccl/backupccl/manifest_handling.go, line 673 at r1 (raw file):

			"%s already contains a %s file",
			readable, BackupManifestName)
	} else if !errors.Is(err, cloud.ErrFileDoesNotExist) {

I think the preference is not to nest if/else chains w/ error handling.
perhaps:
r, err := ....
if err == nil {
... return pgerror
}
if !ErrFileDoesNotExists {
...
}

---

pkg/ccl/backupccl/manifest_handling.go, line 682 at r1 (raw file):

			"%s already contains a %s file (is another operation already in progress?)",
			readable, BackupManifestCheckpointName)
	} else if !errors.Is(err, cloud.ErrFileDoesNotExist) {

same here: avoid nesting if/else chains w/ error handling

---

pkg/storage/cloud/external_storage_test.go, line 220 at r1 (raw file):

		if err := s.Delete(ctx, testingFilename); err != nil {
			t.Fatal(err)
		}

Here and throughout this test:

require.NoError(t, s.Delete(ctx, testingFilename))

---

pkg/storage/cloud/external_storage_test.go, line 250 at r1 (raw file):

		// Attempt to read a file which does not exist.
		_, err = singleFile.ReadFile(ctx, "file_does_not_exist")
		require.Error(t, err, "")

no need for ""

---

pkg/storage/cloud/external_storage_test.go, line 253 at r1 (raw file):

		if !errors.Is(err, ErrFileDoesNotExist) {
			t.Errorf("Expected a file does not exist error but returned %s", err.Error())
		}

you could just do
require.True(t, errors.Is(...))

---

pkg/storage/cloud/http_storage.go, line 353 at r1 (raw file):

		body, _ := ioutil.ReadAll(resp.Body)
		_ = resp.Body.Close()
		return nil, errors.Wrap(ErrFileDoesNotExist, fmt.Sprintf("error response from server: %s %q", resp.Status, body))

does it make sense to keep default: clause, but simply check for 404 inside?
like:
default:
....
err := errors.Errorf("error response from server: %s %q", resp.Status, body)
if resp.StatusCode == 404 {
err = errors.Wrap(ErrFileDoesNotExist, ...)
}
return nil, err

---

pkg/storage/cloud/http_storage_test.go, line 113 at r1 (raw file):

		defer cleanup()
		testExportStore(t, srv.String(), false)
		if expected, actual := 14, files(); expected != actual {

is that suppose to test file not found handling?

[miretskiy]

Reviewed 7 of 11 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @pbardea)

[adityamaru]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @miretskiy and @pbardea)

---

pkg/ccl/backupccl/manifest_handling.go, line 108 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

do you know what this linter does?

I believe it's an escape hatch for when one is swallowing an error. The linter would complain otherwise.

---

pkg/ccl/backupccl/manifest_handling.go, line 673 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

I think the preference is not to nest if/else chains w/ error handling.
perhaps:
r, err := ....
if err == nil {
... return pgerror
}
if !ErrFileDoesNotExists {
...
}

Ack

---

pkg/ccl/backupccl/manifest_handling.go, line 682 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

same here: avoid nesting if/else chains w/ error handling

Done.

---

pkg/storage/cloud/external_storage_test.go, line 220 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

		if err := s.Delete(ctx, testingFilename); err != nil {
			t.Fatal(err)
		}

Here and throughout this test:

require.NoError(t, s.Delete(ctx, testingFilename))

Done.

---

pkg/storage/cloud/external_storage_test.go, line 250 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

no need for ""

Done.

---

pkg/storage/cloud/external_storage_test.go, line 253 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

you could just do
require.True(t, errors.Is(...))

Done.

---

pkg/storage/cloud/http_storage.go, line 353 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

does it make sense to keep default: clause, but simply check for 404 inside?
like:
default:
....
err := errors.Errorf("error response from server: %s %q", resp.Status, body)
if resp.StatusCode == 404 {
err = errors.Wrap(ErrFileDoesNotExist, ...)
}
return nil, err

Done.

---

pkg/storage/cloud/http_storage_test.go, line 113 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

is that suppose to test file not found handling?

I might be misunderstanding the comment but testExportStore contains the test which attempts to read the invalid file. I felt this was the correct place to add a test that would be shared by most of the external storages because other tests in this method do perform simple read/write ops.
Since the test also involves writing/reading a valid file (for sanity), the server encounters a PUT operation which increases the files count.

[miretskiy]

Reviewed 1 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @miretskiy, and @pbardea)

---

pkg/storage/cloud/http_storage.go, line 355 at r2 (raw file):

		err := errors.Errorf("error response from server: %s %q", resp.Status, body)
		if resp.StatusCode == 404 {
			err = errors.Wrap(ErrFileDoesNotExist, "")

just wrap the error you already have (it does have status, etc)

[miretskiy]

Reviewed 2 of 5 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @miretskiy, and @pbardea)

[pbardea]

LGTM with a minor comment.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @miretskiy, and @pbardea)

---

pkg/ccl/backupccl/manifest_handling.go, line 107 at r4 (raw file):

	if err != nil {
		if errors.Is(err, cloud.ErrFileDoesNotExist) {
			//nolint:returnerrcheck

I thinkw e can remove this linter hint? I thought that this linter only checks if the return statement is inside a conditional which checks that an error is not nil. Also, this pattern of using errors.Is as to ignore errors is used elsewhere without the linter comment.

---

pkg/storage/cloud/gcs_storage_test.go, line 80 at r4 (raw file):

	if os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") == "" {
		// This test requires valid GS credential file.
		t.Skip("GOOGLE_APPLICATION_CREDENTIALS env var must be set")

Nice catch.

[adityamaru]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @miretskiy, and @pbardea)

---

pkg/ccl/backupccl/manifest_handling.go, line 107 at r4 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I thinkw e can remove this linter hint? I thought that this linter only checks if the return statement is inside a conditional which checks that an error is not nil. Also, this pattern of using errors.Is as to ignore errors is used elsewhere without the linter comment.

Done.

---

pkg/storage/cloud/http_storage.go, line 355 at r2 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

just wrap the error you already have (it does have status, etc)

Done.

---

pkg/storage/cloud/nodelocal_storage.go, line 91 at r3 (raw file):

if os.IsNotExist(err) || status.Code(err) == codes.NotFound

@miretskiy this is the fix for the offline discussion we had about grpc hiding the syscall error.

[pbardea]

Reviewed 2 of 5 files at r2, 1 of 5 files at r3, 5 of 6 files at r4, 1 of 1 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @miretskiy)

[adityamaru]

TFTR!

bors r+

[craig]

Merge conflict (retrying...)

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)