Skip to content

fix(pull_image): correctly check if node is a docker container #11393

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

Merged
merged 1 commit into from
Jul 15, 2025

Conversation

dimakr
Copy link
Contributor

@dimakr dimakr commented Jul 14, 2025

Previously, the docker_hub_login method in RemoteDocker.pull_image was incorrectly using node.is_docker as a property. Since is_docker is implemented as a method, accessing it without parentheses always evaluated to True value.

This change ensures node.is_docker() is properly called, so the docker_hub_login behaves as intended.

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)

@fruch
Copy link
Contributor

fruch commented Jul 15, 2025

seems like this is the only failure left in the integration tests:

03:07:17  FAILED unit_tests/test_cassandra_stress_thread.py::test_01_cassandra_stress - sdcm.remote.base.SSHConnectTimeoutError: Unable to run "pkill -f '/home/ubuntu/hdrh-cs-write-l1-c0-k1-c2b30902-e7f8-41e5-babc-5326f6bef6a3.hdr'": failed connecting to "ip-52-211-32-93" during 60s

and it's known one related to HDR handling that we need to chase down

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 marked this pull request as ready for review July 15, 2025 06:23
@fruch fruch added test-provision-aws Run provision test on AWS test-provision-gce Run provision test on GCE labels Jul 15, 2025
@fruch
Copy link
Contributor

fruch commented Jul 15, 2025

added provision test for aws and gce, for making sure it doesn't regress those

@dimakr dimakr added test-provision-gce Run provision test on GCE and removed test-provision-gce Run provision test on GCE labels Jul 15, 2025
Previously, the `docker_hub_login` method in `RemoteDocker.pull_image` was incorrectly using
`node.is_docker` as a property. Since `is_docker` is implemented as a method, accessing it
without parentheses always evaluated to True value.

This change ensures `node.is_docker()` is properly called, so the `docker_hub_login`
behaves as intended.
@dimakr dimakr force-pushed the fix_docker_hub_login branch from 28764e3 to 5d88bc0 Compare July 15, 2025 08:11
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

@dimakr
Copy link
Contributor Author

dimakr commented Jul 15, 2025

@fruch Had to re-trigger CI as gce provision test failed the first time (the test itself passed, but pipeline reported a connection error for builder instance). On rerun both tests pass.

Test integration tests are passing now, except for the test_01_cassandra_stress for which the is the issue #11368.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants