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 linux pack actions For CentOS 8 #5035

Merged
merged 17 commits into from
Apr 2, 2022

Conversation

winem
Copy link
Contributor

@winem winem commented Sep 4, 2020

Part of #4999

This PR addresses the actions that fail on CentOS8 but work on other operating systems and distributions and writes the output of the load_diag actionchain to a json file which is parsable via jq or other tools and therefore hopefully a bit more useful to the users.

I did not add any tests yet. I would do this in another PR or give others the chance to add tests.

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Sep 4, 2020
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks for trying fixing this EL8 issue!

Per comments in this PR, we can't rely on a python 2 env only considering active development and aim towards the Python 2 Deprecation.

There might be other ways to explore to fix the pack.

@arm4b arm4b added bug OS support Support/issues/PRs on a specific OS labels Sep 17, 2020
@arm4b
Copy link
Member

arm4b commented Sep 17, 2020

BTW is this an issue only on EL8 or in any python3 environment, including Ubuntu Bionic?

@winem
Copy link
Contributor Author

winem commented Sep 17, 2020

This issue applies to CentOS8 only. Ubuntu 18.04 has python in the path which is linked to one of the installed Python versions. I don't know for sure which one but I think it's the Python3.x

That's why a shebang like #!/usr/bin/env python works on the supported Ubuntu distributions.

@arm4b
Copy link
Member

arm4b commented Sep 17, 2020

Can you elaborate more on the root cause of the issue? Is just #!/usr/bin/env python failing to find the python binary in EL8 system or something else?
If so, can we hardcode it and use the st2 py one, like #!/opt/stackstorm/st2/bin/python? This would also use the st2 virtualenv with all its installed dependencies.

@winem
Copy link
Contributor Author

winem commented Sep 18, 2020

Can you elaborate more on the root cause of the issue? Is just #!/usr/bin/env python failing to find the python binary in EL8 system or something else?
If so, can we hardcode it and use the st2 py one, like #!/opt/stackstorm/st2/bin/python? This would also use the st2 virtualenv with all its installed dependencies.

Exactly, that should to it and seems to be an optimal solution. I'll test this tonight!

CHANGELOG.rst Outdated Show resolved Hide resolved
@winem
Copy link
Contributor Author

winem commented Sep 19, 2020

Can you elaborate more on the root cause of the issue? Is just #!/usr/bin/env python failing to find the python binary in EL8 system or something else?
If so, can we hardcode it and use the st2 py one, like #!/opt/stackstorm/st2/bin/python? This would also use the st2 virtualenv with all its installed dependencies.

Thanks, this works like a charm and it's finally a solution that satisfies me! Latest commit includes the change.

@@ -47,5 +43,5 @@
name: "dump_results"
ref: "core.local"
parameters:
cmd: "echo \"ST2 Workflow\tdiag_loadavg:\t{{__results}}\" >> /tmp/diag_loadavg && echo 'Output written to file /tmp/diag_loadavg'"
cmd: "echo \"{{__results}}\" | python2 -c \"import json; import sys; from datetime import datetime; diag_data=eval(sys.stdin.read()); diag_data['timestamp'] = str(datetime.now()); print(json.dumps(diag_data))\" >> /tmp/diag_loadavg.json && echo 'Output written to file /tmp/diag_loadavg.json'"
Copy link
Member

@arm4b arm4b Sep 22, 2020

Choose a reason for hiding this comment

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

Looks like this is the only place with the hardcoded python2 now.
Is it possible to do something with that so we don't rely on a specific version?

Besides of that, looks good.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, any reason why we need to use python2 here? I believe that line should also work fine with Python 3 (aka just using python system binary - should work if it's either python 2 or python 3).

Copy link
Member

Choose a reason for hiding this comment

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

I also wonder if there is a better way which doesn't rely on eval() which can be unsafe, especially in this context when we are passing 3rd party data to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any issues regarding py3 and it should work as well (I can put that on my list again).

Let's also tackle the eval. The safe alternative without a bunch of JSON processing on stdin is ast.literal_eval: https://docs.python.org/3/library/ast.html#ast.literal_eval

Would this be fine for everyone? Last resort would be some manual parsing & processing with the JSON library.

Copy link
Member

Choose a reason for hiding this comment

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

ast.literal_eval is awesome (and quite safe)! go for it.

This comment was marked as outdated.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

It doesn't look like the black reformat will interfere with this PR much. (hooray!) Master still needs to be merged in though (looks pretty minor).

@@ -47,5 +43,5 @@
name: "dump_results"
ref: "core.local"
parameters:
cmd: "echo \"ST2 Workflow\tdiag_loadavg:\t{{__results}}\" >> /tmp/diag_loadavg && echo 'Output written to file /tmp/diag_loadavg'"
cmd: "echo \"{{__results}}\" | python2 -c \"import json; import sys; from datetime import datetime; diag_data=eval(sys.stdin.read()); diag_data['timestamp'] = str(datetime.now()); print(json.dumps(diag_data))\" >> /tmp/diag_loadavg.json && echo 'Output written to file /tmp/diag_loadavg.json'"
Copy link
Member

Choose a reason for hiding this comment

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

ast.literal_eval is awesome (and quite safe)! go for it.

@CLAassistant
Copy link

CLAassistant commented Sep 6, 2021

CLA assistant check
All committers have signed the CLA.

@cognifloyd
Copy link
Member

I merged in master. Hopefully we can get this bugfix over the hump quickly. If not in 3.6.0, then in 3.7.0.

@cognifloyd cognifloyd added the status:needs cla A manual tag to note PRs that are otherwise ready, but the CLA bot says the CLA has not been signed. label Oct 5, 2021
@cognifloyd cognifloyd added this to the 3.6.0 milestone Oct 7, 2021
@cognifloyd cognifloyd removed the status:needs cla A manual tag to note PRs that are otherwise ready, but the CLA bot says the CLA has not been signed. label Apr 1, 2022
@cognifloyd cognifloyd modified the milestones: 3.6.0, 3.7.0 Apr 1, 2022
@cognifloyd cognifloyd requested a review from arm4b April 1, 2022 18:47
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

There's a need to update the pack version following the changes in this PR:

version : 1.0.2

otherwise looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug OS support Support/issues/PRs on a specific OS size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants