Skip to content

Commit 2669c06

Browse files
Kaan Yaltimergify[bot]
Kaan Yalti
authored and
mergify[bot]
committed
Enhancement/5235 insufficient disk handling retry shows underlying error (#9122)
* enhancement(5235): added stdlib wrapper functions for testability enhancement(5235): added comment in stdlib wrappers enhancement(5235): added license header in stdlib wrappers * enhancement(5235): using stdlib wrappers in http downloader enhancement(5235): added comments explaining why stdlib wrappers are used in http downloader * enhancement(5235): using stdlib wrappers in fs downloader enhancement(5235): added comments in fs downloader explaining why stdlib wrappers are used * enhancement(5235): added stlib mocker utils enhancement(5235): ran mage update enhancement(5235): updated the stdlib wrapper mocker to use map instead of switch * enhancement(5235): wrapping errors in http downloader * enhancement(5235): wrapping errors in fs downloader * enhancement(5235): added disk space error and relevant tests * enhancement(5235): added diskspace error check in http downloader before calling reporter enhancement(5235): added comment in http downloader explaining why diskpace error chec is called before the download progress reporter call enhancement(5235): instead of overwriting the error we are using a copy to report. the returned error is now wrapped in the htttp downloadfile function * enhancement(5235): added http downloader test for disk space error enhancement(5235): using stdlib mocker in http donwloader test enhancement(5235): removed unnecessary fmt enhancement(5235): added state message assertion in http downloader test enhancement(5235): using common stdlib mock util in http downloader test enhancement(5235): updated http downloader test * enhancement(5235): added fs downloader disk space error tests enhancement(5235): using common stdlib mock util in fs downloader * enhancement(5235): replaced errors.New with fmt.Errorf in step_download * enhancement(5235): wrapping error in backoff.Permanent in download with retries if the error is a diskspace error. enhancement(5235): added downloaderrors import in step download * enhancement(5235): added insufficient disk space test case in step download tests * enhancement(5235): added archive helper functions in upgrade tests enhancement(5235): added comments for the archive helper functions enhancement(5235): added archive helper functions in upgrade tests enhancement(5235): added comments for the archive helper functions enhancement(5235): buildArchiveFiles archive helper function * enhancement(5235): added deferred error handler in upgrade function to add insufficient disk space error into the error chain enhancement(5235): added comments for the deferred error handler in upgrade func * enhancement(5235): added upgrader test for download error handling enhancement(5235): updated the download error handling test, added additional assertions in mocked functions to make sure that they are called enhancement(5235): added comments explaining steps taken in upgrader test enhancement(5235): using common stdlib mock util in upgrade test enhancement(5235): removed error string comparison * enhancement(5235): remove stdlib wrappers and mock utils * enhancement(5235): add stdlib funcs in http downloader struct, update http downloader tests * enhancement(5235): add stdlib funcs in fs downloader struct and update tests * enhancement(5235): separated artifact downloader from upgrader, updated relevant tests * enhancement(5235): added artifact downloader interface, updated SetClient function to set downloader fleet server uri. Updated upgrader tests * enhancement(5235): fix fs downloader tests * enhancement(5235): added comment in upgrader struct * enhancement(5235): refactored upgrade test for readabiltiy * enhancement(5235): updated disk space error check function to return false for ErrInsufficientDiskSpace * enhancement(5235): fixed download with retries error handling test case (cherry picked from commit 134b3de)
1 parent 9a515c4 commit 2669c06

File tree

12 files changed

+502
-57
lines changed

12 files changed

+502
-57
lines changed

‎internal/pkg/agent/application/upgrade/artifact/download/errors/disk_space_errors.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
2+
// or more contributor license agreements. Licensed under the Elastic License 2.0;
3+
// you may not use this file except in compliance with the Elastic License 2.0.
4+
5+
package errors
6+
7+
import "errors"
8+
9+
var ErrInsufficientDiskSpace = errors.New("insufficient disk space")
10+
11+
func IsDiskSpaceError(err error) bool {
12+
for _, osErr := range OS_DiskSpaceErrors {
13+
if errors.Is(err, osErr) {
14+
return true
15+
}
16+
}
17+
18+
return false
19+
}

‎internal/pkg/agent/application/upgrade/artifact/download/errors/disk_space_errors_other.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
2+
// or more contributor license agreements. Licensed under the Elastic License 2.0;
3+
// you may not use this file except in compliance with the Elastic License 2.0.
4+
5+
//go:build !windows
6+
7+
package errors
8+
9+
import "syscall"
10+
11+
var OS_DiskSpaceErrors = []error{
12+
syscall.ENOSPC,
13+
syscall.EDQUOT,
14+
}

‎internal/pkg/agent/application/upgrade/artifact/download/errors/disk_space_errors_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
2+
// or more contributor license agreements. Licensed under the Elastic License 2.0;
3+
// you may not use this file except in compliance with the Elastic License 2.0.
4+
5+
package errors
6+
7+
import (
8+
goerrors "errors"
9+
"fmt"
10+
"testing"
11+
12+
"github.com/stretchr/testify/require"
13+
14+
agentErrors "github.com/elastic/elastic-agent/internal/pkg/agent/errors"
15+
)
16+
17+
func TestIsDiskSpaceError(t *testing.T) {
18+
for _, err := range OS_DiskSpaceErrors {
19+
testCases := map[string]struct {
20+
err error
21+
want bool
22+
}{
23+
"os_error": {err: err, want: true},
24+
"wrapped_os_error": {err: fmt.Errorf("wrapped: %w", err), want: true},
25+
"joined_error": {err: goerrors.Join(err, goerrors.New("test")), want: true},
26+
"new_error": {err: agentErrors.New(err, fmt.Errorf("test")), want: false},
27+
}
28+
for name, tc := range testCases {
29+
t.Run(fmt.Sprintf("%s_%s", err.Error(), name), func(t *testing.T) {
30+
require.Equal(t, tc.want, IsDiskSpaceError(tc.err))
31+
})
32+
}
33+
}
34+
}

‎internal/pkg/agent/application/upgrade/artifact/download/errors/disk_space_errors_windows.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
2+
// or more contributor license agreements. Licensed under the Elastic License 2.0;
3+
// you may not use this file except in compliance with the Elastic License 2.0.
4+
5+
//go:build windows
6+
7+
package errors
8+
9+
import "golang.org/x/sys/windows"
10+
11+
var OS_DiskSpaceErrors = []error{
12+
windows.ERROR_DISK_FULL,
13+
windows.ERROR_HANDLE_DISK_FULL,
14+
}

‎internal/pkg/agent/application/upgrade/artifact/download/fs/downloader.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package fs
66

77
import (
88
"context"
9+
goerrors "errors"
910
"fmt"
1011
"io"
1112
"os"
@@ -27,13 +28,20 @@ const (
2728
type Downloader struct {
2829
dropPath string
2930
config *artifact.Config
31+
// The following are abstractions for stdlib functions so that we can mock them in tests.
32+
copy func(dst io.Writer, src io.Reader) (int64, error)
33+
mkdirAll func(name string, perm os.FileMode) error
34+
openFile func(name string, flag int, perm os.FileMode) (*os.File, error)
3035
}
3136

3237
// NewDownloader creates and configures Elastic Downloader
3338
func NewDownloader(config *artifact.Config) *Downloader {
3439
return &Downloader{
3540
config: config,
3641
dropPath: getDropPath(config),
42+
copy: io.Copy,
43+
mkdirAll: os.MkdirAll,
44+
openFile: os.OpenFile,
3745
}
3846
}
3947

@@ -108,18 +116,18 @@ func (e *Downloader) downloadFile(filename, fullPath string) (string, error) {
108116
defer sourceFile.Close()
109117

110118
if destinationDir := filepath.Dir(fullPath); destinationDir != "" && destinationDir != "." {
111-
if err := os.MkdirAll(destinationDir, 0755); err != nil {
119+
if err := e.mkdirAll(destinationDir, 0755); err != nil {
112120
return "", err
113121
}
114122
}
115123

116-
destinationFile, err := os.OpenFile(fullPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, packagePermissions)
124+
destinationFile, err := e.openFile(fullPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, packagePermissions)
117125
if err != nil {
118-
return "", errors.New(err, "creating package file failed", errors.TypeFilesystem, errors.M(errors.MetaKeyPath, fullPath))
126+
return "", goerrors.Join(errors.New("creating package file failed", errors.TypeFilesystem, errors.M(errors.MetaKeyPath, fullPath)), err)
119127
}
120128
defer destinationFile.Close()
121129

122-
_, err = io.Copy(destinationFile, sourceFile)
130+
_, err = e.copy(destinationFile, sourceFile)
123131
if err != nil {
124132
return "", err
125133
}

‎internal/pkg/agent/application/upgrade/artifact/download/fs/downloader_test.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,17 @@ package fs
77
import (
88
"context"
99
"fmt"
10+
"io"
1011
"os"
1112
"path/filepath"
1213
"testing"
1314

1415
"github.com/stretchr/testify/assert"
1516
"github.com/stretchr/testify/require"
1617

18+
"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
1719
"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
20+
"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
1821
agtversion "github.com/elastic/elastic-agent/pkg/version"
1922
)
2023

@@ -161,6 +164,9 @@ func TestDownloader_Download(t *testing.T) {
161164
e := &Downloader{
162165
dropPath: dropPath,
163166
config: config,
167+
copy: io.Copy,
168+
mkdirAll: os.MkdirAll,
169+
openFile: os.OpenFile,
164170
}
165171
got, err := e.Download(context.TODO(), tt.args.a, tt.args.version)
166172
if !tt.wantErr(t, err, fmt.Sprintf("Download(%v, %v)", tt.args.a, tt.args.version)) {
@@ -282,6 +288,9 @@ func TestDownloader_DownloadAsc(t *testing.T) {
282288
e := &Downloader{
283289
dropPath: dropPath,
284290
config: config,
291+
copy: io.Copy,
292+
mkdirAll: os.MkdirAll,
293+
openFile: os.OpenFile,
285294
}
286295
got, err := e.DownloadAsc(context.TODO(), tt.args.a, tt.args.version)
287296
if !tt.wantErr(t, err, fmt.Sprintf("DownloadAsc(%v, %v)", tt.args.a, tt.args.version)) {
@@ -291,3 +300,76 @@ func TestDownloader_DownloadAsc(t *testing.T) {
291300
})
292301
}
293302
}
303+
304+
func TestDownloadDiskSpaceError(t *testing.T) {
305+
testError := errors.New("test error")
306+
307+
testCases := map[string]struct {
308+
mockStdlibFuncs func(downloader *Downloader)
309+
expectedError error
310+
}{
311+
"when io.Copy runs into an error, the downloader should return the error and clean up the downloaded files": {
312+
mockStdlibFuncs: func(downloader *Downloader) {
313+
downloader.copy = func(dst io.Writer, src io.Reader) (int64, error) {
314+
return 0, testError
315+
}
316+
},
317+
expectedError: testError,
318+
},
319+
"when os.OpenFile runs into an error, the downloader should return the error and clean up the downloaded files": {
320+
mockStdlibFuncs: func(downloader *Downloader) {
321+
downloader.openFile = func(name string, flag int, perm os.FileMode) (*os.File, error) {
322+
return nil, testError
323+
}
324+
},
325+
expectedError: testError,
326+
},
327+
"when os.MkdirAll runs into an error, the downloader should return the error and clean up the downloaded files": {
328+
mockStdlibFuncs: func(downloader *Downloader) {
329+
downloader.mkdirAll = func(name string, perm os.FileMode) error {
330+
return testError
331+
}
332+
},
333+
expectedError: testError,
334+
},
335+
}
336+
337+
for name, tc := range testCases {
338+
t.Run(name, func(t *testing.T) {
339+
baseDir := t.TempDir()
340+
paths.SetTop(baseDir)
341+
config := &artifact.Config{
342+
DropPath: filepath.Join(baseDir, "drop"),
343+
TargetDirectory: filepath.Join(baseDir, "target"),
344+
}
345+
346+
err := os.MkdirAll(config.DropPath, 0o755)
347+
require.NoError(t, err)
348+
349+
err = os.MkdirAll(config.TargetDirectory, 0o755)
350+
require.NoError(t, err)
351+
352+
parsedVersion := agtversion.NewParsedSemVer(1, 2, 3, "", "")
353+
354+
artifactName, err := artifact.GetArtifactName(agentSpec, *parsedVersion, config.OS(), config.Arch())
355+
require.NoError(t, err)
356+
357+
sourceArtifactPath := filepath.Join(config.DropPath, artifactName)
358+
sourceArtifactHashPath := sourceArtifactPath + ".sha512"
359+
360+
err = os.WriteFile(sourceArtifactPath, []byte("test"), 0o666)
361+
require.NoError(t, err, "failed to create source artifact file")
362+
363+
err = os.WriteFile(sourceArtifactHashPath, []byte("test"), 0o666)
364+
require.NoError(t, err, "failed to create source artifact hash file")
365+
366+
downloader := NewDownloader(config)
367+
tc.mockStdlibFuncs(downloader)
368+
targetArtifactPath, err := downloader.Download(context.Background(), agentSpec, parsedVersion)
369+
370+
require.ErrorIs(t, err, tc.expectedError)
371+
372+
require.NoFileExists(t, targetArtifactPath)
373+
})
374+
}
375+
}

‎internal/pkg/agent/application/upgrade/artifact/download/http/downloader.go‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package http
66

77
import (
88
"context"
9+
goerrors "errors"
910
"fmt"
1011
"io"
1112
"net/http"
@@ -20,6 +21,7 @@ import (
2021
"github.com/elastic/elastic-agent-libs/transport/httpcommon"
2122
"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact"
2223
"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download"
24+
downloadErrors "github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/artifact/download/errors"
2325
"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details"
2426
"github.com/elastic/elastic-agent/internal/pkg/agent/errors"
2527
"github.com/elastic/elastic-agent/pkg/core/logger"
@@ -49,6 +51,12 @@ type Downloader struct {
4951
config *artifact.Config
5052
client http.Client
5153
upgradeDetails *details.Details
54+
// The following are abstractions for stdlib functions so that we can mock them in tests.
55+
copy func(dst io.Writer, src io.Reader) (int64, error)
56+
mkdirAll func(name string, perm os.FileMode) error
57+
openFile func(name string, flag int, perm os.FileMode) (*os.File, error)
58+
// Abstraction for the disk space error check function so that we can mock it in tests.
59+
isDiskSpaceErrorFunc func(err error) bool
5260
}
5361

5462
// NewDownloader creates and configures Elastic Downloader
@@ -68,10 +76,14 @@ func NewDownloader(log *logger.Logger, config *artifact.Config, upgradeDetails *
6876
// NewDownloaderWithClient creates Elastic Downloader with specific client used
6977
func NewDownloaderWithClient(log *logger.Logger, config *artifact.Config, client http.Client, upgradeDetails *details.Details) *Downloader {
7078
return &Downloader{
71-
log: log,
72-
config: config,
73-
client: client,
74-
upgradeDetails: upgradeDetails,
79+
log: log,
80+
config: config,
81+
client: client,
82+
upgradeDetails: upgradeDetails,
83+
copy: io.Copy,
84+
mkdirAll: os.MkdirAll,
85+
openFile: os.OpenFile,
86+
isDiskSpaceErrorFunc: downloadErrors.IsDiskSpaceError,
7587
}
7688
}
7789

@@ -179,14 +191,14 @@ func (e *Downloader) downloadFile(ctx context.Context, artifactName, filename, f
179191
}
180192

181193
if destinationDir := filepath.Dir(fullPath); destinationDir != "" && destinationDir != "." {
182-
if err := os.MkdirAll(destinationDir, 0o755); err != nil {
194+
if err := e.mkdirAll(destinationDir, 0o755); err != nil {
183195
return "", err
184196
}
185197
}
186198

187-
destinationFile, err := os.OpenFile(fullPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, packagePermissions)
199+
destinationFile, err := e.openFile(fullPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, packagePermissions)
188200
if err != nil {
189-
return "", errors.New(err, "creating package file failed", errors.TypeFilesystem, errors.M(errors.MetaKeyPath, fullPath))
201+
return "", goerrors.Join(errors.New("creating package file failed", errors.TypeFilesystem, errors.M(errors.MetaKeyPath, fullPath)), err)
190202
}
191203
defer destinationFile.Close()
192204

@@ -213,11 +225,18 @@ func (e *Downloader) downloadFile(ctx context.Context, artifactName, filename, f
213225
detailsObserver := newDetailsProgressObserver(e.upgradeDetails)
214226
dp := newDownloadProgressReporter(sourceURI, e.config.Timeout, fileSize, loggingObserver, detailsObserver)
215227
dp.Report(ctx)
216-
_, err = io.Copy(destinationFile, io.TeeReader(resp.Body, dp))
228+
229+
_, err = e.copy(destinationFile, io.TeeReader(resp.Body, dp))
217230
if err != nil {
218-
dp.ReportFailed(err)
231+
// checking for disk space error here before passing it into the reporter
232+
// so the details observer sets the state with clean error message
233+
reportedErr := err
234+
if e.isDiskSpaceErrorFunc(err) {
235+
reportedErr = downloadErrors.ErrInsufficientDiskSpace
236+
}
237+
dp.ReportFailed(reportedErr)
219238
// return path, file already exists and needs to be cleaned up
220-
return fullPath, errors.New(err, "copying fetched package failed", errors.TypeNetwork, errors.M(errors.MetaKeyURI, sourceURI))
239+
return fullPath, goerrors.Join(errors.New("copying fetched package failed", errors.TypeNetwork, errors.M(errors.MetaKeyURI, sourceURI)), err)
221240
}
222241
dp.ReportComplete()
223242

0 commit comments

Comments
 (0)