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 publish result curl fail #1974

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Conversation

phoebusm
Copy link
Collaborator

@phoebusm phoebusm commented Nov 1, 2024

Reference Issues/PRs

#1971

What does this implement or fix?

Fix publish benchmark fails

Any other comments?

Test result: https://github.com/man-group/ArcticDB/actions/runs/11678805668
The git command fails due to the testing branch is not master. The S3 exception blocking step has been cleared.

The publish benchmark step fails due to SSL CA cert cannot be found by ArcticDB. It's because of #1129.
Instead of modifying the script to add CA certs to the connection strings, adding the softlink of the CA certs (ubuntu style -> rhel style) is a more staight forward option.

Ubuntu docker container is needed to be used as the native runner image blocks adding softlinks at the required location. Thus the new environment setup step.

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@phoebusm phoebusm linked an issue Nov 5, 2024 that may be closed by this pull request
@phoebusm phoebusm marked this pull request as ready for review November 5, 2024 10:24
@phoebusm phoebusm self-assigned this Nov 5, 2024
Copy link
Collaborator

@poodlewars poodlewars left a comment

Choose a reason for hiding this comment

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

Why isn't the automated certificate detection that you added in the s3_library_adapter.py working?

        if not self._ca_cert_path and not self._ca_cert_dir and platform.system() == "Linux":
            if ssl.get_default_verify_paths().cafile is not None:
                self._ca_cert_path = ssl.get_default_verify_paths().cafile
            if ssl.get_default_verify_paths().capath is not None:
                self._ca_cert_dir = ssl.get_default_verify_paths().capath

@phoebusm
Copy link
Collaborator Author

phoebusm commented Nov 5, 2024

Why isn't the automated certificate detection that you added in the s3_library_adapter.py working?

        if not self._ca_cert_path and not self._ca_cert_dir and platform.system() == "Linux":
            if ssl.get_default_verify_paths().cafile is not None:
                self._ca_cert_path = ssl.get_default_verify_paths().cafile
            if ssl.get_default_verify_paths().capath is not None:
                self._ca_cert_dir = ssl.get_default_verify_paths().capath

Currently real_s3... doesn't support automatically assign ca file and ca path. Adding that bit is easy but when I modified S3Bucket previously for SSL related test, I didn't expect it will be used for tests other in gh or moto. So setting ca directory is skipped on purpose:

# client_cert_dir is skipped on purpose; It will be tested manually in other tests

I will need to revamp this bit or the delicate ssl tests will just collapsed.
A ticket for tracking: #1986

@phoebusm phoebusm merged commit 84678ec into master Nov 5, 2024
126 of 127 checks passed
@phoebusm phoebusm deleted the fix/publish_result_curl_fail branch November 5, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky publish ASV benchmarks
2 participants