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

WIP: upgrade images from ubi7 to ubi8 and python version from 2 which is deprecated to 3 #1895

Closed
wants to merge 15 commits into from

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Sep 9, 2019

Description of the change:

  • Upgrade image registry.access.redhat.com/ubi7/ubi-minimal:latest to registry.access.redhat.com/ubi8/ubi-minimal:latest
  • Upgrade epel7 to epel8 in docker images
  • Upgrade the python version 2 which is deprecated to 3.

Ignore - It was splited.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 9, 2019
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

when all the tests pass

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 9, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@camilamacedo86 camilamacedo86 changed the title upgrade image from ubi7 to ubi8 WIP: upgrade image from ubi7 to ubi8 (required to do few changes) Sep 10, 2019
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 10, 2019
@joelanford
Copy link
Member

Is this related to #1884?

/cc @fabianvf

@camilamacedo86
Copy link
Contributor Author

If we upgrade the python so I understand that we can solve the deprecation from #1884 as well.

c/c @joelanford @fabianvf

@jmrodri
Copy link
Member

jmrodri commented Sep 11, 2019

The builds are failing because of missing pip3

could not wait for build: the build ansible-operator failed after 3m32s with reason DockerBuildFailed: Docker build strategy has failed.

zip.x86_64 0:3.0-11.el7                                                       

Complete!
/bin/sh: pip3: command not found
error: build error: running 'sed -i 's|enabled=1|enabled=0...an all  && rm -rf /var/cache/yum' failed with exit code 127

@camilamacedo86
Copy link
Contributor Author

Hi @jmrodri,

Thank you for your comment. However, my goal here is to upgrade the version of the python as well. I am trying to find out where it still missing to be done.

@camilamacedo86 camilamacedo86 changed the title WIP: upgrade image from ubi7 to ubi8 (required to do few changes) WIP: upgrade images from ubi7 to ubi8 and python version from 2 which is deprecated to 3 Sep 12, 2019
@camilamacedo86 camilamacedo86 added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Sep 12, 2019
@guymguym
Copy link

This will also fix #1656

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

I'm not really familiar with python versions. Should we use the latest python3 that's available, rather than pinning to 3.6? Are all python 3.x versions expected to be compatible like go minor versions are?

Is there such thing as python3-* packages that track the latest?


env:
global:
- PATH=/opt/python/3.6.7/bin:$PATH
Copy link
Member

Choose a reason for hiding this comment

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

Is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is required in order to solve the same scenario https://travis-ci.community/t/please-activate-python-3-by-default-for-non-python-build/3899

Python 3.6 is the default Python when language: python.
But when not language: python, Python 3 can not be used by default.
There is pip (/usr/bin/pip), but not pip3.

.travis.yml Outdated
@@ -9,13 +9,27 @@ sudo: required

# go modules require xenial for mercurial TLS 1.2 support
dist: xenial
python:
- "3.6"
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this is not necessary since python 3.6 is installed by default:

https://docs.travis-ci.com/user/reference/xenial/#python-support

Ditto for line 24. Not sure if we have to install python3-pip in line 25 or if that's also installed already for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pre-installed Python versions: 2.7.15, 3.6.7, and 3.7.1. - Makes sense remove it.

- &pip3_before_install
before_install:
- pip3 install --upgrade pip

Copy link
Member

Choose a reason for hiding this comment

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

Any reason this is defined here rather than directly in the ansible test defintion since we don't use it anywhere else?

Seems like we could add pip3 install --upgrade pip into the before_script.

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Sep 18, 2019

Choose a reason for hiding this comment

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

It is used just in the Ansible one but I think it can be improved by:

 # Build and test ansible
   - <<: *test
     name: Ansible on OpenShift
     before_script: pip3 install --upgrade pip && pip3 install --user ansible
     script: make test/ci-ansible

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that's what I was getting at. Just FYI, I think the following is also supported by travis:

 # Build and test ansible
 - <<: *test
   name: Ansible on OpenShift
   before_script:
     - pip3 install --upgrade pip
     - pip3 install --user ansible
   script: make test/ci-ansible

Which can make it easier to visually parse when there's more than one command.

.travis.yml Outdated
@@ -78,7 +97,8 @@ jobs:
# Build and test ansible
- <<: *test
name: Ansible on OpenShift
before_script: sudo pip install ansible
<<: *pip3_before_install
before_script: pip3 install --user ansible
Copy link
Member

Choose a reason for hiding this comment

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

Just flagging that this changes from sudo pip to pip3 --user. If it passes CI, probably no worries, but I'm not familar enough to know if we made a conscious decision one way or the other.

/cc @fabianvf

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Sep 18, 2019

Choose a reason for hiding this comment

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

Hi @joelanford,

The --user will execute the command with the logged user. In this case, in my understanding, at this point it still in the .trevis's process and the user is root.

Copy link
Member

Choose a reason for hiding this comment

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

My thought was that pip --user might install things somewhere in ${HOME} and sudo pip somewhere in /usr. Just want to make sure we're all okay with this change.

