From d0ef425ef6ce19ce79d93bf3a2be1b464750e2f8 Mon Sep 17 00:00:00 2001 From: Chris Hoge Date: Thu, 25 Oct 2018 11:33:25 -0700 Subject: [PATCH] Clean up comment style and identify bugs and workarounds Cleans up the comment style to remove author names and clarify the comment as it relates to the code. Using the NOTE (NAME): format is redundant and takes away attention from the purpose of documenting why an action is being taken. Also updates status of TODO and FIXME items, including removing code was a workaround fixed by a recent patch. Change-Id: I2e087be1e204c618d1dbe499b3f69eae34ce656f --- dockerfiles/debian/Dockerfile | 5 ++--- dockerfiles/ubuntu/Dockerfile | 5 ++--- playbooks/files/apache.conf | 2 +- playbooks/loci-builder.yaml | 11 ----------- playbooks/post.yaml | 6 ++++-- playbooks/setup-gate.yaml | 2 +- playbooks/vars.yaml | 10 +++++++--- scripts/cleanup.sh | 11 +++++------ scripts/fetch_wheels.sh | 5 ++--- scripts/install.sh | 4 ---- scripts/install_nova_console.sh | 14 +++++++------- scripts/requirements.sh | 34 ++++++++++++++++++--------------- scripts/setup_pip.sh | 5 ++--- 13 files changed, 52 insertions(+), 62 deletions(-) diff --git a/dockerfiles/debian/Dockerfile b/dockerfiles/debian/Dockerfile index bd202e6..25af3e3 100644 --- a/dockerfiles/debian/Dockerfile +++ b/dockerfiles/debian/Dockerfile @@ -19,6 +19,5 @@ RUN sed -i \ -e "s|%%DEBIAN_SECURITY_DISTRIBUTION%%|${DEBIAN_SECURITY_DISTRIBUTION}|g" \ -e "s|%%CEPH_URL%%|${CEPH_URL}|g" \ /etc/apt/sources.list - -# NOTE(SamYaple): Remove this when infra starts signing thier mirrors -RUN echo "APT::Get::AllowUnauthenticated \"${ALLOW_UNAUTHENTICATED}\";" > /etc/apt/apt.conf.d/allow-unathenticated +RUN echo "APT::Get::AllowUnauthenticated \"${ALLOW_UNAUTHENTICATED}\";" \ + > /etc/apt/apt.conf.d/allow-unathenticated diff --git a/dockerfiles/ubuntu/Dockerfile b/dockerfiles/ubuntu/Dockerfile index 87f855c..6eff674 100644 --- a/dockerfiles/ubuntu/Dockerfile +++ b/dockerfiles/ubuntu/Dockerfile @@ -17,6 +17,5 @@ RUN sed -i \ -e "s|%%CLOUD_ARCHIVE_URL%%|${CLOUD_ARCHIVE_URL}|g" \ -e "s|%%CEPH_URL%%|${CEPH_URL}|g" \ /etc/apt/sources.list - -# NOTE(SamYaple): Remove this when infra starts signing thier mirrors -RUN echo "APT::Get::AllowUnauthenticated \"${ALLOW_UNAUTHENTICATED}\";" > /etc/apt/apt.conf.d/allow-unathenticated +RUN echo "APT::Get::AllowUnauthenticated \"${ALLOW_UNAUTHENTICATED}\";" \ + > /etc/apt/apt.conf.d/allow-unathenticated diff --git a/playbooks/files/apache.conf b/playbooks/files/apache.conf index 3dc7c36..4a91a46 100644 --- a/playbooks/files/apache.conf +++ b/playbooks/files/apache.conf @@ -10,7 +10,7 @@ LoadModule authn_core_module /usr/lib/apache2/modules/mod_authn_core.so LoadModule authz_core_module /usr/lib/apache2/modules/mod_authz_core.so LoadModule cgi_module /usr/lib/apache2/modules/mod_cgi.so -# NOTE(SamYaple): 172.17.0.1 is the network we use for Docker so it will be in +# 172.17.0.1 is the address we use for Docker so it will be in # the same subnet as the internal addesses in the build containers Listen 172.17.0.1:80 diff --git a/playbooks/loci-builder.yaml b/playbooks/loci-builder.yaml index a57d58c..6218ce0 100644 --- a/playbooks/loci-builder.yaml +++ b/playbooks/loci-builder.yaml @@ -9,17 +9,6 @@ - "pydep.txt" environment: LC_ALL: C - # NOTE(evrardjp): While reuse_requirements is very nice and optimises - # checks and gating, there is a race condition here, because - # we are consuming prebuild wheels (see vars.yaml) by default: - # The jobs in zuul can be building a new "requirements" image, working - # And the new "nova" image would still build on previous "requirements" - # image that was last published. This could cause an issue. Instead - # in gating we should build directly what will be consumed. - # NOTE(SamYaple): This process is so we can take advantage of the infra - # DockerHub mirroring as configured through the Docker daemon. We do this - # instead of calling fetch_wheels initially. All-in-all this saves - # bandwidth and time. - name: Gather wheels to local registry block: - docker_image: diff --git a/playbooks/post.yaml b/playbooks/post.yaml index e002d58..453886a 100644 --- a/playbooks/post.yaml +++ b/playbooks/post.yaml @@ -2,9 +2,11 @@ tasks: - name: Collect logs block: - # NOTE(SamYaple): https://github.com/ansible/ansible/issues/14131 + # FIXME: https://github.com/ansible/ansible/issues/14131 + # This issue closed on October 11, 2018. Patch will be released + # with Ansible 2.8 release. - command: cp -r /home/zuul/.ansible_async /logs/async_logs - # FIXME(SamYaple): running this is causing the gate to hang + # FIXME: running this is causing the gate to hang #- command: journalctl -xb -u docker.service # register: docker_daemon_log # no_log: True diff --git a/playbooks/setup-gate.yaml b/playbooks/setup-gate.yaml index 1d3eb34..a5694f7 100644 --- a/playbooks/setup-gate.yaml +++ b/playbooks/setup-gate.yaml @@ -51,7 +51,7 @@ state: started published_ports: - 172.17.0.1:5000:5000 - # NOTE(SamYaple): Allow all connections from containers to host so the + # Allow all connections from containers to host so the # containers can access the http server for git and wheels - iptables: action: insert diff --git a/playbooks/vars.yaml b/playbooks/vars.yaml index ed04330..fe41938 100644 --- a/playbooks/vars.yaml +++ b/playbooks/vars.yaml @@ -7,11 +7,15 @@ docker_daemon: insecure-registries: - 172.17.0.1:5000 +# Setting reuse_requirements to True will use the most recent +# requirements build from the gate registry. This can save bandwidth +# and time. However, it introduces a gate race condition if a change +# is posted that updates requirments. We set to false to prefer +# correctness to speed. reuse_requirements: False -# NOTE(SamYaple): When running in the loci repo, the project is "loci", but -# when running as a post job for cinder, the project is "cinder". We statically -# declare the path rather than using zuul variables so thats not an issue +# Override Zuul inferrence of source directory from project name to always +# use "loci". loci_src_dir: "src/git.openstack.org/openstack/loci" branch: "{{ zuul_execution_branch.split('/')[-1] }}" diff --git a/scripts/cleanup.sh b/scripts/cleanup.sh index 9542d14..88aeb82 100755 --- a/scripts/cleanup.sh +++ b/scripts/cleanup.sh @@ -12,7 +12,7 @@ case ${distro} in rm -rf /var/lib/apt/lists/* ;; centos) - # NOTE(SamYaple): We should be removing 'patch' here, but that breaks + # We should be removing 'patch' here, but that breaks # centos as it tries to rip out systemd for some reason yum -y autoremove \ git \ @@ -21,7 +21,6 @@ case ${distro} in yum clean all ;; opensuse|opensuse-leap|sles) - # NOTE(evrardjp): Remove all them packages! if [[ "${PYTHON3}" == "no" ]]; then remove_packages=("python-virtualenv") else @@ -39,10 +38,10 @@ case ${distro} in ;; esac -# NOTE(SamYaple): Removing this file allows python to use libraries outside of -# the virtualenv if they do not exist inside the venv. This is a requirement -# for using python-rbd which is not pip installable and is only available in -# packaged form. +# Removing this file allows python to use libraries outside of the +# virtualenv if they do not exist inside the venv. This is a requirement +# for using python-rbd which is not pip installable and is only available +# in packaged form. rm /var/lib/openstack/lib/python*/no-global-site-packages.txt rm -rf /tmp/* /root/.cache /etc/machine-id find /usr/ /var/ \( -name "*.pyc" -o -name "__pycache__" \) -delete diff --git a/scripts/fetch_wheels.sh b/scripts/fetch_wheels.sh index 0e00afe..467670e 100755 --- a/scripts/fetch_wheels.sh +++ b/scripts/fetch_wheels.sh @@ -11,7 +11,6 @@ fi ${python} $(dirname $0)/fetch_wheels.py mkdir -p /tmp/wheels/ -# NOTE(SamYaple): We exclude all files starting with '.' as these can be -# control files for AUFS which have special meaning on AUFS backed file -# stores. +# Exclude all files starting with '.' as these can be control files for +# AUFS which have special meaning on AUFS backed file stores. tar xf /tmp/wheels.tar.gz --exclude='.*' -C /tmp/wheels/ diff --git a/scripts/install.sh b/scripts/install.sh index eee1fc3..8aadb8f 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -50,10 +50,6 @@ case ${distro} in sudo \ tar \ ${rpm_python_packages[@]} - #NOTE(evrardjp) Temporary workaround until bindep is fixed - # for leap 15: https://review.openstack.org/#/c/586038/ - # should be merged and released. - sed -i 's/ID="opensuse-leap"/ID="opensuse"/g' /etc/os-release ;; *) echo "Unknown distro: ${distro}" diff --git a/scripts/install_nova_console.sh b/scripts/install_nova_console.sh index 2ca9e57..ed78b8c 100755 --- a/scripts/install_nova_console.sh +++ b/scripts/install_nova_console.sh @@ -2,17 +2,17 @@ set -ex -# NOTE(SamYaple): Nova console is a special case. The html files needed to make -# this work exist only upstream. The "packaged" versions of these come only -# from openstack specific repos and they have hard requirements to a massive -# amount of packages. Installing from "source" is the only way to get these -# html files into the container. In total this adds less than a MB to the image -# size +# Nova console is a special case. The html files needed to make this work +# exist only upstream. The "packaged" versions of these come only from +# OpenStack specific repos and they have hard requirements to a massive +# amount of packages. Installing from "source" is the only way to get +# these html files into the container. In total this adds less than a MB +# to the image size mkdir /usr/share/novnc git clone -b ${NOVNC_REF} --depth 1 ${NOVNC_REPO} /usr/share/novnc if [[ ! -f /usr/share/novnc/vnc_auto.html ]]; then - # NOTE(SamYaple): novnc >= 1.0.0 is installed + # novnc >= 1.0.0 is installed ln -s vnc_lite.html /usr/share/novnc/vnc_auto.html fi diff --git a/scripts/requirements.sh b/scripts/requirements.sh index 3808ff4..0e2074b 100755 --- a/scripts/requirements.sh +++ b/scripts/requirements.sh @@ -9,8 +9,9 @@ $(dirname $0)/install_packages.sh $(dirname $0)/clone_project.sh mv /tmp/requirements/{global-requirements.txt,upper-constraints.txt} / -# NOTE(SamYaple): https://issues.apache.org/jira/browse/PROTON-1381 -# TODO(SamYaple): Make python-qpid-proton build here (possibly patch it) +# TODO: Make python-qpid-proton build here (possibly patch it) +# or remove when python-qpid-proton is updated with build fix. +# https://issues.apache.org/jira/browse/PROTON-1381 if (( $(openssl version | awk -F'[ .]' '{print $3}') >= 1 )); then sed -i '/python-qpid-proton/d' /upper-constraints.txt fi @@ -23,19 +24,22 @@ fi pushd $(mktemp -d) -# NOTE(SamYaple): Build all deps in parallel. This is safe because we are +# Build all dependencies in parallel. This is safe because we are # constrained on the version and we are building with --no-deps export CASS_DRIVER_BUILD_CONCURRENCY=8 -# NOTE(hrw): Drop python packages requested by monasca_analytics. Their + +# Drop python packages requested by monasca_analytics. Their # build time is huge and on !x86 we do not get binaries from Pypi. egrep -v "(scipy|scikit-learn)" /upper-constraints.txt | split -l1 -# NOTE(aostapenko): When a package uses the variable 'setup_requires' in 'setup.py', 'pip wheel' -# dependency management will be overridden, resulting in possible incompatibilities -# between packages. Installing packages using upper-constraints.txt before building the wheels -# ensures the correct package versions will be available and installed locally. -# https://pip.readthedocs.io/en/stable/user_guide/#installation-bundles -# This allows to work around such issues as https://github.com/lxc/pylxd/issues/308 +# When a package uses the variable 'setup_requires' in 'setup.py', +# 'pip wheel' dependency management will be overridden, resulting in +# possible incompatibilities between packages. Installing packages using +# upper-constraints.txt before building the wheels ensures the correct +# package versions will be available and installed locally. +# https://pip.readthedocs.io/en/stable/user_guide/#installation-bundles +# This allows to work around such issues as +# https://github.com/lxc/pylxd/issues/308 if [ ! -z "${PIP_PACKAGES}" ]; then pip install -c /upper-constraints.txt ${PIP_PACKAGES} fi @@ -43,18 +47,18 @@ fi echo uwsgi enum-compat ${PIP_PACKAGES} | xargs -n1 | split -l1 -a3 ls -1 | xargs -n1 -P20 -t bash -c 'pip wheel --no-deps --wheel-dir / -c /upper-constraints.txt -r $1 || echo %1 >> /failure' _ | tee /tmp/wheels.txt -# TODO(SamYaple): Improve the failure catching +# TODO: Improve the failure catching if [[ -f /failure ]]; then echo Wheel failed to build cat /failure exit 1 fi -# NOTE(SamYaple) Remove native-binary wheels, we only want to keep wheels that -# we compiled ourselves. +# Remove native-binary wheels, we only want to keep wheels that we +# compiled ourselves. awk -F'[ ,]+' '/^Skipping/ {gsub("-","_");print $2}' /tmp/wheels.txt | xargs -r -n1 bash -c 'ls /$1-*' _ | sort -u | xargs -t -r rm -# NOTE(SamYaple): We want to purge all files that are not wheels or txt to -# reduce the size of the layer to only what is needed +# Purge all files that are not wheels or txt to reduce the size of the +# layer to only what is needed shopt -s extglob rm -rf /!(*whl|*txt) > /dev/null 2>&1 || : diff --git a/scripts/setup_pip.sh b/scripts/setup_pip.sh index af8ba7f..a5acf5b 100755 --- a/scripts/setup_pip.sh +++ b/scripts/setup_pip.sh @@ -8,9 +8,8 @@ else TMP_VIRTUALENV="python3 -m virtualenv --python=python3" fi -# NOTE(SamYaple): This little dance allows us to install the latest pip and -# setuptools without get_pip.py or the python-pip package (which is in epel on -# centos) +# This little dance allows us to install the latest pip and setuptools +# without get_pip.py or the python-pip package (in epel on centos) if (( $(${TMP_VIRTUALENV} --version | cut -d. -f1) >= 14 )); then SETUPTOOLS="--no-setuptools" fi