Implementing RetryUntil and RetryError fields in Upgrade Details metadata (#3845)

* Implementing RetryUntil and RetryableError fields in Upgrade Details metadata

* Add unit test case

* Adding CHANGELOG entry

* Adding unit test case

* Update unit test

* (re)tried -> retried

* retryable_error_msg/RetryableErrorMsg -> retry_error_msg/RetryErrorMsg

* Adding godoc for mockUpgradeDetails

Author: ycombinator

changelog/fragments/1701293846-upgrade-details-downloading.yaml

@@ -0,0 +1,32 @@
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: feature
+
+# Change summary; a 80ish characters long description of the change.
+summary: Add metadata for retryable upgrade steps to upgrade details
+
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
+#description:
+
+# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
+component: elastic-agent
+
+# PR URL; optional; the PR number that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+pr: https://github.com/elastic/elastic-agent/pull/3845
+
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+issue: https://github.com/elastic/elastic-agent/issues/3818

control_v2.proto

@@ -213,7 +213,7 @@ message UpgradeDetails {
 message UpgradeDetailsMetadata {
   // If the upgrade is a scheduled upgrade, the timestamp of when the
   // upgrade is expected to start.
-  google.protobuf.Timestamp scheduled_at = 1;
+  string scheduled_at = 1;
 
   // If the upgrade is in the UPG_DOWNLOADING state, the percentage of
   // the Elastic Agent artifact that has already been downloaded, to
@@ -225,6 +225,14 @@ message UpgradeDetailsMetadata {
 
   // Any error encountered during the upgrade process.
   string error_msg = 4;
+
+  // Any error message that is a result of a retryable upgrade
+  // step, e.g. the download step, being retried.
+  string retry_error_msg = 5;
+
+  // The deadline until when a retryable upgrade step, e.g. the download
+  // step, will be retried.
+  string retry_until = 6;
 }
 
 // DiagnosticFileResult is a file result from a diagnostic result.

internal/pkg/agent/application/coordinator/diagnostics_test.go

@@ -441,6 +441,7 @@ func TestDiagnosticState(t *testing.T) {
 				DownloadPercent: 0.17469,
 				ScheduledAt:     &now,
 				DownloadRate:    123.56,
+				RetryUntil:      &now,
 			},
 		},
 	}
@@ -470,7 +471,8 @@ upgrade_details:
     download_percent: 0.17469
     scheduled_at: %s
     download_rate: 123.56
-`, now.Format(time.RFC3339Nano))
+    retry_until: %s
+`, now.Format(time.RFC3339Nano), now.Format(time.RFC3339Nano))
 
 	coord := &Coordinator{
 		// This test needs a broadcaster since the components-actual diagnostic

internal/pkg/agent/application/upgrade/details/details.go

@@ -37,6 +37,14 @@ type Metadata struct {
 	// is progressing.
 	DownloadRate details.DownloadRate `json:"download_rate,omitempty" yaml:"download_rate,omitempty"`
 
+	// RetryErrorMsg is any error message that is a result of a retryable upgrade
+	// step, e.g. the download step, being retried.
+	RetryErrorMsg string `json:"retry_error_msg,omitempty" yaml:"retry_error_msg,omitempty"`
+
+	// RetryUntil is the deadline until when a retryable upgrade step, e.g. the download
+	// step, will be retried.
+	RetryUntil *time.Time `json:"retry_until,omitempty" yaml:"retry_until"`
+
 	// FailedState is the state an upgrade was in if/when it failed. Use the
 	// Fail() method of UpgradeDetails to correctly record details when
 	// an upgrade fails.
@@ -89,6 +97,28 @@ func (d *Details) SetDownloadProgress(percent, rateBytesPerSecond float64) {
 	d.notifyObservers()
 }
 
+// SetRetryableError sets the RetryErrorMsg metadata field.
+func (d *Details) SetRetryableError(retryableError error) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	if retryableError == nil {
+		d.Metadata.RetryErrorMsg = ""
+	} else {
+		d.Metadata.RetryErrorMsg = retryableError.Error()
+	}
+	d.notifyObservers()
+}
+
+// SetRetryUntil sets the RetryUntil metadata field.
+func (d *Details) SetRetryUntil(retryUntil *time.Time) {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	d.Metadata.RetryUntil = retryUntil
+	d.notifyObservers()
+}
+
 // Fail is a convenience method to set the state of the upgrade
 // to StateFailed, set metadata associated with the failure, and
 // notify all observers.
@@ -163,7 +193,9 @@ func (m Metadata) Equals(otherM Metadata) bool {
 		m.FailedState == otherM.FailedState &&
 		m.ErrorMsg == otherM.ErrorMsg &&
 		m.DownloadPercent == otherM.DownloadPercent &&
-		m.DownloadRate == otherM.DownloadRate
+		m.DownloadRate == otherM.DownloadRate &&
+		equalTimePointers(m.RetryUntil, otherM.RetryUntil) &&
+		m.RetryErrorMsg == otherM.RetryErrorMsg
 }
 
 func equalTimePointers(t, otherT *time.Time) bool {

internal/pkg/agent/application/upgrade/details/details_test.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"math"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 )
@@ -102,10 +103,16 @@ func TestDetailsDownloadRateJSON(t *testing.T) {
 func TestEquals(t *testing.T) {
 	details1 := NewDetails("8.12.0", StateDownloading, "foobar")
 	details1.SetDownloadProgress(0.1234, 34.56)
+	details1.SetRetryableError(errors.New("retryable error"))
+	retryUntil1 := time.Date(2023, 11, 29, 11, 00, 32, 0, time.UTC)
+	details1.SetRetryUntil(&retryUntil1)
 	details1.Fail(errors.New("download failed"))
 
 	details2 := NewDetails("8.12.0", StateDownloading, "foobar")
 	details2.SetDownloadProgress(0.1234, 34.56)
+	details2.SetRetryableError(errors.New("retryable error"))
+	retryUntil2 := time.Date(2023, 11, 29, 11, 00, 32, 0, time.UTC)
+	details2.SetRetryUntil(&retryUntil2)
 	details2.Fail(errors.New("download failed"))
 
 	details3 := NewDetails("8.12.0", StateDownloading, "foobar")

internal/pkg/agent/application/upgrade/step_download.go

@@ -235,9 +235,12 @@ func (u *Upgrader) downloadWithRetries(
 	settings *artifact.Config,
 	upgradeDetails *details.Details,
 ) (string, error) {
-	cancelCtx, cancel := context.WithTimeout(ctx, settings.Timeout)
+	cancelDeadline := time.Now().Add(settings.Timeout)
+	cancelCtx, cancel := context.WithDeadline(ctx, cancelDeadline)
 	defer cancel()
 
+	upgradeDetails.SetRetryUntil(&cancelDeadline)
+
 	expBo := backoff.NewExponentialBackOff()
 	expBo.InitialInterval = settings.RetrySleepInitDuration
 	boCtx := backoff.WithContext(expBo, cancelCtx)
@@ -259,11 +262,16 @@ func (u *Upgrader) downloadWithRetries(
 	opFailureNotificationFn := func(err error, retryAfter time.Duration) {
 		u.log.Warnf("download attempt %d failed: %s; retrying in %s.",
 			attempt, err.Error(), retryAfter)
+		upgradeDetails.SetRetryableError(err)
 	}
 
 	if err := backoff.RetryNotify(opFn, boCtx, opFailureNotificationFn); err != nil {
 		return "", err
 	}
 
+	// Clear retry details upon success
+	upgradeDetails.SetRetryableError(nil)
+	upgradeDetails.SetRetryUntil(nil)
+
 	return path, nil
 }

internal/pkg/agent/application/upgrade/step_download_test.go

@@ -94,14 +94,27 @@ func TestDownloadWithRetries(t *testing.T) {
 
 		parsedVersion, err := agtversion.ParseVersion("8.9.0")
 		require.NoError(t, err)
-		upgradeDetails := details.NewDetails(parsedVersion.String(), details.StateRequested, "")
+
+		upgradeDetails, upgradeDetailsRetryUntil, upgradeDetailsRetryUntilWasUnset, upgradeDetailsRetryErrorMsg := mockUpgradeDetails(parsedVersion)
+		minRetryDeadline := time.Now().Add(settings.Timeout)
+
 		path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &settings, upgradeDetails)
 		require.NoError(t, err)
 		require.Equal(t, expectedDownloadPath, path)
 
 		logs := obs.TakeAll()
 		require.Len(t, logs, 1)
 		require.Equal(t, "download attempt 1", logs[0].Message)
+
+		// Check that upgradeDetails.Metadata.RetryUntil was set at some point
+		// during the retryable download and then check that it was unset upon
+		// successful download.
+		require.GreaterOrEqual(t, *upgradeDetailsRetryUntil, minRetryDeadline)
+		require.True(t, *upgradeDetailsRetryUntilWasUnset)
+		require.Nil(t, upgradeDetails.Metadata.RetryUntil)
+
+		// Check that upgradeDetails.Metadata.RetryErrorMsg was never set.
+		require.Empty(t, *upgradeDetailsRetryErrorMsg)
 	})
 
 	// Downloader constructor failing on first attempt, but succeeding on second attempt (= first retry)
@@ -131,7 +144,10 @@ func TestDownloadWithRetries(t *testing.T) {
 
 		parsedVersion, err := agtversion.ParseVersion("8.9.0")
 		require.NoError(t, err)
-		upgradeDetails := details.NewDetails(parsedVersion.String(), details.StateRequested, "")
+
+		upgradeDetails, upgradeDetailsRetryUntil, upgradeDetailsRetryUntilWasUnset, upgradeDetailsRetryErrorMsg := mockUpgradeDetails(parsedVersion)
+		minRetryDeadline := time.Now().Add(settings.Timeout)
+
 		path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &settings, upgradeDetails)
 		require.NoError(t, err)
 		require.Equal(t, expectedDownloadPath, path)
@@ -141,6 +157,19 @@ func TestDownloadWithRetries(t *testing.T) {
 		require.Equal(t, "download attempt 1", logs[0].Message)
 		require.Contains(t, logs[1].Message, "unable to create fetcher: failed to construct downloader")
 		require.Equal(t, "download attempt 2", logs[2].Message)
+
+		// Check that upgradeDetails.Metadata.RetryUntil was set at some point
+		// during the retryable download and then check that it was unset upon
+		// successful download.
+		require.GreaterOrEqual(t, *upgradeDetailsRetryUntil, minRetryDeadline)
+		require.True(t, *upgradeDetailsRetryUntilWasUnset)
+		require.Nil(t, upgradeDetails.Metadata.RetryUntil)
+
+		// Check that upgradeDetails.Metadata.RetryErrorMsg was set at some point
+		// during the retryable download and then check that it was unset upon
+		// successful download.
+		require.NotEmpty(t, *upgradeDetailsRetryErrorMsg)
+		require.Empty(t, upgradeDetails.Metadata.RetryErrorMsg)
 	})
 
 	// Download failing on first attempt, but succeeding on second attempt (= first retry)
@@ -170,7 +199,10 @@ func TestDownloadWithRetries(t *testing.T) {
 
 		parsedVersion, err := agtversion.ParseVersion("8.9.0")
 		require.NoError(t, err)
-		upgradeDetails := details.NewDetails(parsedVersion.String(), details.StateRequested, "")
+
+		upgradeDetails, upgradeDetailsRetryUntil, upgradeDetailsRetryUntilWasUnset, upgradeDetailsRetryErrorMsg := mockUpgradeDetails(parsedVersion)
+		minRetryDeadline := time.Now().Add(settings.Timeout)
+
 		path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &settings, upgradeDetails)
 		require.NoError(t, err)
 		require.Equal(t, expectedDownloadPath, path)
@@ -180,6 +212,19 @@ func TestDownloadWithRetries(t *testing.T) {
 		require.Equal(t, "download attempt 1", logs[0].Message)
 		require.Contains(t, logs[1].Message, "unable to download package: download failed; retrying")
 		require.Equal(t, "download attempt 2", logs[2].Message)
+
+		// Check that upgradeDetails.Metadata.RetryUntil was set at some point
+		// during the retryable download and then check that it was unset upon
+		// successful download.
+		require.GreaterOrEqual(t, *upgradeDetailsRetryUntil, minRetryDeadline)
+		require.True(t, *upgradeDetailsRetryUntilWasUnset)
+		require.Nil(t, upgradeDetails.Metadata.RetryUntil)
+
+		// Check that upgradeDetails.Metadata.RetryErrorMsg was set at some point
+		// during the retryable download and then check that it was unset upon
+		// successful download.
+		require.NotEmpty(t, *upgradeDetailsRetryErrorMsg)
+		require.Empty(t, upgradeDetails.Metadata.RetryErrorMsg)
 	})
 
 	// Download timeout expired (before all retries are exhausted)