CHANGELOG.md Outdated
@@ -46,6 +46,7 @@
```
- [`pkg/test.FrameworkClient`](https://github.com/operator-framework/operator-sdk/blob/master/pkg/test/client.go#L33) methods `List()` and `Delete()` have new signatures corresponding to the homonymous methods of `sigs.k8s.io/controller-runtime/pkg/client.Client`. ([#1876](https://github.com/operator-framework/operator-sdk/pull/1876))
- CRD file names were previously of the form `<group>_<version>_<kind>_crd.yaml`. Now that CRD manifest `spec.version` is deprecated in favor of `spec.versions`, i.e. multiple versions can be specified in one CRD, CRD file names have the form `<full group>_<resource>_crd.yaml`. `<full group>` is the full group name of your CRD while `<group>` is the last subdomain of `<full group>`, ex. `foo.bar.com` vs `foo`. `<resource>` is the plural lower-case CRD Kind found at `spec.names.plural`. ([#1876](https://github.com/operator-framework/operator-sdk/pull/1876))
- Upgrade image used for Go ad Helm operators and scorecard proxy from `registry.access.redhat.com/ubi7/ubi-minimal:latest` to `Use `registry.access.redhat.com/ubi8/ubi-minimal:latest`. [#1895](https://github.com/operator-framework/operator-sdk/pull/1895)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Upgrade image used for Go ad Helm operators and scorecard proxy from `registry.access.redhat.com/ubi7/ubi-minimal:latest` to `Use `registry.access.redhat.com/ubi8/ubi-minimal:latest`. [#1895](https://github.com/operator-framework/operator-sdk/pull/1895)
- Upgrade base image for Go, Ansible, Helm, and scorecard proxy from `registry.access.redhat.com/ubi7/ubi-minimal:latest` to `registry.access.redhat.com/ubi8/ubi-minimal:latest`. ([#1895](https://github.com/operator-framework/operator-sdk/pull/1895))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

&& yum install -y epel-release \
&& (yum update || true) \
&& curl -O https://rpmfind.net/linux/fedora/linux/releases/30/Everything/x86_64/os/Packages/i/inotify-tools-3.14-16.fc30.x86_64.rpm \
&& rpm -i inotify-tools-3.14-16.fc30.x86_64.rpm \
Copy link
Member

Choose a reason for hiding this comment

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

Are these lines that download and rpm -i inotify-tools necessary? Looks like we install inotify-tools from epel in line 33.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rpm -i inotify-tools-3.14-16.fc30.x86_64.rpm -> install the pkg in the redhat image ( regystry )
yum --enablerepo=epel install inotify-tools.x86_64 -> install the lib in the cluster

Added a few comments in the code to make it clear.

&& yum --enablerepo=epel install inotify-tools.x86_64 \
&& (yum install python3-setuptools || true) \
&& pip3 install --no-cache-dir --ignore-installed ipaddress \
ansible-runner==1.3.4 \
Copy link
Member

Choose a reason for hiding this comment

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

Again, not too familiar with the ansible-operator internals. Is the ansible-runner version bump required/desired? If so, are there any compatibility concerns elsewhere in the ansible-operator?

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Sep 18, 2019

Choose a reason for hiding this comment

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

Hi @joelanford,

If we check the Changelog I have the impression that these fixes are related to the issue faced before. Also, it should be following the semversion which means that no break changes should be faced with.

@fabianvf I think you are the person who most have knowledge over it, have you any objection? Have any reason for we keep using the old version?

ansible-runner-http==1.0.0 \
openshift==0.8.9 \
ansible==2.8 \
&& yum remove -y gcc \
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to keep python36-devel around or should we remove it like we've been doing?

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 removed it for tests. Let's check if it still working on.

pip install --user molecule==2.20.2
pip install --user docker openshift jmespath
pip3 install --user pyasn1==0.4.7 pyasn1-modules==0.2.6 idna==2.7 ipaddress==1.0.22
pip3 install -U git+https://github.com/ansible/molecule
Copy link
Member

Choose a reason for hiding this comment

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

Can we pin to a version? We've had issues in the past where non-pinned dependencies change can cause our builds to break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, the version is better since it is fixed. Let's see if it will still work if not them we need to wait for a new release of it.

RUN pip install molecule==2.20.1
# sed -i 's|enabled=1|enabled=0|g' /etc/yum/pluginconf.d/product-id.conf && rm -rf /var/yum/cache/* first
RUN sed -i 's|enabled=1|enabled=0|g' /etc/yum/pluginconf.d/product-id.conf && rm -rf /var/cache/yum/* \
&& yum install -y python36-devel.x86_64 gcc libffi-devel
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the arch component of the package (here and in other places its used)? This would prevent us from supporting non-x86_64 builds, which we may want to do in the future. See #1533

Suggested change
&& yum install -y python36-devel.x86_64 gcc libffi-devel
&& yum install -y python36-devel gcc libffi-devel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@joelanford
Copy link
Member

Also @camilamacedo86, the ansible changes seem to be more complex, so would it make sense to update the base images for go, helm, and scorecard proxy in a separate PR so we can get those finished sooner?

@openshift-ci-robot
Copy link

@camilamacedo86: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/images 573e466 link /test images
ci/prow/e2e-aws-helm 573e466 link /test e2e-aws-helm
ci/prow/e2e-aws-ansible 573e466 link /test e2e-aws-ansible
ci/prow/e2e-aws-subcommand 573e466 link /test e2e-aws-subcommand
ci/prow/e2e-aws-go 573e466 link /test e2e-aws-go

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@camilamacedo86
Copy link
Contributor Author

I am closing this one because since we are facing issue regards the python upgrade we will split it.

@camilamacedo86 camilamacedo86 deleted the upgrade-rhel-image branch September 20, 2019 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants