From 5a01b779a54fad55502afe5cafc2456bc2d60eb2 Mon Sep 17 00:00:00 2001 From: Kishan Mochi Date: Fri, 5 Dec 2025 03:04:14 -0800 Subject: [PATCH] PUA improvements - when a new download starts, any stale granular log content from previous updates is cleared - osimage URL to accept other url --- .../internal/downloader/downloader.go | 5 ++ .../internal/downloader/real_downloader.go | 14 ++- .../downloader/real_downloader_test.go | 88 +++++++++++++++++++ .../internal/updater/updater.go | 13 ++- .../internal/updater/updater_test.go | 16 ++++ 5 files changed, 134 insertions(+), 2 deletions(-) diff --git a/platform-update-agent/internal/downloader/downloader.go b/platform-update-agent/internal/downloader/downloader.go index 51103235..a9445cda 100644 --- a/platform-update-agent/internal/downloader/downloader.go +++ b/platform-update-agent/internal/downloader/downloader.go @@ -207,6 +207,11 @@ func (d *Downloader) startDownload(ctx context.Context, prependToImageURL string return false } + // Clear any stale granular log from previous updates + if err := d.metadataController.SetMetaUpdateLog(""); err != nil { + d.log.Warnf("DOWNLOAD: could not clear meta update log at start of download: %v", err) + } + err := d.downloadExecutor.Download(ctx, prependToImageURL, updateSource) if err != nil { if err := d.setStatus(pb.UpdateStatus_STATUS_TYPE_FAILED); err != nil { diff --git a/platform-update-agent/internal/downloader/real_downloader.go b/platform-update-agent/internal/downloader/real_downloader.go index b4b8be7a..1c54c6ad 100644 --- a/platform-update-agent/internal/downloader/real_downloader.go +++ b/platform-update-agent/internal/downloader/real_downloader.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "os/exec" + "strings" pb "github.com/open-edge-platform/infra-managers/maintenance/pkg/api/maintmgr/v1" "github.com/sirupsen/logrus" @@ -44,8 +45,14 @@ func (r *Downloader) setStatus(status pb.UpdateStatus_StatusType) error { // Download starts the real download process with inbc // prependToImageURL will be prepended to the image URL on download +// If OsImageUrl is already a complete URL (starts with http:// or https://), it will be used as-is func (r *RealDownloadExecutor) Download(ctx context.Context, prependToImageURL string, source *pb.OSProfileUpdateSource) error { - actualUrl := prependToImageURL + source.OsImageUrl + actualUrl := source.OsImageUrl + + // Only prepend if OsImageUrl is not already a complete URL + if !isCompleteURL(source.OsImageUrl) { + actualUrl = prependToImageURL + source.OsImageUrl + } r.log.Info("DOWNLOAD: started") @@ -69,3 +76,8 @@ func (r *RealDownloadExecutor) runInbcCommand(ctx context.Context, args ...strin } return nil } + +// isCompleteURL checks if a URL string is a complete URL (starts with http:// or https://) +func isCompleteURL(url string) bool { + return strings.HasPrefix(url, "http://") || strings.HasPrefix(url, "https://") +} diff --git a/platform-update-agent/internal/downloader/real_downloader_test.go b/platform-update-agent/internal/downloader/real_downloader_test.go index f71b6294..80b0da89 100644 --- a/platform-update-agent/internal/downloader/real_downloader_test.go +++ b/platform-update-agent/internal/downloader/real_downloader_test.go @@ -86,3 +86,91 @@ func TestNewDownloadExecutor_NoError(t *testing.T) { executor := NewDownloadExecutor(nil) assert.NotNil(t, executor, "NewDownloadExecutor(nil) should not return nil") } + +func TestRealDownloadExecutor_Download_WithPrepend(t *testing.T) { + logger := logrus.New() + logEntry := logrus.NewEntry(logger) + mockRunner := &MockCommandRunner{} + + r := &RealDownloadExecutor{ + log: logEntry, + commandRunner: mockRunner, + } + + prependURL := "https://files-rs.example.com/" + relativeURL := "repository/images/image.raw.gz" + sha := "1234567890abcdef" + + source := &pb.OSProfileUpdateSource{ + OsImageUrl: relativeURL, + OsImageSha: sha, + ProfileName: "test-profile", + } + + mockRunner.SetResponse([]byte("command success"), nil) + + err := r.Download(context.Background(), prependURL, source) + + assert.NoError(t, err) + calls := mockRunner.Calls() + assert.Len(t, calls, 1) + + expectedURL := prependURL + relativeURL + expectedCommand := []string{"inbc", "sota", "--mode", "download-only", "--signature", sha, "--uri", expectedURL} + assert.Equal(t, expectedCommand, calls[0].Args) +} + +func TestRealDownloadExecutor_Download_WithCompleteURL(t *testing.T) { + logger := logrus.New() + logEntry := logrus.NewEntry(logger) + mockRunner := &MockCommandRunner{} + + r := &RealDownloadExecutor{ + log: logEntry, + commandRunner: mockRunner, + } + + prependURL := "https://files-rs.example.com/" + completeURL := "https://artifactory.example.com/repository/images/image.raw.gz" + sha := "1234567890abcdef" + + source := &pb.OSProfileUpdateSource{ + OsImageUrl: completeURL, + OsImageSha: sha, + ProfileName: "test-profile", + } + + mockRunner.SetResponse([]byte("command success"), nil) + + err := r.Download(context.Background(), prependURL, source) + + assert.NoError(t, err) + calls := mockRunner.Calls() + assert.Len(t, calls, 1) + + // Should use complete URL without prepending + expectedCommand := []string{"inbc", "sota", "--mode", "download-only", "--signature", sha, "--uri", completeURL} + assert.Equal(t, expectedCommand, calls[0].Args) +} + +func TestIsCompleteURL(t *testing.T) { + tests := []struct { + name string + url string + expected bool + }{ + {"https URL", "https://example.com/path", true}, + {"http URL", "http://example.com/path", true}, + {"relative path", "repository/images/file.gz", false}, + {"absolute path", "/var/cache/file.gz", false}, + {"empty string", "", false}, + {"ftp URL", "ftp://example.com/file", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isCompleteURL(tt.url) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/platform-update-agent/internal/updater/updater.go b/platform-update-agent/internal/updater/updater.go index de872be7..2cfb0757 100644 --- a/platform-update-agent/internal/updater/updater.go +++ b/platform-update-agent/internal/updater/updater.go @@ -213,7 +213,18 @@ func (u *UpdateController) StartUpdate(durationSeconds int64) { log.Infof("Starting Edge Node Update.") - err := u.metaController.SetMetaUpdateStatus(pb.UpdateStatus_STATUS_TYPE_STARTED) + // Clean up granular log from previous update before starting new update + err := u.cleaner.CleanupAfterUpdate(u.granularLogPath) + if err != nil { + log.Warnf("Failed to cleanup granular log before update: %v", err) + } + + // Also clear the metadata UpdateLog to prevent stale logs from appearing + if err := u.metaController.SetMetaUpdateLog(""); err != nil { + log.Warnf("Failed to clear metadata update log before update: %v", err) + } + + err = u.metaController.SetMetaUpdateStatus(pb.UpdateStatus_STATUS_TYPE_STARTED) if err != nil { log.Errorf("failed to set metadata - %v", err) return diff --git a/platform-update-agent/internal/updater/updater_test.go b/platform-update-agent/internal/updater/updater_test.go index 8589a3ee..20962c3b 100644 --- a/platform-update-agent/internal/updater/updater_test.go +++ b/platform-update-agent/internal/updater/updater_test.go @@ -65,7 +65,11 @@ func TestUpdater_StartUpdate_handleErrorThrownBySetMetaUpdateStatus(t *testing.T interceptedStatusType = s return fmt.Errorf("SetMetaUpdateStatusError") }, + SetMetaUpdateLog: func(s string) error { + return nil + }, }, + cleaner: &MockCleaner{}, } assert.NotEqual(t, pb.UpdateStatus_STATUS_TYPE_STARTED, interceptedStatusType) @@ -93,6 +97,9 @@ func TestUpdater_StartUpdate_handleErrorThrownBySetMetaUpdateTime(t *testing.T) require.Fail(t, "SetMetaUpdateDuration function shouldn't be called") return nil }, + SetMetaUpdateLog: func(s string) error { + return nil + }, }, edgeNodeUpdater: testUpdater{ updateFn: func() error { @@ -103,6 +110,7 @@ func TestUpdater_StartUpdate_handleErrorThrownBySetMetaUpdateTime(t *testing.T) timeNow: func() time.Time { return now }, + cleaner: &MockCleaner{}, } u.StartUpdate(1) @@ -139,6 +147,9 @@ func TestUpdater_StartUpdate_handleErrorThrownBySetMetaUpdateDuration(t *testing interceptedUpdateDuration = updateDuration return fmt.Errorf("SetMetaUpdateDurationError") }, + SetMetaUpdateLog: func(s string) error { + return nil + }, }, edgeNodeUpdater: testUpdater{ updateFn: func() error { @@ -149,6 +160,7 @@ func TestUpdater_StartUpdate_handleErrorThrownBySetMetaUpdateDuration(t *testing timeNow: func() time.Time { return now }, + cleaner: &MockCleaner{}, } u.StartUpdate(1) @@ -241,6 +253,9 @@ func TestUpdater_StartUpdate_happyPath(t *testing.T) { interceptedUpdateDuration = updateDuration return nil }, + SetMetaUpdateLog: func(s string) error { + return nil + }, }, edgeNodeUpdater: testUpdater{updateFn: func() error { interceptedUpdateAllExecution = true @@ -249,6 +264,7 @@ func TestUpdater_StartUpdate_happyPath(t *testing.T) { timeNow: func() time.Time { return now }, + cleaner: &MockCleaner{}, } u.StartUpdate(1)