@@ -197,7 +242,10 @@ func TestDownloadWithRetries(t *testing.T) {
 
 		parsedVersion, err := agtversion.ParseVersion("8.9.0")
 		require.NoError(t, err)
-		upgradeDetails := details.NewDetails(parsedVersion.String(), details.StateRequested, "")
+
+		upgradeDetails, upgradeDetailsRetryUntil, upgradeDetailsRetryUntilWasUnset, upgradeDetailsRetryErrorMsg := mockUpgradeDetails(parsedVersion)
+		minRetryDeadline := time.Now().Add(testCaseSettings.Timeout)
+
 		path, err := u.downloadWithRetries(context.Background(), mockDownloaderCtor, parsedVersion, &testCaseSettings, upgradeDetails)
 		require.Equal(t, "context deadline exceeded", err.Error())
 		require.Equal(t, "", path)
@@ -209,5 +257,48 @@ func TestDownloadWithRetries(t *testing.T) {
 			require.Equal(t, fmt.Sprintf("download attempt %d", i+1), logs[(2*i)].Message)
 			require.Contains(t, logs[(2*i+1)].Message, "unable to download package: download failed; retrying")
 		}
+
+		// Check that upgradeDetails.Metadata.RetryUntil was set at some point
+		// during the retryable download and then check that it was never unset,
+		// since we didn't have a successful download.
+		require.GreaterOrEqual(t, *upgradeDetailsRetryUntil, minRetryDeadline)
+		require.False(t, *upgradeDetailsRetryUntilWasUnset)
+		require.Equal(t, *upgradeDetailsRetryUntil, *upgradeDetails.Metadata.RetryUntil)
+
+		// Check that upgradeDetails.Metadata.RetryErrorMsg was set at some point
+		// during the retryable download and then check that it was never unset,
+		//since we didn't have a successful download.
+		require.NotEmpty(t, *upgradeDetailsRetryErrorMsg)
+		require.Equal(t, *upgradeDetailsRetryErrorMsg, upgradeDetails.Metadata.RetryErrorMsg)
 	})
 }
