-
Notifications
You must be signed in to change notification settings - Fork 18
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
[DPE-4239] Warn user when deploying charm with wrong architecture #613
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #613 +/- ##
==========================================
+ Coverage 68.65% 68.76% +0.10%
==========================================
Files 10 11 +1
Lines 2983 3009 +26
Branches 564 568 +4
==========================================
+ Hits 2048 2069 +21
- Misses 818 822 +4
- Partials 117 118 +1 ☔ View full report in Codecov by Sentry. |
src/charm.py
Outdated
manifest_path = f"{os.environ.get('CHARM_DIR')}/manifest.yaml" | ||
if os.path.exists(manifest_path): | ||
with open(manifest_path, "r") as manifest: | ||
charm_arch = yaml.safe_load(manifest)["bases"][0]["architectures"][0] | ||
hw_arch = os.uname().machine | ||
if (charm_arch == "amd64" and hw_arch != "x86_64") or ( | ||
charm_arch == "arm64" and hw_arch != "aarch64" | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to move this check at the top of the exception handling and create and run the fake charm only when we detect wrong arch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm: Let's suppose the code enters this error handling block (psycopg2
is borked) but the check doesn't pass (architecture seems to match) - Then we'll skip creating the fake charm and let the real charm fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this check will be redundant in 99.9% of the cases (why would psycopg2 import fail if not due to wrong arch?) but better safe than sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if we split it in a different file we can do something like:
class PostgresqlOperatorCharm(CharmBase):
self.unit.status = BlockedStatus(f"Cannot install: {charm_arch} charm not compatible with {hw_arch} machine")
def check_arch():
if wrong_arch:
main(PostgresqlOperatorCharm(), use_juju_for_storage=True)
sys.exit(0)
And then import and run check_arch()
in the ModuleNotFoundError
exception handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just commited a refactored version, LMK how it looks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fake_charm
is not a meaningful name for the module. Otherwise the refactor LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about arch_utils
? that was the best I could think of tbh, LMK if you have better suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Added unit tests, working on integration tests now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks for the tests.
I have tried to install ARM revision on AMD:
The error:
The story of installing AMD revision on ARM hardware is even worth: all stuck on CC: @dragomirp @lucasgameiroborges (on Monday!) |
Not pushed to edge yet. |
Checked again, it's released and I managed to replicate on Juju 3.5 |
Hi guys! Just saw this issue now, currently investigating what's happening. |
Issue
When deploying a revision that doesn't correspond to the underlying architecture (amd/arm) the charm deployment bugs out with import errors instead of properly warning the user.
Solution
Not very pretty - Try-catching the first platform specific library (psycopg2 in our case) and creating a fake charm blocked with the error message.
I personally don't like it, but that's the only working solution we have so far, since the charm code will run the import statements before everything else. Let me know if you think it's something worth adding.
Tests
Unit and Integration tests added