Skip to content

Commit e4f2eb1

Browse files
tests: Improve stability on CI (#1494)
Some of our tests were quite flaky when running on GitHub CI. This PR should fix the issues. More details about each fix are explained in each commit message.
2 parents df3c0be + deda816 commit e4f2eb1

10 files changed

Lines changed: 960 additions & 73 deletions

File tree

‎cmd/adsysd/integration_tests/adsys_test.go‎

Lines changed: 197 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,14 @@ import (
1313
"os"
1414
"os/exec"
1515
"path/filepath"
16+
"slices"
1617
"strconv"
1718
"strings"
1819
"sync"
1920
"testing"
2021
"time"
2122

23+
"github.com/godbus/dbus/v5"
2224
"github.com/stretchr/testify/require"
2325
"github.com/ubuntu/adsys"
2426
"github.com/ubuntu/adsys/cmd/adsysd/client"
@@ -30,6 +32,11 @@ import (
3032

3133
const dockerSystemDaemonsImage = "ghcr.io/ubuntu/adsys/systemdaemons:0.1"
3234

35+
// daemonsStartupTimeout is how long we wait for the mock containers to expose
36+
// their buses. It is deliberately generous: the containers are started in
37+
// parallel and a loaded CI runner can take a while to get them all up.
38+
const daemonsStartupTimeout = 90 * time.Second
39+
3340
var (
3441
rootProjectDir string
3542
)
@@ -437,18 +444,38 @@ func runDaemons() (teardown func()) {
437444
groupFile := writeUserDatabase(dir, "group", "/etc/group")
438445

439446
var errsDocker error
440-
var wg sync.WaitGroup
447+
var errsDockerMu sync.Mutex
448+
// The container goroutines outlive this function, so reads and writes of
449+
// errsDocker must be synchronized.
450+
addDockerErr := func(err error) {
451+
errsDockerMu.Lock()
452+
defer errsDockerMu.Unlock()
453+
errsDocker = errors.Join(errsDocker, err)
454+
}
455+
dockerErr := func() error {
456+
errsDockerMu.Lock()
457+
defer errsDockerMu.Unlock()
458+
return errsDocker
459+
}
460+
461+
var runWG sync.WaitGroup
462+
runDone := make(map[string]chan struct{}, len(answers))
463+
for answer := range answers {
464+
runDone[answer] = make(chan struct{})
465+
}
441466
for answer, socketDir := range answers {
442-
wg.Add(1)
467+
runWG.Add(1)
443468

444-
go func() {
445-
defer wg.Done()
469+
go func(answer, socketDir string, done chan struct{}) {
470+
defer runWG.Done()
471+
defer close(done)
446472

447473
// 24.04 introduced some polkit changes that make the daemon drop root privileges before executing. In order
448474
// to be able to connect to the bus and run polkitd, we need more permissions in the socket directory.
449475
//nolint:gosec
450476
if err := os.MkdirAll(socketDir, 0755); err != nil {
451-
log.Fatalf("Setup: can’t create %s socket directory: %v", answer, err)
477+
addDockerErr(fmt.Errorf("Setup: can’t create %s socket directory: %w", answer, err))
478+
return
452479
}
453480

454481
// #nosec G204: we control the name in tests
@@ -462,50 +489,189 @@ func runDaemons() (teardown func()) {
462489
dockerSystemDaemonsImage,
463490
answer,
464491
)
465-
out, _ := cmd.CombinedOutput()
492+
out, err := cmd.CombinedOutput()
493+
if cmd.ProcessState == nil {
494+
if err == nil {
495+
err = errors.New("command returned without a process state")
496+
}
497+
addDockerErr(fmt.Errorf("Error running system daemons container named %q: %w\n%v", answer, err, string(out)))
498+
return
499+
}
466500
// Docker stop -t 0 will kill it anyway the container with exit code 143 or 137 in mantic (-t 0 does not work there)
467-
if cmd.ProcessState.ExitCode() > 0 && cmd.ProcessState.ExitCode() != 137 && cmd.ProcessState.ExitCode() != 143 {
468-
errsDocker = errors.Join(errsDocker, fmt.Errorf("Error running system daemons container named %q:\nExit code: %d\n%v", answer, cmd.ProcessState.ExitCode(), string(out)))
501+
if exitCode := cmd.ProcessState.ExitCode(); exitCode > 0 && exitCode != 137 && exitCode != 143 {
502+
addDockerErr(fmt.Errorf("Error running system daemons container named %q:\nExit code: %d\n%v", answer, exitCode, string(out)))
469503
}
470-
}()
504+
}(answer, socketDir, runDone[answer])
471505
}
472506

473507
for a, s := range answers {
474508
dbusSockets[a] = fmt.Sprintf("unix:path=%s", s)
475509
}
476510

477-
// give time for polkit containers to start
478-
// TODO: wait for polkit containers to be ready
479-
time.Sleep(5 * time.Second)
511+
// Wait for the polkit containers to be ready instead of hoping a fixed delay
512+
// is enough: a loaded machine may need much longer, and connecting to a bus
513+
// that is not listening yet fails the tests using it.
514+
stopContainers := func() error {
515+
var stopWG sync.WaitGroup
516+
stopErrors := make(chan error, len(answers))
517+
for answer := range answers {
518+
stopWG.Add(1)
519+
go func(answer string) {
520+
defer stopWG.Done()
521+
522+
deadline := time.Now().Add(daemonsStartupTimeout)
523+
var lastErr error
524+
var lastOut string
525+
for {
526+
select {
527+
case <-runDone[answer]:
528+
return
529+
default:
530+
}
531+
if time.Now().After(deadline) {
532+
stopErr := fmt.Errorf("Teardown: can't stop system daemons container %q within %s", answer, daemonsStartupTimeout)
533+
if lastErr != nil {
534+
stopErr = fmt.Errorf("%w (last error: %w: %s)", stopErr, lastErr, lastOut)
535+
}
536+
stopErrors <- stopErr
537+
return
538+
}
539+
540+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
541+
// #nosec G204: we control the args in tests
542+
out, err := exec.CommandContext(ctx, "docker", "stop", "-t", "0", containerName+answer).CombinedOutput()
543+
cancel()
544+
if err != nil {
545+
lastErr = err
546+
lastOut = string(out)
547+
}
548+
select {
549+
case <-runDone[answer]:
550+
return
551+
case <-time.After(100 * time.Millisecond):
552+
}
553+
}
554+
}(answer)
555+
}
556+
557+
stopWG.Wait()
558+
close(stopErrors)
559+
var stopErr error
560+
for err := range stopErrors {
561+
stopErr = errors.Join(stopErr, err)
562+
}
563+
if stopErr != nil {
564+
return stopErr
565+
}
566+
567+
runWG.Wait()
568+
return dockerErr()
569+
}
570+
cleanup := func() error {
571+
stopErr := stopContainers()
572+
if stopErr != nil {
573+
stopErr = fmt.Errorf("can't stop system daemons containers: %w", stopErr)
574+
}
575+
if err := os.RemoveAll(dir); err != nil {
576+
err = fmt.Errorf("failed to delete temporary directory: %w", err)
577+
return errors.Join(stopErr, err)
578+
}
579+
return stopErr
580+
}
480581

481-
// Check if we got startup docker error
482-
if errsDocker != nil {
483-
log.Fatalf("Setup: docker errors (some may still be ALIVE): %v", errsDocker)
582+
if err := waitForDaemons(answers, dockerErr); err != nil {
583+
if cleanupErr := cleanup(); cleanupErr != nil {
584+
err = errors.Join(err, fmt.Errorf("cleanup after setup failure: %w", cleanupErr))
585+
}
586+
log.Fatalf("Setup: %v", err)
484587
}
485588

486589
return func() {
487-
defer func() {
488-
err := os.RemoveAll(dir)
489-
if err != nil {
490-
log.Fatalf("Teardown: failed to delete temporary directory: %v", err)
590+
if err := cleanup(); err != nil {
591+
log.Fatalf("Teardown: %v", err)
592+
}
593+
}
594+
}
595+
596+
// waitForDaemons blocks until every mock container is ready to serve requests,
597+
// or until daemonsStartupTimeout elapses. It fails early if a container reported
598+
// a startup error in the meantime.
599+
func waitForDaemons(answers map[string]string, dockerErr func() error) error {
600+
deadline := time.Now().Add(daemonsStartupTimeout)
601+
ctx, cancel := context.WithDeadline(context.Background(), deadline)
602+
defer cancel()
603+
604+
for answer, socketDir := range answers {
605+
for {
606+
err := daemonsReady(ctx, socketDir)
607+
if err == nil {
608+
break
491609
}
492-
}()
493610

494-
for answer := range answers {
495-
go func() {
496-
// #nosec G204: we control the args in tests
497-
out, err := exec.Command("docker", "stop", "-t", "0", containerName+answer).CombinedOutput()
498-
if err != nil {
499-
errsDocker = errors.Join(errsDocker, fmt.Errorf("Teardown: can't stop system daemons container: %v", string(out)))
500-
}
501-
}()
611+
if dockerErrs := dockerErr(); dockerErrs != nil {
612+
return fmt.Errorf("docker errors while waiting for the %q daemons: %w", answer, dockerErrs)
613+
}
614+
if time.Now().After(deadline) {
615+
return fmt.Errorf("%q daemons not ready after %s: %w", answer, daemonsStartupTimeout, err)
616+
}
617+
618+
time.Sleep(100 * time.Millisecond)
502619
}
620+
}
503621

504-
wg.Wait()
505-
if errsDocker != nil {
506-
log.Fatalf("Teardown: docker errors: %v", errsDocker)
622+
if dockerErrs := dockerErr(); dockerErrs != nil {
623+
return fmt.Errorf("docker errors while waiting for daemon readiness: %w", dockerErrs)
624+
}
625+
626+
return nil
627+
}
628+
629+
// daemonsReady reports whether the mock daemons exposed in socketDir can serve
630+
// requests: both buses must accept connections and polkitd must have claimed its
631+
// name on the system bus, as it is started after them.
632+
func daemonsReady(ctx context.Context, socketDir string) error {
633+
systemSocket := filepath.Join(socketDir, "system_bus_socket")
634+
for _, socket := range []string{systemSocket, filepath.Join(socketDir, "session_bus_socket")} {
635+
conn, err := (&net.Dialer{}).DialContext(ctx, "unix", socket)
636+
if err != nil {
637+
return err
507638
}
639+
_ = conn.Close()
508640
}
641+
642+
netConn, err := (&net.Dialer{}).DialContext(ctx, "unix", systemSocket)
643+
if err != nil {
644+
return err
645+
}
646+
if deadline, ok := ctx.Deadline(); ok {
647+
if err := netConn.SetDeadline(deadline); err != nil {
648+
_ = netConn.Close()
649+
return fmt.Errorf("set D-Bus readiness deadline: %w", err)
650+
}
651+
}
652+
653+
conn, err := dbus.NewConn(netConn, dbus.WithContext(ctx))
654+
if err != nil {
655+
_ = netConn.Close()
656+
return fmt.Errorf("create D-Bus connection: %w", err)
657+
}
658+
defer func() { _ = conn.Close() }()
659+
if err := conn.Auth(nil); err != nil {
660+
return fmt.Errorf("authenticate D-Bus connection: %w", err)
661+
}
662+
if err := conn.Hello(); err != nil {
663+
return fmt.Errorf("complete D-Bus handshake: %w", err)
664+
}
665+
666+
var names []string
667+
if err := conn.BusObject().CallWithContext(ctx, "org.freedesktop.DBus.ListNames", 0).Store(&names); err != nil {
668+
return err
669+
}
670+
if !slices.Contains(names, "org.freedesktop.PolicyKit1") {
671+
return errors.New("polkitd has not claimed its name on the system bus yet")
672+
}
673+
674+
return nil
509675
}
510676

511677
// writeUserDatabase copies the host NSS database file (/etc/passwd or /etc/group)

‎internal/policies/scripts/export_test.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package scripts
22

33
import (
4+
"context"
45
"os/user"
6+
"time"
57
)
68

79
const (
@@ -14,3 +16,15 @@ func WithUserLookup(userLookup func(string) (*user.User, error)) Option {
1416
o.userLookup = userLookup
1517
}
1618
}
19+
20+
// RunScriptsWithBusyScriptRetryHook executes scripts and calls hook when a
21+
// script execution fails with ETXTBSY and will be retried.
22+
func RunScriptsWithBusyScriptRetryHook(ctx context.Context, order string, allowOrderMissing bool, hook func()) error {
23+
return runScripts(ctx, order, allowOrderMissing, hook)
24+
}
25+
26+
// RunScriptsWithBusyScriptRetrySettings executes scripts with explicit ETXTBSY
27+
// retry-loop settings and calls hook when a retry is about to happen.
28+
func RunScriptsWithBusyScriptRetrySettings(ctx context.Context, order string, allowOrderMissing bool, retries int, delay time.Duration, hook func()) error {
29+
return runScriptsWithBusyScriptRetrySettings(ctx, order, allowOrderMissing, retries, delay, hook)
30+
}

0 commit comments

Comments
 (0)