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

Copy target min python check into ansiballz template #82690

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

sivel
Copy link
Member

@sivel sivel commented Feb 14, 2024

SUMMARY

Copy target min python check into ansiballz template. Fixes #82068

ISSUE TYPE
  • Bugfix Pull Request
ADDITIONAL INFORMATION
porky | FAILED! => {
    "changed": false,
    "msg": "ansible-core requires a minimum of Python version 3.7. Current version: 3.6.12 (default, Sep 17 2020, 10:43:21) [GCC 9.3.0]"
}

@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. has_issue needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 14, 2024
Copy link
Member

@mattclay mattclay left a comment

Choose a reason for hiding this comment

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

The code changes look fine. However, my original concerns mentioned in #82068 (comment) still apply:

  • It's difficult to automate testing for the Python version check, since it requires a Python version that we don't otherwise support.
  • If we decide to use newer Python syntax in the AnsiballZ wrapper at some point, then the check will have the same issues the one in basic.py currently has.

@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Feb 15, 2024
"{0}. Current version: {1}"
).format(
'.'.join(map(str, _PY_MIN)),
''.join(sys.version.splitlines()),
Copy link

@jdicke jdicke Feb 16, 2024

Choose a reason for hiding this comment

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

The sys.version attribute typically returns a string with additional information, such as the build date and other details. Using splitlines() on sys.version might not give you the desired version format.

@webknjaz

This comment was marked as resolved.

This comment was marked as resolved.

@webknjaz
Copy link
Member

Looks like restarting the CI doesn't help clear out those macOS issues. This seems related to #73505. At the moment, I don't understand why this PR is affected but others don't seem to be...

@webknjaz
Copy link
Member

webknjaz commented Feb 17, 2024

I checked this out locally, merged in devel and it keeps failing with --remote macos/14.3 consistently, just like with --remote macos/13.2. And switching to clean devel doesn't have this issue.

I don't know what it is exactly, but clearly the PR is somehow causing the following integration tests to fail:

  • become
  • copy
  • keyword_inheritance
  • ansible-galaxy-collection-scm

It looks like something removes the underlying working directory while modules are being executed. And then, os.getcwd() gets called and that's where that OSError is coming from. For some reason, this seems to be happening in conjunction with become: yes.

In case of copy, there's actually a visible removal in the log:

<testhost> EXEC /bin/sh -c 'sudo -H -S -n  -u nobody /bin/sh -c '"'"'echo BECOME-SUCCESS-qcbxuucefxjxyispipwalmtonykdixaa ; /usr/local/bin/python3.11'"'"' && sleep 0'
<testhost> EXEC /bin/sh -c 'rm -f -r /var/tmp/ansible-tmp-1708120536.920125-7434-145564630886208/ > /dev/null 2>&1 && sleep 0'
fatal: [testhost]: FAILED! => {
    "msg": "Failed to get information on remote file (/tmp/worldwritable/file.txt): shell-init: error retrieving current directory: getcwd: cannot access parent directories: Permission denied\njob-working-directory: error retrieving current directory: getcwd: cannot access parent directories: Permission denied\nTraceback (most recent call last):\n  File \"<stdin>\", line 6, in <module>\n  File \"<frozen importlib._bootstrap>\", line 1178, in _find_and_load\n  File \"<frozen importlib._bootstrap>\", line 1140, in _find_and_load_unlocked\n  File \"<frozen importlib._bootstrap>\", line 1080, in _find_spec\n  File \"<frozen importlib._bootstrap_external>\", line 1504, in find_spec\n  File \"<frozen importlib._bootstrap_external>\", line 1473, in _get_spec\n  File \"<frozen importlib._bootstrap_external>\", line 1431, in _path_importer_cache\nPermissionError: [Errno 13] Permission denied\n"
}

(https://dev.azure.com/ansible/ansible/_build/results?buildId=104232&view=logs&j=a5bc2140-af2a-524a-0c25-b9b66206fded&t=b4892af7-c3bb-5bba-9fe6-5272a87e2a89&l=19477)

I unescaped the traceback for readability:

Failed to get information on remote file (/tmp/worldwritable/file.txt): shell-init: error retrieving current directory: getcwd: cannot access parent directories: Permission denied
job-working-directory: error retrieving current directory: getcwd: cannot access parent directories: Permission denied
Traceback (most recent call last):
  File "<stdin>", line 6, in <module>
  File "<frozen importlib._bootstrap>", line 1178, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1140, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1080, in _find_spec
  File "<frozen importlib._bootstrap_external>", line 1504, in find_spec
  File "<frozen importlib._bootstrap_external>", line 1473, in _get_spec
  File "<frozen importlib._bootstrap_external>", line 1431, in _path_importer_cache
PermissionError: [Errno 13] Permission denied

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Feb 17, 2024
@@ -104,6 +126,9 @@
# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
# USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

%(min_python)s
Copy link
Member

Choose a reason for hiding this comment

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

Move this down below the existing check (if sys.version_info < (3,):) -- that will fix the macOS failures.

@@ -74,6 +74,28 @@

# ******************************************************************************

ANSIBALLZ_MIN_PYTHON = '''
import sys
Copy link
Member

Choose a reason for hiding this comment

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

Remove this import, it won't be needed once the insertion point is moved.

@@ -74,6 +74,28 @@

# ******************************************************************************

ANSIBALLZ_MIN_PYTHON = '''
import sys
import json
Copy link
Member

Choose a reason for hiding this comment

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

Move this import just above the print statement below, so it's not imported here unless it's needed.

Copy link
Member

Choose a reason for hiding this comment

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

I just verified that import json is definitely what's calling os.getcwd() somewhere during import time. Commenting the lines below doesn't make the error go away, dropping this import too does.

Copy link
Member

Choose a reason for hiding this comment

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

Underlying problem reproducer:

root@administrators-Mac:~/ansible$ mkdir /tmp/oserror-repro
root@administrators-Mac:~/ansible$ cd /tmp/oserror-repro
root@administrators-Mac:/tmp/oserror-repro$ chmod 777 .
root@administrator python3 -c 'import time; time.sleep(5); from os import getcwd; print(f"{getcwd()= !r}")' &
[1] 570
root@administrators-Mac:/tmp/oserror-repro$ chmod 000 /tmp/oserror-repro
root@administrators-Mac:/tmp/oserror-repro$ Traceback (most recent call last):
  File "<string>", line 1, in <module>
PermissionError: [Errno 13] Permission denied

[1]+  Exit 1                  sudo -u administrator python3 -c 'import time; time.sleep(5); from os import getcwd; print(f"{getcwd()= !r}")'

root@administrators-Mac:/tmp/oserror-repro$ chmod 777 /tmp/oserror-repro
root@administrators-Mac:/tmp/oserror-repro$ sudo -u administrator python3 -c 'import time; time.sleep(5); import json' &
[1] 633
root@administrators-Mac:/tmp/oserror-repro$ chmod 000 /tmp/oserror-repro
root@administrators-Mac:/tmp/oserror-repro$ Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "<frozen importlib._bootstrap>", line 1178, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1140, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1080, in _find_spec
  File "<frozen importlib._bootstrap_external>", line 1504, in find_spec
  File "<frozen importlib._bootstrap_external>", line 1473, in _get_spec
  File "<frozen importlib._bootstrap_external>", line 1431, in _path_importer_cache
PermissionError: [Errno 13] Permission denied

[1]+  Exit 1                  sudo -u administrator python3 -c 'import time; time.sleep(5); import json'

Copy link
Member

@webknjaz webknjaz Feb 20, 2024

Choose a reason for hiding this comment

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

This is happening with other modules, some of which are imported by json. Namely, _json, re, _re and sysconfig. But not codecs.

I found that the exception in the traceback is coming from the call to os.getcwd() @ https://github.com/python/cpython/blob/v3.11.2/Lib/importlib/_bootstrap_external.py#L1431 which indicates a bug in CPython stdlib. There is only one place in that module that guards against a PermissionError that was added over a decade ago (python/cpython@a9976b3).

Not sure which imports hit that code path, though. Perhaps the ones loading C-extensions... In case of sysconfig, though, it's happening right in the Python module during import.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug. ci_verified Changes made in this PR are causing tests to fail. has_issue needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SyntaxError: future feature annotations is not defined
6 participants