From 5f3eaa01780aa2e09c4ab0877df2d6dc1ac90694 Mon Sep 17 00:00:00 2001 From: Abin Simon Date: Sat, 4 Feb 2023 06:13:23 +0530 Subject: [PATCH] Adds more retries to OneDrive API calls (#2387) ## Description Adds more reties to handle timeout issues. ## Does this PR need a docs update or release note? - [x] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [ ] :no_entry: No ## Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup ## Issue(s) * # ## Test Plan - [x] :muscle: Manual - [ ] :zap: Unit test - [ ] :green_heart: E2E --- CHANGELOG.md | 1 + src/internal/connector/discovery/api/users.go | 31 +++++++++++--- src/internal/connector/exchange/api/api.go | 24 ----------- .../connector/exchange/api/contacts.go | 10 ++--- src/internal/connector/exchange/api/events.go | 10 ++--- src/internal/connector/exchange/api/mail.go | 8 ++-- src/internal/connector/graph/service.go | 25 +++++++++++ src/internal/connector/onedrive/api/drive.go | 36 ++++++++++++++-- src/internal/connector/onedrive/drive.go | 13 +++++- src/internal/connector/onedrive/drive_test.go | 42 ++++++++++--------- 10 files changed, 132 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf2cd7f0f..0303c9206 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Backing up a calendar that has the same name as the default calendar +- Added additional backoff-retry to all OneDrive queries. ### Known Issues diff --git a/src/internal/connector/discovery/api/users.go b/src/internal/connector/discovery/api/users.go index ff41ee06f..e4a8e0f00 100644 --- a/src/internal/connector/discovery/api/users.go +++ b/src/internal/connector/discovery/api/users.go @@ -77,7 +77,13 @@ func (c Users) GetAll(ctx context.Context) ([]models.Userable, error) { return nil, err } - resp, err := service.Client().Users().Get(ctx, userOptions(&userFilterNoGuests)) + var resp models.UserCollectionResponseable + + err = graph.RunWithRetry(func() error { + resp, err = service.Client().Users().Get(ctx, userOptions(&userFilterNoGuests)) + return err + }) + if err != nil { return nil, support.ConnectorStackErrorTraceWrap(err, "getting all users") } @@ -114,22 +120,37 @@ func (c Users) GetAll(ctx context.Context) ([]models.Userable, error) { } func (c Users) GetByID(ctx context.Context, userID string) (models.Userable, error) { - user, err := c.stable.Client().UsersById(userID).Get(ctx, nil) + var ( + resp models.Userable + err error + ) + + err = graph.RunWithRetry(func() error { + resp, err = c.stable.Client().UsersById(userID).Get(ctx, nil) + return err + }) + if err != nil { return nil, support.ConnectorStackErrorTraceWrap(err, "getting user by id") } - return user, nil + return resp, err } func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { // Assume all services are enabled // then filter down to only services the user has enabled - userInfo := newUserInfo() + var ( + err error + userInfo = newUserInfo() + ) // TODO: OneDrive + err = graph.RunWithRetry(func() error { + _, err = c.stable.Client().UsersById(userID).MailFolders().Get(ctx, nil) + return err + }) - _, err := c.stable.Client().UsersById(userID).MailFolders().Get(ctx, nil) if err != nil { if !graph.IsErrExchangeMailFolderNotFound(err) { return nil, support.ConnectorStackErrorTraceWrap(err, "getting user's exchange mailfolders") diff --git a/src/internal/connector/exchange/api/api.go b/src/internal/connector/exchange/api/api.go index c47cc9b5b..c4858c5c1 100644 --- a/src/internal/connector/exchange/api/api.go +++ b/src/internal/connector/exchange/api/api.go @@ -10,7 +10,6 @@ import ( "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/connector/graph" - "github.com/alcionai/corso/src/internal/connector/support" "github.com/alcionai/corso/src/pkg/account" ) @@ -154,26 +153,3 @@ func HasAttachments(body models.ItemBodyable) bool { return strings.Contains(content, "src=\"cid:") } - -// Run a function with retries -func runWithRetry(run func() error) error { - var err error - - for i := 0; i < numberOfRetries; i++ { - err = run() - if err == nil { - return nil - } - - // only retry on timeouts and 500-internal-errors. - if !(graph.IsErrTimeout(err) || graph.IsInternalServerError(err)) { - break - } - - if i < numberOfRetries { - time.Sleep(time.Duration(3*(i+2)) * time.Second) - } - } - - return support.ConnectorStackErrorTraceWrap(err, "maximum retries or unretryable") -} diff --git a/src/internal/connector/exchange/api/contacts.go b/src/internal/connector/exchange/api/contacts.go index 458f364d1..ac4afeb34 100644 --- a/src/internal/connector/exchange/api/contacts.go +++ b/src/internal/connector/exchange/api/contacts.go @@ -68,7 +68,7 @@ func (c Contacts) GetItem( err error ) - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { cont, err = c.stable.Client().UsersById(user).ContactsById(itemID).Get(ctx, nil) return err }) @@ -94,7 +94,7 @@ func (c Contacts) GetAllContactFolderNamesForUser( var resp models.ContactFolderCollectionResponseable - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { resp, err = c.stable.Client().UsersById(user).ContactFolders().Get(ctx, options) return err }) @@ -113,7 +113,7 @@ func (c Contacts) GetContainerByID( var resp models.ContactFolderable - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { resp, err = c.stable.Client().UsersById(userID).ContactFoldersById(dirID).Get(ctx, ofcf) return err }) @@ -154,7 +154,7 @@ func (c Contacts) EnumerateContainers( ChildFolders() for { - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { resp, err = builder.Get(ctx, ofcf) return err }) @@ -206,7 +206,7 @@ func (p *contactPager) getPage(ctx context.Context) (api.DeltaPageLinker, error) err error ) - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { resp, err = p.builder.Get(ctx, p.options) return err }) diff --git a/src/internal/connector/exchange/api/events.go b/src/internal/connector/exchange/api/events.go index 70a1a45e9..b9c16f319 100644 --- a/src/internal/connector/exchange/api/events.go +++ b/src/internal/connector/exchange/api/events.go @@ -77,7 +77,7 @@ func (c Events) GetContainerByID( var cal models.Calendarable - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { cal, err = service.Client().UsersById(userID).CalendarsById(containerID).Get(ctx, ofc) return err }) @@ -99,7 +99,7 @@ func (c Events) GetItem( err error ) - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { event, err = c.stable.Client().UsersById(user).EventsById(itemID).Get(ctx, nil) return err }) @@ -154,7 +154,7 @@ func (c Client) GetAllCalendarNamesForUser( var resp models.CalendarCollectionResponseable - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { resp, err = c.stable.Client().UsersById(user).Calendars().Get(ctx, options) return err }) @@ -193,7 +193,7 @@ func (c Events) EnumerateContainers( for { var err error - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { resp, err = builder.Get(ctx, ofc) return err }) @@ -250,7 +250,7 @@ func (p *eventPager) getPage(ctx context.Context) (api.DeltaPageLinker, error) { err error ) - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { resp, err = p.builder.Get(ctx, p.options) return err }) diff --git a/src/internal/connector/exchange/api/mail.go b/src/internal/connector/exchange/api/mail.go index 6acc05162..01a485fbb 100644 --- a/src/internal/connector/exchange/api/mail.go +++ b/src/internal/connector/exchange/api/mail.go @@ -99,7 +99,7 @@ func (c Mail) GetContainerByID( var resp graph.Container - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { resp, err = service.Client().UsersById(userID).MailFoldersById(dirID).Get(ctx, ofmf) return err }) @@ -118,7 +118,7 @@ func (c Mail) GetItem( err error ) - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { mail, err = c.stable.Client().UsersById(user).MessagesById(itemID).Get(ctx, nil) return err }) @@ -188,7 +188,7 @@ func (c Mail) EnumerateContainers( for { var err error - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { resp, err = builder.Get(ctx, nil) return err }) @@ -235,7 +235,7 @@ func (p *mailPager) getPage(ctx context.Context) (api.DeltaPageLinker, error) { err error ) - err = runWithRetry(func() error { + err = graph.RunWithRetry(func() error { page, err = p.builder.Get(ctx, p.options) return err }) diff --git a/src/internal/connector/graph/service.go b/src/internal/connector/graph/service.go index 093995a42..fd6142028 100644 --- a/src/internal/connector/graph/service.go +++ b/src/internal/connector/graph/service.go @@ -8,6 +8,7 @@ import ( "time" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" + "github.com/alcionai/corso/src/internal/connector/support" "github.com/microsoft/kiota-abstractions-go/serialization" ka "github.com/microsoft/kiota-authentication-azure-go" khttp "github.com/microsoft/kiota-http-go" @@ -22,6 +23,7 @@ import ( const ( logGraphRequestsEnvKey = "LOG_GRAPH_REQUESTS" + numberOfRetries = 3 ) // AllMetadataFileNames produces the standard set of filenames used to store graph @@ -294,3 +296,26 @@ func (handler *LoggingMiddleware) Intercept( return resp, err } + +// Run a function with retries +func RunWithRetry(run func() error) error { + var err error + + for i := 0; i < numberOfRetries; i++ { + err = run() + if err == nil { + return nil + } + + // only retry on timeouts and 500-internal-errors. + if !(IsErrTimeout(err) || IsInternalServerError(err)) { + break + } + + if i < numberOfRetries { + time.Sleep(time.Duration(3*(i+2)) * time.Second) + } + } + + return support.ConnectorStackErrorTraceWrap(err, "maximum retries or unretryable") +} diff --git a/src/internal/connector/onedrive/api/drive.go b/src/internal/connector/onedrive/api/drive.go index fea6e53a7..ce246da85 100644 --- a/src/internal/connector/onedrive/api/drive.go +++ b/src/internal/connector/onedrive/api/drive.go @@ -61,7 +61,17 @@ func NewItemPager( } func (p *driveItemPager) GetPage(ctx context.Context) (api.DeltaPageLinker, error) { - return p.builder.Get(ctx, p.options) + var ( + resp api.DeltaPageLinker + err error + ) + + err = graph.RunWithRetry(func() error { + resp, err = p.builder.Get(ctx, p.options) + return err + }) + + return resp, err } func (p *driveItemPager) SetNext(link string) { @@ -99,7 +109,17 @@ func NewUserDrivePager( } func (p *userDrivePager) GetPage(ctx context.Context) (api.PageLinker, error) { - return p.builder.Get(ctx, p.options) + var ( + resp api.PageLinker + err error + ) + + err = graph.RunWithRetry(func() error { + resp, err = p.builder.Get(ctx, p.options) + return err + }) + + return resp, err } func (p *userDrivePager) SetNext(link string) { @@ -137,7 +157,17 @@ func NewSiteDrivePager( } func (p *siteDrivePager) GetPage(ctx context.Context) (api.PageLinker, error) { - return p.builder.Get(ctx, p.options) + var ( + resp api.PageLinker + err error + ) + + err = graph.RunWithRetry(func() error { + resp, err = p.builder.Get(ctx, p.options) + return err + }) + + return resp, err } func (p *siteDrivePager) SetNext(link string) { diff --git a/src/internal/connector/onedrive/drive.go b/src/internal/connector/onedrive/drive.go index 8e1578caf..ebcbe8b6f 100644 --- a/src/internal/connector/onedrive/drive.go +++ b/src/internal/connector/onedrive/drive.go @@ -80,7 +80,7 @@ func drives( page, err = pager.GetPage(ctx) if err != nil { // Various error handling. May return an error or perform a retry. - detailedError := support.ConnectorStackErrorTrace(err) + detailedError := err.Error() if strings.Contains(detailedError, userMysiteURLNotFound) || strings.Contains(detailedError, userMysiteNotFound) { logger.Ctx(ctx).Infof("resource owner does not have a drive") @@ -236,7 +236,16 @@ func getFolder( rawURL := fmt.Sprintf(itemByPathRawURLFmt, driveID, parentFolderID, folderName) builder := msdrive.NewItemsDriveItemItemRequestBuilder(rawURL, service.Adapter()) - foundItem, err := builder.Get(ctx, nil) + var ( + foundItem models.DriveItemable + err error + ) + + err = graph.RunWithRetry(func() error { + foundItem, err = builder.Get(ctx, nil) + return err + }) + if err != nil { var oDataError *odataerrors.ODataError if errors.As(err, &oDataError) && diff --git a/src/internal/connector/onedrive/drive_test.go b/src/internal/connector/onedrive/drive_test.go index a67c89ab1..5eeda6aac 100644 --- a/src/internal/connector/onedrive/drive_test.go +++ b/src/internal/connector/onedrive/drive_test.go @@ -15,6 +15,7 @@ import ( "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/graph/api" + "github.com/alcionai/corso/src/internal/connector/support" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/pkg/control" "github.com/alcionai/corso/src/pkg/logger" @@ -76,6 +77,15 @@ func TestOneDriveUnitSuite(t *testing.T) { suite.Run(t, new(OneDriveUnitSuite)) } +func odErr(code string) *odataerrors.ODataError { + odErr := &odataerrors.ODataError{} + merr := odataerrors.MainError{} + merr.SetCode(&code) + odErr.SetError(&merr) + + return odErr +} + func (suite *OneDriveUnitSuite) TestDrives() { numDriveResults := 4 emptyLink := "" @@ -84,26 +94,18 @@ func (suite *OneDriveUnitSuite) TestDrives() { // These errors won't be the "correct" format when compared to what graph // returns, but they're close enough to have the same info when the inner // details are extracted via support package. - tmp := userMysiteURLNotFound - tmpMySiteURLNotFound := odataerrors.NewMainError() - tmpMySiteURLNotFound.SetMessage(&tmp) - - mySiteURLNotFound := odataerrors.NewODataError() - mySiteURLNotFound.SetError(tmpMySiteURLNotFound) - - tmp2 := userMysiteNotFound - tmpMySiteNotFound := odataerrors.NewMainError() - tmpMySiteNotFound.SetMessage(&tmp2) - - mySiteNotFound := odataerrors.NewODataError() - mySiteNotFound.SetError(tmpMySiteNotFound) - - tmp3 := contextDeadlineExceeded - tmpDeadlineExceeded := odataerrors.NewMainError() - tmpDeadlineExceeded.SetMessage(&tmp3) - - deadlineExceeded := odataerrors.NewODataError() - deadlineExceeded.SetError(tmpDeadlineExceeded) + mySiteURLNotFound := support.ConnectorStackErrorTraceWrap( + odErr(userMysiteURLNotFound), + "maximum retries or unretryable", + ) + mySiteNotFound := support.ConnectorStackErrorTraceWrap( + odErr(userMysiteNotFound), + "maximum retries or unretryable", + ) + deadlineExceeded := support.ConnectorStackErrorTraceWrap( + odErr(contextDeadlineExceeded), + "maximum retries or unretryable", + ) resultDrives := make([]models.Driveable, 0, numDriveResults)