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

Upgrades the dependencies to latest versions #92

Closed

Conversation

TrilokGeer
Copy link
Contributor

@TrilokGeer TrilokGeer commented Sep 4, 2024

Description of the change:

Upgrades the dependencies to latest versions of python, ansible-core and collections.

Motivation for the change:

As the packages are either not supported or nearing EOL by this year end, upgrading the packages for better maintenance and support.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2024
@acornett21
Copy link
Contributor

acornett21 commented Sep 13, 2024

@TrilokGeer Are you running into issues with

https://github.com/ansible/ansible-runner-http

being on an older version requests-unixsocket?

- relates: #88

&& yum install -y libffi-devel openssl-devel python39-devel gcc python39-pip python39-setuptools \
&& pip3 install --upgrade pip~=23.3.2 \
&& pip3 install pipenv==2023.11.15 \
&& pipenv install --deploy \
Copy link
Contributor Author

@TrilokGeer TrilokGeer Sep 15, 2024

Choose a reason for hiding this comment

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

Pipenv usage seems to contradict by installing packages to global path and also, blocks usage of different version when used with requires version. As the container provides isolation, using pip installer performs the same installation on the container layers without conflicts.

Choose a reason for hiding this comment

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

The following issue and PRs provide a context as to why Pipfile and Pipfile.lock were added:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point here is about the conflicts with respect to package installation path. The package management is still being managed by pipenv with usage of pipfile and pipfile.lock. Using PIPENV_SYSTEM=1 enables installing the packages outside the virtual env set by pipenv. This resorts to using underlying system's python version than the desired version with which virtual env is instantiated.

&& yum install -y libffi-devel openssl-devel gcc python3.11-devel python3.11-pip python3.11-setuptools \
&& pip3.11 install --upgrade pip~=24.2 \
&& pip3.11 install pipenv \
&& pipenv requirements > requirements.txt \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using pipenv for now to reuse piplock.

Choose a reason for hiding this comment

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

I think creating the requirements.txt file while building the image may have some issues while reproducing it locally as was the ask in operator-framework/operator-sdk#4237.
We can have fixed dependencies list beforehand and then use it while building the image. This can be achieved in 2 ways:

  • Either we continue with the same method and generate the Pipfile.lock as mentioned here:
    We build the base image using the Dockerfile, which validates the python requirements scaffolding that it copies from this directory.
    To update the requirements (`Pipfile` and `Pipfile.lock`) build and execute the image generated by `pipfile.Dockerfile` like so:
    1. docker build -f ./pipfile.Dockerfile -t pipfile-generator .
    2. docker run --rm -it -v .:/tmp/pip-airlock:Z pipfile-generator
    3. Commit the newly-generated `Pipfile.lock` file (NB: this directory is in root .gitignore file, so you must `git add -f`)
  • Or we generate the requirements.txt file along with Pipfile.lock and then use the requirements.txt file during building the image.

Copy link
Contributor Author

@TrilokGeer TrilokGeer Sep 20, 2024

Choose a reason for hiding this comment

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

  • Or we generate the requirements.txt file along with Pipfile.lock and then use the requirements.txt file during building the image.

The requirements.txt is generated based on Pipfile.lock, refer https://pipenv.pypa.io/en/latest/cli.html#requirements.
Regarding the issue mentioned, the dependencies are managed via pipfile.

Choose a reason for hiding this comment

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

Ack. But I think the Pipfile.lock file is not updated after your last change to the Pipfile. I used both the files to generate the requirements.txt.backup file. After that I updated the Pipfile.lock using the latest Pipfile and then generated the requirements.txt. Following is the diff between the files:

# diff -y requirements.txt requirements.txt.backup 
-i https://pypi.org/simple					-i https://pypi.org/simple
annotated-types==0.7.0; python_version >= '3.8'			annotated-types==0.7.0; python_version >= '3.8'
							      >	ansible==10.4.0; python_version >= '3.10'
ansible-core==2.17.4; python_version >= '3.10'			ansible-core==2.17.4; python_version >= '3.10'
ansible-runner==2.4.0; python_version >= '3.9'			ansible-runner==2.4.0; python_version >= '3.9'
ansible-runner-http==1.0.0					ansible-runner-http==1.0.0
authlib==1.3.2; python_version >= '3.8'				authlib==1.3.2; python_version >= '3.8'
cachetools==5.5.0; python_version >= '3.7'			cachetools==5.5.0; python_version >= '3.7'
certifi==2024.8.30; python_version >= '3.6'			certifi==2024.8.30; python_version >= '3.6'
cffi==1.17.1; platform_python_implementation != 'PyPy'		cffi==1.17.1; platform_python_implementation != 'PyPy'
charset-normalizer==3.3.2; python_full_version >= '3.7.0'	charset-normalizer==3.3.2; python_full_version >= '3.7.0'
click==8.1.7; python_version >= '3.7'				click==8.1.7; python_version >= '3.7'
cryptography==43.0.1; python_version >= '3.7'			cryptography==43.0.1; python_version >= '3.7'
docutils==0.21.2; python_version >= '3.9'			docutils==0.21.2; python_version >= '3.9'
dparse==0.6.4b0; python_version >= '3.7'			dparse==0.6.4b0; python_version >= '3.7'
filelock==3.12.4; python_version >= '3.8'			filelock==3.12.4; python_version >= '3.8'
google-auth==2.35.0; python_version >= '3.7'		      |	google-auth==2.34.0; python_version >= '3.7'
idna==3.10; python_version >= '3.6'			      |	idna==3.8; python_version >= '3.6'
jinja2==3.1.4; python_version >= '3.7'				jinja2==3.1.4; python_version >= '3.7'
kubernetes==29.0.0; python_version >= '3.6'			kubernetes==29.0.0; python_version >= '3.6'
lockfile==0.12.2						lockfile==0.12.2
markdown-it-py==3.0.0; python_version >= '3.8'			markdown-it-py==3.0.0; python_version >= '3.8'
markupsafe==2.1.5; python_version >= '3.7'			markupsafe==2.1.5; python_version >= '3.7'
marshmallow==3.22.0; python_version >= '3.8'			marshmallow==3.22.0; python_version >= '3.8'
mdurl==0.1.2; python_version >= '3.7'				mdurl==0.1.2; python_version >= '3.7'
oauthlib==3.2.2; python_version >= '3.6'			oauthlib==3.2.2; python_version >= '3.6'
packaging==24.1; python_version >= '3.8'			packaging==24.1; python_version >= '3.8'
pexpect==4.9.0							pexpect==4.9.0
psutil==6.0.0; python_version >= '2.7' and python_version not	psutil==6.0.0; python_version >= '2.7' and python_version not
ptyprocess==0.7.0						ptyprocess==0.7.0
pyasn1==0.6.1; python_version >= '3.8'				pyasn1==0.6.1; python_version >= '3.8'
pyasn1-modules==0.4.1; python_version >= '3.8'			pyasn1-modules==0.4.1; python_version >= '3.8'
pycparser==2.22; python_version >= '3.8'			pycparser==2.22; python_version >= '3.8'
pydantic==2.9.2; python_version >= '3.8'		      |	pydantic==2.9.1; python_version >= '3.8'
pydantic-core==2.23.4; python_version >= '3.8'		      |	pydantic-core==2.23.3; python_version >= '3.8'
pygments==2.18.0; python_version >= '3.8'			pygments==2.18.0; python_version >= '3.8'
python-daemon==3.0.1; python_version >= '3'			python-daemon==3.0.1; python_version >= '3'
python-dateutil==2.9.0.post0; python_version >= '2.7' and pyt	python-dateutil==2.9.0.post0; python_version >= '2.7' and pyt
pyyaml==6.0.2; python_version >= '3.8'				pyyaml==6.0.2; python_version >= '3.8'
requests==2.31.0; python_version >= '3.7'			requests==2.31.0; python_version >= '3.7'
requests-oauthlib==2.0.0; python_version >= '3.4'		requests-oauthlib==2.0.0; python_version >= '3.4'
requests-unixsocket==0.3.0					requests-unixsocket==0.3.0
resolvelib==1.0.1						resolvelib==1.0.1
rich==13.8.1; python_full_version >= '3.7.0'			rich==13.8.1; python_full_version >= '3.7.0'
rsa==4.9; python_version >= '3.6' and python_version < '4'	rsa==4.9; python_version >= '3.6' and python_version < '4'
ruamel.yaml==0.18.6; python_version >= '3.7'			ruamel.yaml==0.18.6; python_version >= '3.7'
ruamel.yaml.clib==0.2.8; python_version < '3.13' and platform	ruamel.yaml.clib==0.2.8; python_version < '3.13' and platform
safety==3.2.7; python_version >= '3.7'				safety==3.2.7; python_version >= '3.7'
safety-schemas==0.0.5; python_version >= '3.7'			safety-schemas==0.0.5; python_version >= '3.7'
setuptools==75.1.0; python_version >= '3.8'		      |	setuptools==74.1.2; python_version >= '3.8'
shellingham==1.5.4; python_version >= '3.7'			shellingham==1.5.4; python_version >= '3.7'
six==1.16.0; python_version >= '2.7' and python_version not i	six==1.16.0; python_version >= '2.7' and python_version not i
typer==0.12.5; python_version >= '3.7'				typer==0.12.5; python_version >= '3.7'
typing-extensions==4.12.2; python_version >= '3.8'		typing-extensions==4.12.2; python_version >= '3.8'
urllib3==1.26.20; python_version >= '2.7' and python_version 	urllib3==1.26.20; python_version >= '2.7' and python_version 
websocket-client==1.8.0; python_version >= '3.8'		websocket-client==1.8.0; python_version >= '3.8'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch

@TrilokGeer TrilokGeer changed the title [WIP] Upgrades the dependencies to latest versions Upgrades the dependencies to latest versions Sep 15, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2024
@TrilokGeer
Copy link
Contributor Author

@TrilokGeer Are you running into issues with

https://github.com/ansible/ansible-runner-http

being on an older version requests-unixsocket?

@acornett21 , the issue did not surface while running the tests (manually and e2e). Though, there were other issues related to 2.32.0 version of requests (docker/docker-py#3256) and incompatibility between urllib3 version causing bad chunk in response errors.

@acornett21
Copy link
Contributor

@TrilokGeer I actually meant this issue... oops

@TrilokGeer
Copy link
Contributor Author

@TrilokGeer I actually meant this issue... oops

Ah, yes! The issue is present when using 2.32.* versions. The requests version is pinned to 2.31.0 .

@TrilokGeer TrilokGeer changed the title Upgrades the dependencies to latest versions [WIP] Upgrades the dependencies to latest versions Sep 19, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2024
@TrilokGeer TrilokGeer changed the title [WIP] Upgrades the dependencies to latest versions Upgrades the dependencies to latest versions Sep 19, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2024
@TrilokGeer
Copy link
Contributor Author

/label tide/merge-method-squash

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 20, 2024
@TrilokGeer TrilokGeer changed the title Upgrades the dependencies to latest versions [WIP] Upgrades the dependencies to latest versions Sep 20, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2024
@arkadeepsen
Copy link

@TrilokGeer I think we can also update the versions of the ansible-collections here:

- name: operator_sdk.util
version: "0.5.0"
- name: kubernetes.core
version: "2.4.0"
- name: cloud.common
version: "2.1.1"
- name: community.docker
version: "3.10.3"

@TrilokGeer
Copy link
Contributor Author

@TrilokGeer I think we can also update the versions of the ansible-collections here:

- name: operator_sdk.util
version: "0.5.0"
- name: kubernetes.core
version: "2.4.0"
- name: cloud.common
version: "2.1.1"
- name: community.docker
version: "3.10.3"

Thanks for the suggestion, the changes are still in progress. It'd be easier for you to take a look when all the tests are successful.

urllib3 = "~=1.26.17"
urllib3 = "~=1.26.2"

Choose a reason for hiding this comment

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

Are we downgrading the urllib3 version?

Copy link

@arkadeepsen arkadeepsen Sep 24, 2024

Choose a reason for hiding this comment

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

@TrilokGeer TrilokGeer changed the title [WIP] Upgrades the dependencies to latest versions Upgrades the dependencies to latest versions Sep 24, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2024
Comment on lines +12 to +13
safety = "==3.2.7"
PyYAML = "==6.0.2"

Choose a reason for hiding this comment

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

Any specific reason why safety and PyYAML are pinned to the specific versions? I think these will be automatically upgraded to the latest version when we re-generate the Pipfile.lock from the Pipfile when they are not pinned to a specific version.

Copy link
Contributor Author

@TrilokGeer TrilokGeer Sep 24, 2024

Choose a reason for hiding this comment

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

These packages are pinned to have dependent pinned version of these packages as pip freeze is not being used. Pip freeze will result in a list of packages that generate version conflicts. Hence, using this way of installation of these packages pinned with latest possible versions.

@@ -2,7 +2,7 @@
# It is built with dependencies that take a while to download, thus speeding
# up ansible deploy jobs.

FROM registry.access.redhat.com/ubi8/ubi:8.9-1107 AS basebuilder
FROM registry.access.redhat.com/ubi9/ubi:9.4-1214 AS basebuilder

Choose a reason for hiding this comment

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

Since we are upgrading the ubi version as well as the python version, I think the pipfile.Dockerfile should also be updated with the similar changes here:

FROM registry.access.redhat.com/ubi8/ubi:8.9-1107 AS basebuilder
# Install Rust so that we can ensure backwards compatibility with installing/building the cryptography wheel across all platforms
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y
ENV PATH="/root/.cargo/bin:${PATH}"
RUN rustc --version
# Copy python dependencies (including ansible) to be installed using Pipenv
COPY ./Pipfile ./
# Instruct pip(env) not to keep a cache of installed packages,
# to install into the global site-packages and
# to clear the pipenv cache as well
ENV PIP_NO_CACHE_DIR=1 \
PIPENV_SYSTEM=1 \
PIPENV_CLEAR=1
# Ensure fresh metadata rather than cached metadata, install system and pip python deps,
# and remove those not needed at runtime.
RUN set -e && yum clean all && rm -rf /var/cache/yum/* \
&& yum update -y \
&& yum install -y libffi-devel openssl-devel python39-devel gcc python39-pip python39-setuptools \
&& pip3 install --upgrade pip~=23.3.2 \
&& pip3 install pipenv==2023.11.15 \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah!, missed to check-in this file.

&& pip3 install pipenv==2023.11.15 \
&& pipenv install --deploy \
&& yum install -y python3.11 \
&& yum install -y libffi-devel openssl-devel gcc python3.11-devel python3.11-pip python3.11-setuptools \

Choose a reason for hiding this comment

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

Any specific reason to change to python3.11 and not latest python3.12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Historically during the development, there were package conflicts, I believe it was because of cached packages when trying manually. Hence the 3.11 got stuck. Nevertheless, 3.12 seems to work fine with no issues, upgraded the version.

- name: community.docker
version: "3.10.3"
version: "3.12.1"

Choose a reason for hiding this comment

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

The latest version of community.docker is 3.12.2 which was released last week (https://github.com/ansible-collections/community.docker/releases). Should we upgrade 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.

Well, at the time of this work, the latest is 3.12.1., let's check with this upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completed the upgrade, all e2e cases are successful.

@@ -1,4 +1,4 @@
FROM registry.access.redhat.com/ubi8/ubi:8.9-1107 AS basebuilder
FROM registry.access.redhat.com/ubi9/ubi:9.4-1214 AS basebuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: We should update operator-sdk's images to the same version as here to not run into issues.

@acornett21
Copy link
Contributor

I'm not 100% sure, but I believe that the test-ansible GH action needs/should be updated as well.

- uses: actions/setup-python@v4
with:
python-version: '3.9'
- name: Run test e2e ansible molecule
run: |
env
pip3 install --user --upgrade setuptools pip
pip3 install --user ansible-core~=2.15.0
make test-e2e-ansible-molecule

@acornett21
Copy link
Contributor

@arkadeepsen @chiragkyal If your concerns have been addressed would you mind resolving them? Also do you see any other issues with this? We'd like to merge this, and then iterate on the gorelease issue where the cryptography library takes way to long to build on s390x all of a sudden. Thanks for the help on this one, really appreciated.

@arkadeepsen
Copy link

@arkadeepsen @chiragkyal If your concerns have been addressed would you mind resolving them? Also do you see any other issues with this? We'd like to merge this, and then iterate on the gorelease issue where the cryptography library takes way to long to build on s390x all of a sudden. Thanks for the help on this one, really appreciated.

@acornett21 I am good with the changes.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2024
Signed-off-by: Trilok Geer <tgeer@redhat.com>
Signed-off-by: Trilok Geer <tgeer@redhat.com>
…e.Dockerfile

Signed-off-by: Trilok Geer <tgeer@redhat.com>
Signed-off-by: Trilok Geer <tgeer@redhat.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2024
@TrilokGeer
Copy link
Contributor Author

Closing the PR in favour of #101

@TrilokGeer TrilokGeer closed this Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants