Skip to content

Ssh Test and Debugging Fixups #1015

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 87 commits into
base: main
Choose a base branch
from
Draft

Ssh Test and Debugging Fixups #1015

wants to merge 87 commits into from

Conversation

@bpkroth
Copy link
Contributor

@bpkroth bpkroth commented Oct 28, 2025 •
edited
Loading

Pull Request

Title

SSH Test and Debugging Fixups

Description

  • Fixes Intermittent unit test failure at test_remote_ssh_env #1005 by
    • Adding connection attempt retry loop to ssh_service.py
      • Needs proper config handling
    • Adding wait_for_docker_service_healthy check during test fixture setup to avoid a seeming race between when docker claims containers and ready (and we therefore check the dynamic port mapping) vs when they are actually ready.
  • Improves logging infrastructure for pytest in order to
    • capture logs from fixtures so that they can be reported during test failure
    • capture full worker logs
    • upload those logs as artifacts for full review in CI
    • Add ms resolution to the logger formats for those files

Type of Change

  • 🛠️ Bug fix
  • ✨ New feature
  • 🧪 Tests

Testing

  • CI Passes (multiple times)

Additional Notes (optional)

  • Future PRs will add
    • ms resolution for all loggers (and fixup associated tests)

bpkroth and others added 30 commits July 17, 2025 20:11
@bpkroth
Accept the terms of service for conda repos
0678f96
@bpkroth
Make conda output easier for debugging
a3a82f7
@bpkroth
more output adjustments
fcbfc49
@bpkroth
Revert "more output adjustments"
c666915 
This reverts commit fcbfc49.
@bpkroth
Revert "Make conda output easier for debugging"
2dcfa42 
This reverts commit a3a82f7.
@bpkroth
try a suggestion
9965602
@bpkroth
fixups
57b673a
@bpkroth
Merge branch 'main' into ci-fixups
2084a08
@bpkroth
type fixups
ecebe33
@bpkroth
type checking fixups
e7f524d
@bpkroth
use conda by default
1b490ae
@bpkroth
small pyright fixups
9ad7794
@bpkroth
Install pre-built pyrfr from conda to workaround build error.
3c0aa23 
See Also: microsoft#1001
@bpkroth
ignore a deprecation warning in matplotlib
a8d9ccd
@bpkroth
fixups
1862037
@bpkroth
ignore more warnings
b2530a3
@bpkroth
Avoid Debian trixie for now since there's no azure-cli package yet. C…
d914bcc 
…loses microsoft#1002.
@bpkroth
Moby has also been removed from Debian Trixie
b6b5561
@bpkroth
add more debug logging for missing docker support
b059ac3
@bpkroth
more debugging
021f451
@bpkroth
comments
ac57951
@bpkroth
fixup for local testing
4462cdf
@bpkroth
log docker missing warnings
74d94cf
@bpkroth
Split docker test fixtures out so they can come up and down in parallel.
06a9826
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
cba92cd 
for more information, see https://pre-commit.ci
@bpkroth
comments and revert to moby
797ac40
@bpkroth
comments and revert to moby
07924d6
@bpkroth
make some more fixtures available
9870d35
@bpkroth
quotes fixup
c775d89
@bpkroth
upload the coverage.xml file regardless
72aec7b
motus and others added 19 commits October 24, 2025 17:46
@motus
Merge branch 'main' into ssh-test-fixups
e6af465
@bpkroth
avoid mypy error
8334ede
@bpkroth
copilot debugging
4458b57
@bpkroth
capture all output
5a79292
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
59893d7 
for more information, see https://pre-commit.ci
@bpkroth
more debugging hacks
991d504
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
ca59432 
for more information, see https://pre-commit.ci
@bpkroth
more logs
5904817
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
74fa084 
for more information, see https://pre-commit.ci
@bpkroth
test logging
421e2c5
@bpkroth
partial revert to use logger instead of warn()
0f52e5f
@bpkroth
comment
5ddff64
@bpkroth
logging tweaks
a59c2a8
@bpkroth
adding pytest log capture and upload - even on failure
804bb91
@bpkroth
adding a basic healthcheck to look for readiness of the container
ff31b4e
@bpkroth
pylint
dae49cd
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
4b981f8 
for more information, see https://pre-commit.ci
@bpkroth
remove
fcd2266
@bpkroth
remove special testing checks
3d5da84
bpkroth
bpkroth commented Oct 29, 2025
View reviewed changes
bpkroth
bpkroth commented Oct 29, 2025
View reviewed changes
mlos_bench/mlos_bench/services/remote/ssh/ssh_service.py
_LOG.debug(
"%s: Establishing client connection to %s",
connection_id = SshClient.id_from_params(connect_params)
for i in range(3): # TODO: make the retry count configurable
Copy link
Contributor Author

@bpkroth bpkroth Oct 29, 2025

Choose a reason for hiding this comment

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

polish

Copy link
Contributor Author

@bpkroth bpkroth Oct 29, 2025

Choose a reason for hiding this comment

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

and add tests

Copy link
Contributor Author

@bpkroth bpkroth Oct 29, 2025

Choose a reason for hiding this comment

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

and config support

bpkroth
bpkroth commented Oct 29, 2025
View reviewed changes
bpkroth
bpkroth commented Oct 29, 2025
View reviewed changes
conftest.py
file_handler.setLevel(logging.DEBUG)
else:
file_handler.setLevel(logging.INFO)
# logging.basicConfig(level=file_handler.level, format=LOG_FMT, datefmt=DATE_FMT)
Copy link
Contributor Author

@bpkroth bpkroth Oct 29, 2025

Choose a reason for hiding this comment

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

fixme - only apply to console logger for pytest

@bpkroth bpkroth mentioned this pull request Oct 29, 2025
Closed
1 task
@bpkroth bpkroth changed the title Ssh test debugging Ssh Test and Debugging Fixups Oct 29, 2025
bpkroth
bpkroth commented Oct 29, 2025
View reviewed changes
mlos_bench/mlos_bench/tests/services/remote/ssh/__init__.py
# NOTE: this cache may become stale in another worker if the
# container restarts in one and the other worker doesn't notice the new port.
# Optionally check the status of the cached port before returning it.
if not check_port or self.validate_connection():
Copy link
Contributor Author

@bpkroth bpkroth Oct 29, 2025

Choose a reason for hiding this comment

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

TODO (possibly future PR): add a test that uses an unvalidated connection.
Useful to check for SshServer error handling.

For instance:

  1. Connect.
  2. Execute an OOB (docker exec) command that sends a SIGSTOP command to the sshd service.
  3. Try to issue a command over SshHostService.
  4. Check for timeout.
  5. Check for reconnect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Intermittent unit test failure at test_remote_ssh_env

2 participants

@bpkroth @motus