+
+// mockUpgradeDetails returns a *details.Details value that has an observer registered on it for inspecting
+// certain properties of the object being set and unset.  It also returns:
+// - a *time.Time value, which will be not nil if Metadata.RetryUntil is set on the mock value,
+// - a *bool value, which will be true if Metadata.RetryUntil is set and then unset on the mock value,
+// - a *string value, which will be non-empty if Metadata.RetryErrorMsg is set on the mock value.
+func mockUpgradeDetails(parsedVersion *agtversion.ParsedSemVer) (*details.Details, *time.Time, *bool, *string) {
+	var upgradeDetailsRetryUntil time.Time
+	var upgradeDetailsRetryUntilWasUnset bool
+	var upgradeDetailsRetryErrorMsg string
+
+	upgradeDetails := details.NewDetails(parsedVersion.String(), details.StateRequested, "")
+	upgradeDetails.RegisterObserver(func(details *details.Details) {
+		if details.Metadata.RetryUntil != nil {
+			upgradeDetailsRetryUntil = *details.Metadata.RetryUntil
+		}
+
+		if !upgradeDetailsRetryUntil.IsZero() && details.Metadata.RetryUntil == nil {
+			upgradeDetailsRetryUntilWasUnset = true
+		}
+
+		if details.Metadata.RetryErrorMsg != "" {
+			upgradeDetailsRetryErrorMsg = details.Metadata.RetryErrorMsg
+		}
+	})
+
+	return upgradeDetails,
+		&upgradeDetailsRetryUntil, &upgradeDetailsRetryUntilWasUnset,
+		&upgradeDetailsRetryErrorMsg
+}

internal/pkg/agent/cmd/status.go

@@ -14,6 +14,7 @@ import (
 	"time"
 
 	"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details"
+	"github.com/elastic/elastic-agent/pkg/control"
 	"github.com/elastic/elastic-agent/pkg/control/v2/client"
 	"github.com/elastic/elastic-agent/pkg/control/v2/cproto"
 
@@ -168,8 +169,8 @@ func listUpgradeDetails(l list.Writer, upgradeDetails *cproto.UpgradeDetails) {
 	if upgradeDetails.Metadata != nil {
 		l.AppendItem("metadata")
 		l.Indent()
-		if upgradeDetails.Metadata.ScheduledAt != nil && !upgradeDetails.Metadata.ScheduledAt.AsTime().IsZero() {
-			l.AppendItem("scheduled_at: " + upgradeDetails.Metadata.ScheduledAt.AsTime().UTC().Format(time.RFC3339))
+		if upgradeDetails.Metadata.ScheduledAt != "" {
+			l.AppendItem("scheduled_at: " + upgradeDetails.Metadata.ScheduledAt)
 		}
 		if upgradeDetails.Metadata.FailedState != "" {
 			l.AppendItem("failed_state: " + upgradeDetails.Metadata.FailedState)
@@ -180,6 +181,12 @@ func listUpgradeDetails(l list.Writer, upgradeDetails *cproto.UpgradeDetails) {
 		if upgradeDetails.State == string(details.StateDownloading) {
 			l.AppendItem(fmt.Sprintf("download_percent: %.2f%%", upgradeDetails.Metadata.DownloadPercent*100))
 		}
+		if upgradeDetails.Metadata.RetryUntil != "" {
+			l.AppendItem("retry_until: " + humanDurationUntil(upgradeDetails.Metadata.RetryUntil, time.Now()))
+		}
+		if upgradeDetails.Metadata.RetryErrorMsg != "" {
+			l.AppendItem("retry_error_msg: " + upgradeDetails.Metadata.RetryErrorMsg)
+		}
 		l.UnIndent()
 	}
 
@@ -236,3 +243,13 @@ func yamlOutput(w io.Writer, out interface{}) error {
 	fmt.Fprintf(w, "%s\n", bytes)
 	return nil
 }
+
+func humanDurationUntil(targetTime string, from time.Time) string {
+	target, err := time.Parse(control.TimeFormat(), targetTime)
+	if err != nil {
+		return targetTime
+	}
+
+	until := target.Sub(from)
+	return until.String()
+}