From 8544a2896bb71b307ffb5a01d59cc74ae89b4e5f Mon Sep 17 00:00:00 2001
From: Tiago Queiroz <tiago.queiroz@elastic.co>
Date: Tue, 3 Sep 2024 11:47:26 -0400
Subject: [PATCH 1/5] [Integration Test Framework] fix createTempDir
createTempDir register a test cleanup function to remove the folder it
created, however, on Windows, this folder sometimes fails to be
removed because there are still open file handlers for the files
within the folder.
We fix this problem by retrying to remove the folder with a maximum
overall wait time of 3 seconds. This is a very similar approach to
what Go's t.TempDir does.
---
pkg/testing/fixture.go | 33 +++++++++++++++++++++-
pkg/testing/fixture_other.go | 14 ++++++++++
pkg/testing/fixture_windows.go | 34 +++++++++++++++++++++++
testing/integration/event_logging_test.go | 1 -
4 files changed, 80 insertions(+), 2 deletions(-)
create mode 100644 pkg/testing/fixture_other.go
create mode 100644 pkg/testing/fixture_windows.go
diff --git a/pkg/testing/fixture.go b/pkg/testing/fixture.go
index bdd05e32723..257c5496b33 100644
--- a/pkg/testing/fixture.go
+++ b/pkg/testing/fixture.go
@@ -1209,7 +1209,7 @@ func createTempDir(t *testing.T) string {
cleanup := func() {
if !t.Failed() {
- if err := os.RemoveAll(tempDir); err != nil {
+ if err := removeAll(tempDir); err != nil {
t.Errorf("could not remove temp dir '%s': %s", tempDir, err)
}
} else {
@@ -1221,6 +1221,37 @@ func createTempDir(t *testing.T) string {
return tempDir
}
+// removeAll tries to remove path, if it fails and the error
+// is retry able (only on Windows), it will keep retrying with an exponential
+// backoff up to 3 seconds.
+func removeAll(path string) error {
+ maxBackoffTime := 3 * time.Second
+ myyBackoff := backoff.NewExponentialBackOff(
+ backoff.WithMaxElapsedTime(maxBackoffTime),
+ backoff.WithInitialInterval(time.Millisecond))
+
+ myyBackoff.Reset()
+ for {
+ err := os.RemoveAll(path)
+ if err == nil {
+ break
+ }
+
+ if !isWindowsRetryable(err) {
+ return fmt.Errorf("could not remove temp dir '%s': %s", path, err)
+ }
+
+ interval := myyBackoff.NextBackOff()
+ if interval == backoff.Stop {
+ return fmt.Errorf("could not remove '%s' after waiting for %s: %w", path, maxBackoffTime, err)
+ }
+
+ time.Sleep(interval)
+ }
+
+ return nil
+}
+
type AgentStatusOutput struct {
Info struct {
ID string `json:"id"`
diff --git a/pkg/testing/fixture_other.go b/pkg/testing/fixture_other.go
new file mode 100644
index 00000000000..419ce6dd654
--- /dev/null
+++ b/pkg/testing/fixture_other.go
@@ -0,0 +1,14 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License;
+// you may not use this file except in compliance with the Elastic License.
+
+//go:build !windows
+
+package testing
+
+// isWindowsRetryable reports whether err is a Windows error code
+// that may be fixed by retrying a failed filesystem operation.
+// Source: https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/testing/testing_other.go;l=11-15
+func isWindowsRetryable(err error) bool {
+ return false
+}
diff --git a/pkg/testing/fixture_windows.go b/pkg/testing/fixture_windows.go
new file mode 100644
index 00000000000..2fcd38eee84
--- /dev/null
+++ b/pkg/testing/fixture_windows.go
@@ -0,0 +1,34 @@
+// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+// or more contributor license agreements. Licensed under the Elastic License;
+// you may not use this file except in compliance with the Elastic License.
+
+//go:build windows
+
+package testing
+
+import (
+ "errors"
+ "syscall"
+
+ "golang.org/x/sys/windows"
+)
+
+// isWindowsRetryable reports whether err is a Windows error code
+// that may be fixed by retrying a failed filesystem operation.
+// Source: https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/testing/testing_windows.go;l=17-34
+func isWindowsRetryable(err error) bool {
+ for {
+ unwrapped := errors.Unwrap(err)
+ if unwrapped == nil {
+ break
+ }
+ err = unwrapped
+ }
+ if err == syscall.ERROR_ACCESS_DENIED {
+ return true // Observed in https://go.dev/issue/50051.
+ }
+ if err == windows.ERROR_SHARING_VIOLATION {
+ return true // Observed in https://go.dev/issue/51442.
+ }
+ return false
+}
diff --git a/testing/integration/event_logging_test.go b/testing/integration/event_logging_test.go
index aecd1eb9de2..11a90da8bd5 100644
--- a/testing/integration/event_logging_test.go
+++ b/testing/integration/event_logging_test.go
@@ -75,7 +75,6 @@ func TestEventLogFile(t *testing.T) {
Local: true,
Sudo: false,
})
- t.Skip("Flaky test: https://github.com/elastic/elastic-agent/issues/5397")
ctx, cancel := testcontext.WithDeadline(
t,
context.Background(),
From 756b92fd33e52ac95b91f5684b98cdc04704e9a9 Mon Sep 17 00:00:00 2001
From: Tiago Queiroz <tiago.queiroz@elastic.co>
Date: Tue, 3 Sep 2024 12:18:22 -0400
Subject: [PATCH 2/5] Use the remove function from install
Remove the custom removeAll function in favour of the
install.RemovePath that already exists.
---
pkg/testing/fixture.go | 34 ++--------------------------------
pkg/testing/fixture_other.go | 14 --------------
pkg/testing/fixture_windows.go | 34 ----------------------------------
3 files changed, 2 insertions(+), 80 deletions(-)
delete mode 100644 pkg/testing/fixture_other.go
delete mode 100644 pkg/testing/fixture_windows.go
diff --git a/pkg/testing/fixture.go b/pkg/testing/fixture.go
index 257c5496b33..512181494c9 100644
--- a/pkg/testing/fixture.go
+++ b/pkg/testing/fixture.go
@@ -26,6 +26,7 @@ import (
"github.com/elastic/elastic-agent/internal/pkg/agent/application/paths"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/upgrade/details"
+ "github.com/elastic/elastic-agent/internal/pkg/agent/install"
"github.com/elastic/elastic-agent/pkg/component"
"github.com/elastic/elastic-agent/pkg/control"
"github.com/elastic/elastic-agent/pkg/control/v2/client"
@@ -1209,7 +1210,7 @@ func createTempDir(t *testing.T) string {
cleanup := func() {
if !t.Failed() {
- if err := removeAll(tempDir); err != nil {
+ if err := install.RemovePath(tempDir); err != nil {
t.Errorf("could not remove temp dir '%s': %s", tempDir, err)
}
} else {
@@ -1221,37 +1222,6 @@ func createTempDir(t *testing.T) string {
return tempDir
}
-// removeAll tries to remove path, if it fails and the error
-// is retry able (only on Windows), it will keep retrying with an exponential
-// backoff up to 3 seconds.
-func removeAll(path string) error {
- maxBackoffTime := 3 * time.Second
- myyBackoff := backoff.NewExponentialBackOff(
- backoff.WithMaxElapsedTime(maxBackoffTime),
- backoff.WithInitialInterval(time.Millisecond))
-
- myyBackoff.Reset()
- for {
- err := os.RemoveAll(path)
- if err == nil {
- break
- }
-
- if !isWindowsRetryable(err) {
- return fmt.Errorf("could not remove temp dir '%s': %s", path, err)
- }
-
- interval := myyBackoff.NextBackOff()
- if interval == backoff.Stop {
- return fmt.Errorf("could not remove '%s' after waiting for %s: %w", path, maxBackoffTime, err)
- }
-
- time.Sleep(interval)
- }
-
- return nil
-}
-
type AgentStatusOutput struct {
Info struct {
ID string `json:"id"`
diff --git a/pkg/testing/fixture_other.go b/pkg/testing/fixture_other.go
deleted file mode 100644
index 419ce6dd654..00000000000
--- a/pkg/testing/fixture_other.go
+++ /dev/null
@@ -1,14 +0,0 @@
-// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
-// or more contributor license agreements. Licensed under the Elastic License;
-// you may not use this file except in compliance with the Elastic License.
-
-//go:build !windows
-
-package testing
-
-// isWindowsRetryable reports whether err is a Windows error code
-// that may be fixed by retrying a failed filesystem operation.
-// Source: https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/testing/testing_other.go;l=11-15
-func isWindowsRetryable(err error) bool {
- return false
-}
diff --git a/pkg/testing/fixture_windows.go b/pkg/testing/fixture_windows.go
deleted file mode 100644
index 2fcd38eee84..00000000000
--- a/pkg/testing/fixture_windows.go
+++ /dev/null
@@ -1,34 +0,0 @@
-// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
-// or more contributor license agreements. Licensed under the Elastic License;
-// you may not use this file except in compliance with the Elastic License.
-
-//go:build windows
-
-package testing
-
-import (
- "errors"
- "syscall"
-
- "golang.org/x/sys/windows"
-)
-
-// isWindowsRetryable reports whether err is a Windows error code
-// that may be fixed by retrying a failed filesystem operation.
-// Source: https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/testing/testing_windows.go;l=17-34
-func isWindowsRetryable(err error) bool {
- for {
- unwrapped := errors.Unwrap(err)
- if unwrapped == nil {
- break
- }
- err = unwrapped
- }
- if err == syscall.ERROR_ACCESS_DENIED {
- return true // Observed in https://go.dev/issue/50051.
- }
- if err == windows.ERROR_SHARING_VIOLATION {
- return true // Observed in https://go.dev/issue/51442.
- }
- return false
-}
From a7befe4b72a3e327d6231f675b08958f07cc810f Mon Sep 17 00:00:00 2001
From: Tiago Queiroz <tiago.queiroz@elastic.co>
Date: Tue, 3 Sep 2024 16:48:08 -0400
Subject: [PATCH 3/5] [WIP] Trying to fix TestUpgradeHandler*
---
.../handlers/handler_action_upgrade_test.go | 58 ++++++++++++-------
1 file changed, 37 insertions(+), 21 deletions(-)
diff --git a/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go b/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
index 2f127c699f0..3cd916c9b93 100644
--- a/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
+++ b/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
@@ -6,10 +6,12 @@ package handlers
import (
"context"
+ "errors"
"testing"
"github.com/stretchr/testify/require"
+ "github.com/elastic/elastic-agent-libs/logp"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/coordinator"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/info"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/reexec"
@@ -25,8 +27,9 @@ import (
)
type mockUpgradeManager struct {
- msgChan chan string
- completedChan chan struct{}
+ msgChan chan string
+ successChan chan string
+ errChan chan error
}
func (u *mockUpgradeManager) Upgradeable() bool {
@@ -39,11 +42,13 @@ func (u *mockUpgradeManager) Reload(rawConfig *config.Config) error {
func (u *mockUpgradeManager) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, details *details.Details, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error) {
select {
- case <-u.completedChan:
- u.msgChan <- "completed " + version
+ case msg := <-u.successChan:
+ u.msgChan <- msg
return nil, nil
+ case err := <-u.errChan:
+ u.msgChan <- err.Error()
+ return nil, ctx.Err()
case <-ctx.Done():
- u.msgChan <- "canceled " + version
return nil, ctx.Err()
}
}
@@ -66,7 +71,7 @@ func TestUpgradeHandler(t *testing.T) {
agentInfo := &info.AgentInfo{}
msgChan := make(chan string)
- completedChan := make(chan struct{})
+ completedChan := make(chan string)
// Create and start the coordinator
c := coordinator.New(
@@ -76,7 +81,7 @@ func TestUpgradeHandler(t *testing.T) {
agentInfo,
component.RuntimeSpecs{},
nil,
- &mockUpgradeManager{msgChan: msgChan, completedChan: completedChan},
+ &mockUpgradeManager{msgChan: msgChan, successChan: completedChan},
nil, nil, nil, nil, nil, false)
//nolint:errcheck // We don't need the termination state of the Coordinator
go c.Run(ctx)
@@ -96,6 +101,8 @@ func TestUpgradeHandler(t *testing.T) {
func TestUpgradeHandlerSameVersion(t *testing.T) {
// Create a cancellable context that will shut down the coordinator after
// the test.
+ logp.DevelopmentSetup()
+ logger.SetLevel(logp.DebugLevel)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
@@ -103,7 +110,8 @@ func TestUpgradeHandlerSameVersion(t *testing.T) {
agentInfo := &info.AgentInfo{}
msgChan := make(chan string)
- completedChan := make(chan struct{})
+ successChan := make(chan string)
+ errChan := make(chan error)
// Create and start the Coordinator
c := coordinator.New(
@@ -113,7 +121,7 @@ func TestUpgradeHandlerSameVersion(t *testing.T) {
agentInfo,
component.RuntimeSpecs{},
nil,
- &mockUpgradeManager{msgChan: msgChan, completedChan: completedChan},
+ &mockUpgradeManager{msgChan: msgChan, successChan: successChan, errChan: errChan},
nil, nil, nil, nil, nil, false)
//nolint:errcheck // We don't need the termination state of the Coordinator
go c.Run(ctx)
@@ -126,10 +134,12 @@ func TestUpgradeHandlerSameVersion(t *testing.T) {
err2 := u.Handle(ctx, &a, ack)
require.NoError(t, err1)
require.NoError(t, err2)
- // indicate that upgrade is completed
- close(completedChan)
- msg := <-msgChan
- require.Equal(t, "completed 8.3.0", msg)
+
+ successChan <- "completed 8.3.0"
+ require.Equal(t, "completed 8.3.0", <-msgChan)
+ errChan <- errors.New("duplicated update, not finishing it?")
+ require.Equal(t, "duplicated update, not finishing it?", <-msgChan)
+
}
func TestUpgradeHandlerNewVersion(t *testing.T) {
@@ -142,7 +152,8 @@ func TestUpgradeHandlerNewVersion(t *testing.T) {
agentInfo := &info.AgentInfo{}
msgChan := make(chan string)
- completedChan := make(chan struct{})
+ completedChan := make(chan string)
+ errorChan := make(chan error)
// Create and start the Coordinator
c := coordinator.New(
@@ -152,7 +163,7 @@ func TestUpgradeHandlerNewVersion(t *testing.T) {
agentInfo,
component.RuntimeSpecs{},
nil,
- &mockUpgradeManager{msgChan: msgChan, completedChan: completedChan},
+ &mockUpgradeManager{msgChan: msgChan, successChan: completedChan, errChan: errorChan},
nil, nil, nil, nil, nil, false)
//nolint:errcheck // We don't need the termination state of the Coordinator
go c.Run(ctx)
@@ -163,14 +174,19 @@ func TestUpgradeHandlerNewVersion(t *testing.T) {
a2 := fleetapi.ActionUpgrade{Data: fleetapi.ActionUpgradeData{
Version: "8.5.0", SourceURI: "http://localhost"}}
ack := noopacker.New()
+
+ // Send both upgrade actions, a1 will error before a2 succeeds
err1 := u.Handle(ctx, &a1, ack)
require.NoError(t, err1)
err2 := u.Handle(ctx, &a2, ack)
require.NoError(t, err2)
- msg1 := <-msgChan
- require.Equal(t, "canceled 8.2.0", msg1)
- // indicate that upgrade is completed
- close(completedChan)
- msg2 := <-msgChan
- require.Equal(t, "completed 8.5.0", msg2)
+
+ // Send an error so the first action is "cancelled"
+ errorChan <- errors.New("cancelled 8.2.0")
+ // Wait for the mockUpgradeHandler to receive and "process" the error
+ require.Equal(t, "cancelled 8.2.0", <-msgChan, "mockUpgradeHandler.Upgrade did not receive the expected error")
+
+ // Send a success so the second action succeeds
+ completedChan <- "completed 8.5.0"
+ require.Equal(t, "completed 8.5.0", <-msgChan, "mockUpgradeHandler.Upgrade did not receive the success signal")
}
From 87a5ca05302a04bac100ae7bebab423e3e371f33 Mon Sep 17 00:00:00 2001
From: Tiago Queiroz <tiago.queiroz@elastic.co>
Date: Wed, 4 Sep 2024 15:35:02 -0400
Subject: [PATCH 4/5] Fix flakiness in TestUpgradeHandler*
Fix the flakiness from TestUpgradeHandler* tests by re-working the
mockUpgradeManager, now it accepts a function for its Upgrade method
and their implementation is goroutine safe
---
.../handlers/handler_action_upgrade_test.go | 161 +++++++++++++-----
1 file changed, 118 insertions(+), 43 deletions(-)
diff --git a/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go b/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
index 3cd916c9b93..4ec7e79886d 100644
--- a/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
+++ b/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
@@ -7,7 +7,9 @@ package handlers
import (
"context"
"errors"
+ "sync/atomic"
"testing"
+ "time"
"github.com/stretchr/testify/require"
@@ -27,9 +29,15 @@ import (
)
type mockUpgradeManager struct {
- msgChan chan string
- successChan chan string
- errChan chan error
+ UpgradeFn func(
+ ctx context.Context,
+ version string,
+ sourceURI string,
+ action *fleetapi.ActionUpgrade,
+ details *details.Details,
+ skipVerifyOverride bool,
+ skipDefaultPgp bool,
+ pgpBytes ...string) (reexec.ShutdownCallbackFn, error)
}
func (u *mockUpgradeManager) Upgradeable() bool {
@@ -40,17 +48,25 @@ func (u *mockUpgradeManager) Reload(rawConfig *config.Config) error {
return nil
}
-func (u *mockUpgradeManager) Upgrade(ctx context.Context, version string, sourceURI string, action *fleetapi.ActionUpgrade, details *details.Details, skipVerifyOverride bool, skipDefaultPgp bool, pgpBytes ...string) (_ reexec.ShutdownCallbackFn, err error) {
- select {
- case msg := <-u.successChan:
- u.msgChan <- msg
- return nil, nil
- case err := <-u.errChan:
- u.msgChan <- err.Error()
- return nil, ctx.Err()
- case <-ctx.Done():
- return nil, ctx.Err()
- }
+func (u *mockUpgradeManager) Upgrade(
+ ctx context.Context,
+ version string,
+ sourceURI string,
+ action *fleetapi.ActionUpgrade,
+ details *details.Details,
+ skipVerifyOverride bool,
+ skipDefaultPgp bool,
+ pgpBytes ...string) (reexec.ShutdownCallbackFn, error) {
+
+ return u.UpgradeFn(
+ ctx,
+ version,
+ sourceURI,
+ action,
+ details,
+ skipVerifyOverride,
+ skipDefaultPgp,
+ pgpBytes...)
}
func (u *mockUpgradeManager) Ack(ctx context.Context, acker acker.Acker) error {
@@ -70,8 +86,7 @@ func TestUpgradeHandler(t *testing.T) {
log, _ := logger.New("", false)
agentInfo := &info.AgentInfo{}
- msgChan := make(chan string)
- completedChan := make(chan string)
+ upgradeCalledChan := make(chan struct{})
// Create and start the coordinator
c := coordinator.New(
@@ -81,7 +96,21 @@ func TestUpgradeHandler(t *testing.T) {
agentInfo,
component.RuntimeSpecs{},
nil,
- &mockUpgradeManager{msgChan: msgChan, successChan: completedChan},
+ &mockUpgradeManager{
+ UpgradeFn: func(
+ ctx context.Context,
+ version string,
+ sourceURI string,
+ action *fleetapi.ActionUpgrade,
+ details *details.Details,
+ skipVerifyOverride bool,
+ skipDefaultPgp bool,
+ pgpBytes ...string) (reexec.ShutdownCallbackFn, error) {
+
+ upgradeCalledChan <- struct{}{}
+ return nil, nil
+ },
+ },
nil, nil, nil, nil, nil, false)
//nolint:errcheck // We don't need the termination state of the Coordinator
go c.Run(ctx)
@@ -91,11 +120,14 @@ func TestUpgradeHandler(t *testing.T) {
Version: "8.3.0", SourceURI: "http://localhost"}}
ack := noopacker.New()
err := u.Handle(ctx, &a, ack)
- // indicate that upgrade is completed
- close(completedChan)
require.NoError(t, err)
- msg := <-msgChan
- require.Equal(t, "completed 8.3.0", msg)
+
+ // Make sure this test does not dead lock or wait for too long
+ select {
+ case <-time.Tick(50 * time.Millisecond):
+ t.Fatal("mockUpgradeManager.Upgrade was not called")
+ case <-upgradeCalledChan:
+ }
}
func TestUpgradeHandlerSameVersion(t *testing.T) {
@@ -109,11 +141,10 @@ func TestUpgradeHandlerSameVersion(t *testing.T) {
log, _ := logger.New("", false)
agentInfo := &info.AgentInfo{}
- msgChan := make(chan string)
- successChan := make(chan string)
- errChan := make(chan error)
+ upgradeCalledChan := make(chan struct{})
// Create and start the Coordinator
+ upgradeCalled := atomic.Bool{}
c := coordinator.New(
log,
configuration.DefaultConfiguration(),
@@ -121,7 +152,26 @@ func TestUpgradeHandlerSameVersion(t *testing.T) {
agentInfo,
component.RuntimeSpecs{},
nil,
- &mockUpgradeManager{msgChan: msgChan, successChan: successChan, errChan: errChan},
+ &mockUpgradeManager{
+ UpgradeFn: func(
+ ctx context.Context,
+ version string,
+ sourceURI string,
+ action *fleetapi.ActionUpgrade,
+ details *details.Details,
+ skipVerifyOverride bool,
+ skipDefaultPgp bool,
+ pgpBytes ...string) (reexec.ShutdownCallbackFn, error) {
+
+ if upgradeCalled.CompareAndSwap(false, true) {
+ upgradeCalledChan <- struct{}{}
+ return nil, nil
+ }
+ err := errors.New("mockUpgradeManager.Upgrade called more than once")
+ t.Error(err.Error())
+ return nil, err
+ },
+ },
nil, nil, nil, nil, nil, false)
//nolint:errcheck // We don't need the termination state of the Coordinator
go c.Run(ctx)
@@ -135,11 +185,12 @@ func TestUpgradeHandlerSameVersion(t *testing.T) {
require.NoError(t, err1)
require.NoError(t, err2)
- successChan <- "completed 8.3.0"
- require.Equal(t, "completed 8.3.0", <-msgChan)
- errChan <- errors.New("duplicated update, not finishing it?")
- require.Equal(t, "duplicated update, not finishing it?", <-msgChan)
-
+ // Make sure this test does not dead lock or wait for too long
+ select {
+ case <-time.Tick(50 * time.Millisecond):
+ t.Fatal("mockUpgradeManager.Upgrade was not called")
+ case <-upgradeCalledChan:
+ }
}
func TestUpgradeHandlerNewVersion(t *testing.T) {
@@ -149,11 +200,9 @@ func TestUpgradeHandlerNewVersion(t *testing.T) {
defer cancel()
log, _ := logger.New("", false)
+ upgradeCalledChan := make(chan string)
agentInfo := &info.AgentInfo{}
- msgChan := make(chan string)
- completedChan := make(chan string)
- errorChan := make(chan error)
// Create and start the Coordinator
c := coordinator.New(
@@ -163,7 +212,27 @@ func TestUpgradeHandlerNewVersion(t *testing.T) {
agentInfo,
component.RuntimeSpecs{},
nil,
- &mockUpgradeManager{msgChan: msgChan, successChan: completedChan, errChan: errorChan},
+ &mockUpgradeManager{
+ UpgradeFn: func(
+ ctx context.Context,
+ version string,
+ sourceURI string,
+ action *fleetapi.ActionUpgrade,
+ details *details.Details,
+ skipVerifyOverride bool,
+ skipDefaultPgp bool,
+ pgpBytes ...string) (reexec.ShutdownCallbackFn, error) {
+
+ defer func() {
+ upgradeCalledChan <- version
+ }()
+ if version == "8.2.0" {
+ return nil, errors.New("upgrade to 8.2.0 will always fail")
+ }
+
+ return nil, nil
+ },
+ },
nil, nil, nil, nil, nil, false)
//nolint:errcheck // We don't need the termination state of the Coordinator
go c.Run(ctx)
@@ -175,18 +244,24 @@ func TestUpgradeHandlerNewVersion(t *testing.T) {
Version: "8.5.0", SourceURI: "http://localhost"}}
ack := noopacker.New()
+ checkMsg := func(c <-chan string, expected, errMsg string) {
+ t.Helper()
+ // Make sure this test does not dead lock or wait for too long
+ // For some reason < 1s sometimes makes the test fail.
+ select {
+ case <-time.Tick(1300 * time.Millisecond):
+ t.Fatal("timed out waiting for Upgrade to return")
+ case msg := <-c:
+ require.Equal(t, expected, msg, errMsg)
+ }
+ }
+
// Send both upgrade actions, a1 will error before a2 succeeds
err1 := u.Handle(ctx, &a1, ack)
require.NoError(t, err1)
+ checkMsg(upgradeCalledChan, "8.2.0", "first call must be with version 8.2.0")
+
err2 := u.Handle(ctx, &a2, ack)
require.NoError(t, err2)
-
- // Send an error so the first action is "cancelled"
- errorChan <- errors.New("cancelled 8.2.0")
- // Wait for the mockUpgradeHandler to receive and "process" the error
- require.Equal(t, "cancelled 8.2.0", <-msgChan, "mockUpgradeHandler.Upgrade did not receive the expected error")
-
- // Send a success so the second action succeeds
- completedChan <- "completed 8.5.0"
- require.Equal(t, "completed 8.5.0", <-msgChan, "mockUpgradeHandler.Upgrade did not receive the success signal")
+ checkMsg(upgradeCalledChan, "8.5.0", "second call to Upgrade must be with version 8.5.0")
}
From 9d98ac889c1fbc2af41313036d9cc84a2c7c3a2a Mon Sep 17 00:00:00 2001
From: Tiago Queiroz <tiago.queiroz@elastic.co>
Date: Wed, 4 Sep 2024 15:53:18 -0400
Subject: [PATCH 5/5] remove call to logp.DevelopmentSetup
---
.../actions/handlers/handler_action_upgrade_test.go | 3 ---
1 file changed, 3 deletions(-)
diff --git a/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go b/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
index 4ec7e79886d..84e30dd40df 100644
--- a/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
+++ b/internal/pkg/agent/application/actions/handlers/handler_action_upgrade_test.go
@@ -13,7 +13,6 @@ import (
"github.com/stretchr/testify/require"
- "github.com/elastic/elastic-agent-libs/logp"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/coordinator"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/info"
"github.com/elastic/elastic-agent/internal/pkg/agent/application/reexec"
@@ -133,8 +132,6 @@ func TestUpgradeHandler(t *testing.T) {
func TestUpgradeHandlerSameVersion(t *testing.T) {
// Create a cancellable context that will shut down the coordinator after
// the test.
- logp.DevelopmentSetup()
- logger.SetLevel(logp.DebugLevel)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()