Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(stress-threads): kill only labeled docker containers on loaders #9235

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

dimakr
Copy link
Contributor

@dimakr dimakr commented Nov 18, 2024

There is no any filtering of docker instances, when killing containerized stress threads on loader nodes during teardown of a test. In the case of docker backend this can result in deleting all containers in the system.

The change fixes this by labeling stress thread related docker containers when they are created; and deleting only the labeled containers during the teardown.

Fixes: #9027

Testing

  • [ ]

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevant to this change (if needed)

There is no any filtering of docker instances, when killing containerized stress threads on
loader nodes during teardown of a test. In the case of docker backend this can result in
deleting all containers in the system.

The change fixes this by labeling stress thread related docker containers when they
are created; and deleting only the labeled containers during the teardown.

Fixes: scylladb#9027
@dimakr dimakr marked this pull request as ready for review November 18, 2024 10:05
@dimakr dimakr requested a review from fruch November 18, 2024 10:05
Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fruch fruch requested a review from a team November 18, 2024 12:09
Copy link
Contributor

@vponomaryov vponomaryov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@soyacz
Copy link
Contributor

soyacz commented Nov 18, 2024

@dimakr how did you test it?
backports?

@dimakr
Copy link
Contributor Author

dimakr commented Nov 18, 2024

@dimakr how did you test it? backports?

As it is against docker backend I tested/executed it locally (pausing at the corresponding places to ensure that stress containers are labeled as expected and can be filtered by the labels). And ran test-provision-docker as a regression check.
Wrt to backports, from the original issue it is not clear which version it was failing for, but we can probably backport it 2024.x and 6.x branches, what do you think? (although it is docker backend specific, so not many people are probably using the backend, but still, it would nice to have the patch in some recent versions)
@soyacz

@soyacz
Copy link
Contributor

soyacz commented Nov 18, 2024

@dimakr how did you test it? backports?

As it is against docker backend I tested/executed it locally (pausing at the corresponding places to ensure that stress containers are labeled as expected and can be filtered by the labels). And ran test-provision-docker as a regression check. Wrt to backports, from the original issue it is not clear which version it was failing for, but we can probably backport it 2024.x and 6.x branches, what do you think? (although it is docker backend specific, so not many people are probably using the backend, but still, it would nice to have the patch in some recent versions) @soyacz

I suppose people use docker backends with sct master only - as we develop tests on this branch mainly.
LGTM

@soyacz soyacz added the backport/none Backport is not required label Nov 18, 2024
@soyacz soyacz merged commit 87ffeb7 into scylladb:master Nov 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sct docker backend deletes all docker instances on the system in teardown
5 participants