Skip to content

[flaky test] TestDBResiliencyYugabyteScenarios: cluster teardown fails on an already-exited container #760

Description

@liran-funaro

TestDBResiliencyYugabyteScenarios/leader-master-and-tablet failed in CI on #759 with no failing
assertion in the test body
— the only failure is in the cluster teardown, after the scenario itself
had already passed.

    yugabyte.go:122: stopping and removing node: sc_test_yuga_tablet_c1d55b32-...
    yugabyte.go:122:
        	Error Trace:	utils/testdb/container.go:449
        	            	utils/testdb/container.go:438
        	            	integration/runner/cluster_controller.go:106
        	            	integration/runner/yugabyte.go:122
        	            	testing.go:1317        # t.Cleanup
        	Error:      	Received unexpected error:
        	            	Container not running: b5c2725e0972212bd527220ff71844e328a0bfecf6e9e5b1fe7ee13a86f28af6
        	Test:       	TestDBResiliencyYugabyteScenarios/leader-master-and-tablet
    --- FAIL: TestDBResiliencyYugabyteScenarios/leader-master-and-tablet (410.79s)
DONE 10 tests, 2 failures in 414.497s

Teardown treats an already-exited container as a test failure

StopAndRemoveContainer stops before removing, and the stop is fatal
(utils/testdb/container.go:447):

func (dc *DatabaseContainer) StopAndRemoveContainer(t *testing.T) {
	...
	dc.StopContainer(t)                                  // container.go:438
	require.NoError(t, dc.client.RemoveContainer(docker.RemoveContainerOptions{
		ID: dc.ContainerID(), Force: true,
	}))
}

// StopContainer stops db container.
func (dc *DatabaseContainer) StopContainer(t *testing.T) {
	require.NoError(t, dc.client.StopContainer(dc.ContainerID(), 10))   // container.go:449
}

go-dockerclient returns a typed *docker.ContainerNotRunning when the container has already exited
(go-dockerclient@v1.13.2/container.go:596), whose Error() is literally
"Container not running: " + ID — the message above. So any container that exits on its own before
teardown reaches it turns a passing test into a failing one.

Why a node is already gone in this scenario

stopAndRemoveCluster walks cc.nodes in creation order
(cluster_controller.go:102),
and StartYugaCluster creates all masters before all tablets, so every master is stopped before the
first tablet
. A tserver left with no reachable master can exit by itself in that window.

leader-master-and-tablet makes that window much more likely: it has already removed one tablet and
the leader master during the test, so teardown starts from a 2-master/2-tablet cluster. The subtest also
ran 410.79s against a typical 75-90s, consistent with a cluster that was already struggling.

Note this is not the deliberately-removed nodes being revisited —
StopAndRemoveSingleNodeByIndex drops them from cc.nodes (cluster_controller.go:41).

The two teardown paths disagree

utils/testdb already has a tolerant version of exactly this call, for the shared unit-test container
(database_setup.go:204):

if err := dc.client.StopContainer(dc.ContainerID(), 10); err != nil {
	log.Printf("Warning: failed to stop container %s: %v", dc.Name, err)
}

and start already handles the symmetric case with a typed check (container.go:136-142):

var containerAlreadyRunning *docker.ContainerAlreadyRunning
if err != nil && !errors.As(err, &containerAlreadyRunning) {
	return err
}

So the fatal path is the odd one out.

Suggested fix

StopContainer is only ever reached from StopAndRemoveContainer (its sole caller), so making it
tolerant cannot mask a stop failure anywhere else. Either:

  1. Mirror the existing typed check — ignore *docker.ContainerNotRunning, and *docker.NoSuchContainer
    for a container removed by something else:

    func (dc *DatabaseContainer) StopContainer(t *testing.T) {
    	t.Helper()
    	err := dc.client.StopContainer(dc.ContainerID(), 10)
    	var notRunning *docker.ContainerNotRunning
    	var noSuchContainer *docker.NoSuchContainer
    	if errors.As(err, &notRunning) || errors.As(err, &noSuchContainer) {
    		return // already gone; teardown has nothing to do
    	}
    	require.NoError(t, err)
    }
  2. Or drop the stop from teardown altogether: RemoveContainer is already called with Force: true,
    which stops a running container, so the separate graceful stop buys teardown nothing.

Either way StopAndRemoveContainer becomes idempotent for a container in any state, matching the
containerID == "" guard it already has for one that was never created.

Impact

A cleanup-only failure on a scenario whose own assertions all passed — so the reported failure points at
the wrong thing, and it blocks the PR's required check on a manual re-run. Distinct from #755 (fixed by
#758): there is no panic here, the binary survives, and the other 8 tests report normally.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions