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.

Sign up for GitHub

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

Containerized installs on RHEL were downgrading docker unnecessarily #1718

Merged
merged 8 commits into from
Apr 12, 2016
Merged

Containerized installs on RHEL were downgrading docker unnecessarily #1718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
477f04a
Containerized installs on RHEL were downgrading docker unnecessarily
brenton Apr 6, 2016
f443be0
Handle cases where the pacemaker variables aren't set
brenton Apr 7, 2016
463737f
Installs and upgrades from authenticated registries are not supported…
brenton Apr 8, 2016
ad9aad9
use RestartSec to avoid default rate limit in systemd
brenton Apr 11, 2016
ff06c2e
Make sure Docker is restarted after we have correctly configured the …
brenton Apr 11, 2016
09d6e9f
Fetching the current version a little more carefully
brenton Apr 11, 2016
5c7b510
Fixing non-HA master restart conditional
brenton Apr 11, 2016
0879620
Bug 1324728 - Ansible should not downgrade docker when installing 3.2…
brenton Apr 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions filter_plugins/oo_filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ def oo_get_hosts_from_hostvars(hostvars, hosts):
return retval

@staticmethod
def oo_image_tag_to_rpm_version(version):
def oo_image_tag_to_rpm_version(version, include_dash=False):
""" Convert an image tag string to an RPM version if necessary
Empty strings and strings that are already in rpm version format
are ignored.
Expand All @@ -802,7 +802,10 @@ def oo_image_tag_to_rpm_version(version):
raise errors.AnsibleFilterError("|failed expects a string or unicode")

if version.startswith("v"):
version = "-" + version.replace("v", "")
version = version.replace("v", "")

if include_dash:
version = "-" + version

return version

Expand Down
10 changes: 5 additions & 5 deletions playbooks/common/openshift-cluster/upgrades/files/openshift_container_versions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,19 @@
# Here we don't really care if this is a master, api, controller or node image.
# We just need to know the version of one of them.
unit_file=$(ls /etc/systemd/system/${1}*.service | head -n1)
installed_container_name=$(basename -s .service ${unit_file})
installed=$(docker exec ${installed_container_name} openshift version 2> /dev/null | grep openshift | awk '{ print $2 }' | cut -f1 -d"-" | tr -d 'v')

if [ ${1} == "origin" ]; then
image_name="openshift/origin"
elif grep aep $unit_file 2>&1 > /dev/null; then
image_name="aep3/aep"
image_name="aep3/node"
elif grep openshift3 $unit_file 2>&1 > /dev/null; then
image_name="openshift3/ose"
image_name="openshift3/node"
fi

installed=$(docker run --rm --entrypoint=/bin/openshift ${image_name} version 2> /dev/null | grep openshift | awk '{ print $2 }' | cut -f1 -d"-" | tr -d 'v')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@detiber, are you OK with this? It's true there's an edge case of a running service being a different version from the node image that's already cached, however, if they haven't run the installer yet and restarted their system they would be running that version anyway. My goal here is to do something failsafe that will never result in setting the version to something that won't even start. Also, this way the upgrade playbook can safely be re-run.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brenton yeah, I think that's fine... it's funny how I always thought the containerized installs would be "better" and "easier" :)


docker pull ${image_name} 2>&1 > /dev/null
available=$(docker run --rm ${image_name} version 2> /dev/null | grep openshift | awk '{ print $2 }' | cut -f1 -d"-" | tr -d 'v')
available=$(docker run --rm --entrypoint=/bin/openshift ${image_name} version 2> /dev/null | grep openshift | awk '{ print $2 }' | cut -f1 -d"-" | tr -d 'v')

echo "---"
echo "curr_version: ${installed}"
Expand Down
22 changes: 15 additions & 7 deletions playbooks/common/openshift-cluster/upgrades/v3_1_to_v3_2/pre.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
msg: >
This upgrade does not support Pacemaker:
https://docs.openshift.{{ g_pacemaker_upgrade_url_segment }}/install_config/upgrading/pacemaker_to_native_ha.html
when: openshift.master.cluster_method == 'pacemaker'
when: openshift.master.cluster_method is defined and openshift.master.cluster_method == 'pacemaker'

- fail:
msg: >
Expand All @@ -57,6 +57,11 @@
roles:
- openshift_facts
tasks:
- openshift_facts:
role: master
local_facts:
ha: "{{ groups.oo_masters_to_config | length > 1 }}"

- name: Ensure Master is running
service:
name: "{{ openshift.common.service_type }}-master"
Expand All @@ -77,11 +82,6 @@
state: started
enabled: yes
when: openshift.master.ha is defined and openshift.master.ha | bool and openshift.common.is_containerized | bool
post_tasks:
- openshift_facts:
role: master
local_facts:
ha: "{{ groups.oo_masters_to_config | length > 1 }}"

- name: Verify upgrade can proceed
hosts: oo_nodes_to_config
Expand All @@ -105,8 +105,12 @@
- include: ../../../../../roles/openshift_master/handlers/main.yml
- include: ../../../../../roles/openshift_node/handlers/main.yml
roles:
# We want the cli role to evaluate so that the containerized oc/oadm wrappers
# are modified to use the correct image tag. However, this can trigger a
# docker restart if new configuration is laid down which would immediately
# pull the latest image and defeat the purpose of these tasks.
- openshift_cli
tasks:
pre_tasks:
- name: Clean package cache
command: "{{ ansible_pkg_mgr }} clean all"
when: not openshift.common.is_atomic | bool
Expand Down Expand Up @@ -145,6 +149,10 @@
g_new_version: "{{ openshift_image_tag | replace('v','') }}"
when: openshift_image_tag is defined

- fail:
msg: Verifying the correct version was found
when: g_aos_versions.curr_version == ""

- fail:
msg: Verifying the correct version was found
when: verify_upgrade_version is defined and g_new_version != verify_upgrade_version
Expand Down
43 changes: 14 additions & 29 deletions playbooks/common/openshift-cluster/upgrades/v3_1_to_v3_2/upgrade.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,4 @@
---
# This is a workaround for authenticated registries
- name: Download new images
hosts: oo_nodes_to_config
roles:
- openshift_facts
tasks:
- name: Pull Images
command: >
docker pull {{ item }}:v{{ g_new_version }}
with_items:
- "{{ openshift.node.node_image }}"
- "{{ openshift.node.ovs_image }}"
- "{{ openshift.common.pod_image }}"
- "{{ openshift.common.router_image }}"
- "{{ openshift.common.registry_image }}"
- "{{ openshift.common.deployer_image }}"

# This is a workaround for authenticated registries
- name: Download new images
hosts: oo_masters_to_config
roles:
- openshift_facts
tasks:
- name: Pull Images
command: >
docker pull {{ item }}:v{{ g_new_version }}
with_items:
- "{{ openshift.master.master_image }}"

###############################################################################
# The restart playbook should be run after this playbook completes.
###############################################################################
Expand All @@ -40,6 +11,20 @@
- include: docker_upgrade.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

docker_upgrade.yml file missing from PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That file already existed. I just had to move it earlier due to bugs in docker 1.8.2's --add-registry. Without docker 1.9 the workaround for authenticated registries doesn't even work.

when: not openshift.common.is_atomic | bool

# The cli image is used by openshift_facts to determine the currently installed
# version. We need to explicitly pull the latest image to handle cases where
# the locally cached 'latest' tag is older the g_new_version.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abutcher, PTAL. This was why Docker was downgraded when upgrading to 3.2.

- name: Download cli image
hosts: oo_masters_to_config:oo_nodes_to_config
roles:
- openshift_facts
tasks:
- name: Pull Images
command: >
docker pull {{ item }}:latest
with_items:
- "{{ openshift.common.cli_image }}"

###############################################################################
# Upgrade Masters
###############################################################################
Expand Down
23 changes: 14 additions & 9 deletions roles/docker/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,27 @@
register: docker_version_result
changed_when: false

- stat: path=/etc/sysconfig/docker-storage
register: docker_storage_check

- name: Remove deferred deletion for downgrades from 1.9
command: >
sed -i 's/--storage-opt dm.use_deferred_deletion=true//' /etc/sysconfig/docker-storage
when: docker_storage_check.stat.exists | bool and not docker_version_result | skipped and docker_version_result.stdout | default('0.0', True) | version_compare('1.9', '>=') and docker_version | version_compare('1.9', '<')

- name: Downgrade docker if necessary
command: "{{ ansible_pkg_mgr }} downgrade -y docker-{{ docker_version }}"
register: docker_downgrade_result
when: not docker_version_result | skipped and docker_version_result.stdout | default('0.0', True) | version_compare(docker_version, 'gt')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sdodson, I think not using .stdout was just a bug previously.

Copy link
Member

Choose a reason for hiding this comment

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

👍


- name: Install docker
action: "{{ ansible_pkg_mgr }} name=docker{{ '-' + docker_version if docker_version != '' else '' }} state=present"
when: not openshift.common.is_atomic | bool and not docker_downgrade_result | changed
action: "{{ ansible_pkg_mgr }} name=docker{{ '-' + docker_version if docker_version is defined and docker_version != '' else '' }} state=present"
when: not openshift.common.is_atomic | bool and not docker_version_result | skipped and docker_version_result.stdout | default('0.0', True) | version_compare(docker_version, 'lt')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@detiber, @sdodson, this is what I'm testing. It's weird but it seems to handle the 1) no docker installed 2) downgrade and 3) upgrade cases. The idea is that if docker_version_result.stdout is empty we need to install the minimum required version. If there is a version installed and we didn't need to downgrade then we can ensure the latest version.

I think there's still an edgecase if a version like 1.8.1 was installed and we needed 1.8.2. I think it would actually result in 1.9.1 being installed. This is a mess.

Copy link
Member

Choose a reason for hiding this comment

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

Did it not work without changing from if docker_version != '' to docker_version_result.stdout == '' As you have it we'd always need to set docker_version, which would be the case if we changed from > 3.2/1.2 to >= 3.2/1.2 in the facts.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, I see you're accounting for that in the when condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually surprised there isn't an error for line 16 and not having a else clause... I suppose that is just a jinja2'ism.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I really think we should avoid latest to avoid upgrading docker inadvertently during regular CM operations, since we should only have to worry about initial installation, and 1.2/3.2 packages would have a requirement on docker-1.9, that should only leave us with issues related to containerized installs and docker versions.

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'm fine with reworking this to avoid using 'latest'. I think one of the problems I was hitting was coming up with some simple logic without reworking what we already had too much. I think if we don't have 'latest' the way the logic is now it could result in docker-1.9.1 being explicitly installed even though a 1.9.2 package was available. That isn't the end of the world but it's a spell of overly complicated logic.

Copy link
Member

Choose a reason for hiding this comment

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

I've posted this[0] question regarding how we should express requirements of the host os. If we're not worried about containerized installs this is a non issue because installing 3.2 atomic-openshift-node package will ensure that we're at a minimum version so it's really the upper version limits for 3.1 and 3.0 that we require.

0 - http://ask.projectatomic.io/en/question/3641/expressing-requirements-of-the-host-os/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@detiber, @abutcher do you have any idea how this line could result in this error?

One or more undefined variables: the inline if-expression on line 1 evaluated to false and no else section was defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This showed that all the variables were actually defined as expected:

- debug: var=openshift
- debug: var=docker_version_result
- debug: var=docker_version

Copy link
Contributor

Choose a reason for hiding this comment

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

@brenton maybe stdout is not in docker_version_result?

Copy link
Member

Choose a reason for hiding this comment

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

Line above it is missing an else

{{ '-' + docker_version if docker_version_result.stdout == '' else '' }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the logs I have it is:

2016-04-08 16:51:21,001 p=924 u=root |  ok: [ip-172-18-0-28.ec2.internal] => {
    "var": {
        "docker_version_result": {
            "changed": false,
            "cmd": [
                "repoquery",
                "--installed",
                "--qf",
                "%{version}",
                "docker"
            ],
** SNIP**
            "start": "2016-04-08 16:51:19.957724",
            "stderr": "",
            "stdout": "1.8.2",
            "stdout_lines": [
                "1.8.2"
            ],
            "warnings": []
        }
    }
}
2016-04-08 16:51:21,003 p=924 u=root |  ok: [ip-172-18-15-36.ec2.internal] => {
    "var": {
        "docker_version_result": {
            "changed": false,
**SNIP**
            "rc": 0,
            "start": "2016-04-08 16:51:20.019622",
            "stderr": "",
            "stdout": "1.8.2",
            "stdout_lines": [
                "1.8.2"
            ],
            "warnings": []
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abutcher, yeah, that was 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.

@detiber, this is pretty hard to read but here's the thinking:

If docker_version_result.stdout == '' that means docker was never installed. Therefore we have to rely on the docker_version and result in something like yum install docker-1.8.2. If there is stdout that means it's installed. We've already previously downgraded to the correct version in the previous step. So really we just have to make sure the package is installed to skip that test. I'll be honest, I would have expected the 'when' clause to prevent that logic from ever running.

Copy link
Member

Choose a reason for hiding this comment

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

There's gaps in this that have been identified since I did the backport but should we try to be less clever for the sake of clarity? I liked how easy this was to follow in my backport

openshift-ansible/roles/docker/tasks/main.yml

Lines 10 to 65 in 7e39163

# Avoid docker 1.9 when installing origin < 1.2 or OSE < 3.2 on RHEL/Centos and
# See: https://bugzilla.redhat.com/show_bug.cgi?id=1304038
- name: Default to latest docker
set_fact:
docker_version: ''
docker_downgrade: False
- name: Gather installed version of common package
shell: >
rpm -q {{ openshift.common.service_type }} --queryformat '%{version}' | cut -d '.' -f1,2 | grep -v 'not installed'
register: installed_common_version
failed_when: false
changed_when: false
- name: Gather available version of common package
shell: >
yum list available -e 0 -q "{{ openshift.common.service_type}}" 2>&1 | tail -n +2 | awk '{ print $2 }' | sort -r | tr '\n' ' ' | tail -n 1 | cut -d '.' -f1,2
register: available_common_version
failed_when: false
changed_when: false
- name: Gather installed version of docker
shell: >
rpm -q docker --queryformat '%{version}' | cut -d '.' -f1,2 | grep -v 'not installed'
register: installed_docker_version
failed_when: false
changed_when: false
- name: Gather available version of docker
shell: >
yum list available -e 0 -q "docker" 2>&1 | tail -n +2 | awk '{ print $2 }' | sort -r | tr '\n' ' ' | tail -n 1
register: available_docker_version
failed_when: false
changed_when: false
- set_fact:
docker_version: "{{ '1.8.2' }}"
when: " installed_common_version.stdout in ['3.0','3.1','1.0','1.1'] or available_common_version.stdout in ['3.0','3.1','1.0','1.1']"
- set_fact:
docker_downgrade: True
when: " docker_version < installed_docker_version.stdout and docker_version != ''"
- name: Remove deferred removal and deletion for 1.8.2 downgrades
command: >
sed -i 's/--storage-opt dm.use_deferred_deletion=true//' /etc/sysconfig/docker-storage
when: docker_downgrade | bool and docker_version == '1.8.2'
- name: Downgrade docker
command: >
{{ ansible_pkg_mgr }} downgrade -y docker{{ '-' + docker_version if docker_version != '' else '' }}
when: not openshift.common.is_atomic and docker_downgrade | bool
- name: Install docker
action: "{{ ansible_pkg_mgr }} name=docker{{ '-' + docker_version if docker_version != '' else '' }} state=present"
when: not openshift.common.is_atomic | bool and not docker_downgrade | bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's getting late on a Friday. I'm honestly struggling to see how either of these are easy to follow given all the logic branches for containerized installs.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, I didn't test with Docker not installed, but with the current logic it failed for the upgrade scenario


- stat: path=/etc/sysconfig/docker-storage
register: docker_storage_check

- name: Remove deferred deletion for downgrades from 1.9
command: >
sed -i 's/--storage-opt dm.use_deferred_deletion=true//' /etc/sysconfig/docker-storage
when: docker_downgrade_result | changed and docker_storage_check.stat.exists | bool and docker_version_result.stdout | default('0.0', True) | version_compare('1.9', '>=') and docker_version | version_compare('1.9', '<')
# We're getting ready to start docker. This is a workaround for cases where it
# seems a package install/upgrade/downgrade has rebooted docker and crashed it.
- name: Reset docker service state
command: systemctl reset-failed docker.service

- name: enable and start the docker service
service:
Expand Down
1 change: 1 addition & 0 deletions roles/etcd/templates/etcd.docker.service
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ ExecStart=/usr/bin/docker run --name {{ etcd_service }} --rm -v /var/lib/etcd:/v
ExecStop=/usr/bin/docker stop {{ etcd_service }}
SyslogIdentifier=etcd_container
Restart=always
RestartSec=5s

[Install]
WantedBy=docker.service
2 changes: 1 addition & 1 deletion roles/openshift_common/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
# versions or image tags. openshift_common's usage requires that it be a RPM
# version and openshift_cli expects it to be an image tag.
- name: Install the base package for versioning
action: "{{ ansible_pkg_mgr }} name={{ openshift.common.service_type }}{{ openshift_version | default('') | oo_image_tag_to_rpm_version }} state=present"
action: "{{ ansible_pkg_mgr }} name={{ openshift.common.service_type }}{{ openshift_version | default('') | oo_image_tag_to_rpm_version(include_dash=True) }} state=present"
when: not openshift.common.is_containerized | bool

- name: Set version facts
Expand Down
20 changes: 17 additions & 3 deletions roles/openshift_docker_facts/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,24 @@
register: common_version
failed_when: false
changed_when: false
when: not openshift.common.is_atomic | bool
when: not openshift.common.is_containerized | bool

- set_fact:
l_common_version: "{{ openshift.common.image_tag | default('0.0', True) | oo_image_tag_to_rpm_version }}"
when: openshift.common.is_containerized | bool

- set_fact:
l_common_version: "{{ common_version.stdout | default('0.0', True) }}"
when: not openshift.common.is_containerized | bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably have an irrational hate of set_fact... I can't think of a way to put this into a vars file though, with the use of common_version.stdout though...

it could be limited down to one set_fact if you do the following though:

l_common_version: "{{ ( openshift.common.image_tag | oo_image_tag_to_rpm_version if openshift.common.is_containerized | bool else common_version.stdout) | default('0.0', True) }}"

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'd do whatever you prefer. I sort of hate inlining if/else like this. It almost always gets more complicated and I can no longer count how many bugs I've seen because of followup edits that subtly break things.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, if it can't be eliminated by pushing it to vars, it's not really worth it anyway.


- name: Set docker version to be installed
set_fact:
docker_version: "{{ '1.8.2' }}"
when: " ( common_version.stdout | default('0.0', True) | version_compare('3.2','<') and openshift.common.service_type == 'atomic-openshift' ) or
( common_version.stdout | default('0.0', True) | version_compare('1.1.4','<') and openshift.common.service_type == 'origin' )"
when: " ( l_common_version | version_compare('3.2','<') and openshift.common.service_type == 'atomic-openshift' ) or
( l_common_version | version_compare('1.1.4','<') and openshift.common.service_type == 'origin' )"

- name: Set docker version to be installed
set_fact:
docker_version: "{{ '1.9.1' }}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@detiber, @sdodson, I'm not really excited about having to explicitly declare compatible version numbers. I suspect our supported versions of docker for each OSE release are only going to get more complicated over time. I definitely want the ansible install code to be a simple as possible and least likely to fail. I wanted to set docker_version to 1.9 however that would complicate the gt and lt checks. On OSE 3.1 if you are running 1.9.1 you need to downgrade. However, if you are running OSE 3.2 you don't need to downgrade or upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brenton I'm wondering if we shouldn't worry about upper bounds on docker version unless we have a known issue like we have with 3.1. Then during upgrades we could handle upgrading docker if necessary.

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'd prefer to wait to handle the upper bound issue when we see what the docker 1.9 vs 1.10 packaging looks like. There are still a lot of possibilities and I don't think we would guess correctly at this point.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if ensure=latest does the right thing if you're specifying version? We could just not specify the version for 3.2 until such a point that we know of an upper bound. Then the install task would ensure it's the latest.

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'll test that out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Every use of the docker role should be through the openshift_docker role which is pulling data from openshift_facts.

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 was mostly just thinking out loud that the conditional is going to be a little tricky and require a lot of testing. I think my perception that this role was being applied multiple times was that I had multiple hosts in my environment that are all master+node.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brenton indeed, for a HA master host, it will be applied 3 times (etcd, master, and node)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to add a comment that we should avoid using latest for rpm installation to avoid issues when using these playbooks/roles for ongoing management.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why I didn't think about this earlier, but would it make sense to push the complex version logic in openshift_docker_facts into the openshift_facts module directly, then we wouldn't need to run multiple tasks to set the docker_version that is passed to the docker role.

Moving the package version tests into the openshift_facts module would also probably prevent any issues around installing a newer version of docker only to downgrade later as well.

We'd still have some funkiness in the docker role proper to handle downgrade, but I think it'd still be worth it to keep the docker role generic.

when: " ( l_common_version | version_compare('3.2','>') and openshift.common.service_type == 'atomic-openshift' ) or
( l_common_version | version_compare('1.2','>') and openshift.common.service_type == 'origin' )"
19 changes: 11 additions & 8 deletions roles/openshift_facts/library/openshift_facts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,20 +1080,23 @@ def get_openshift_version(facts, cli_image=None):
elif 'node' in facts:
container = facts['common']['service_type'] + '-node'

if container is not None:
exit_code, output, _ = module.run_command(['docker', 'exec', container, 'openshift', 'version'])
# if for some reason the container is installed but not running
# we'll fall back to using docker run later in this method.
if exit_code == 0:
version = parse_openshift_version(output)

# Try to get the version fromthe available cli image _before_ resorting
# to exec'ing in to the running container. This is to be more fault
# tolerant in environments where the container is not running.
if version is None and cli_image is not None:
# Assume we haven't installed the environment yet and we need
# to query the latest image, but only if docker is installed
if 'docker' in facts and 'version' in facts['docker']:
exit_code, output, _ = module.run_command(['docker', 'run', '--rm', cli_image, 'version'])
version = parse_openshift_version(output)

if version is None and container is not None:
exit_code, output, _ = module.run_command(['docker', 'exec', container, 'openshift', 'version'])
# if for some reason the container is installed but not running
# we'll fall back to using docker run later in this method.
if exit_code == 0:
version = parse_openshift_version(output)

return version

def parse_openshift_version(output):
Expand Down Expand Up @@ -1351,7 +1354,7 @@ def set_container_facts_if_unset(facts):
facts['common']['admin_binary'] = '/usr/local/bin/oadm'
facts['common']['client_binary'] = '/usr/local/bin/oc'
openshift_version = get_openshift_version(facts, cli_image)
if openshift_version is not None:
if openshift_version is not None and openshift_version is not "":
base_version = openshift_version.split('-')[0]
facts['common']['image_tag'] = "v" + base_version

Expand Down
6 changes: 3 additions & 3 deletions roles/openshift_master/handlers/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
---
- name: restart master
service: name={{ openshift.common.service_type }}-master state=restarted
when: (openshift.master.ha is defined and not openshift.master.ha | bool) and (not (master_service_status_changed | default(false) | bool))
when: (openshift.master.ha is not defined or not openshift.master.ha | bool) and (not (master_service_status_changed | default(false) | bool))
notify: Verify API Server

- name: restart master api
service: name={{ openshift.common.service_type }}-master-api state=restarted
when: (openshift.master.ha is defined and openshift_master_ha | bool) and (not (master_api_service_status_changed | default(false) | bool)) and openshift.master.cluster_method == 'native'
when: (openshift.master.ha is defined and openshift.master.ha | bool) and (not (master_api_service_status_changed | default(false) | bool)) and openshift.master.cluster_method == 'native'
notify: Verify API Server

- name: restart master controllers
service: name={{ openshift.common.service_type }}-master-controllers state=restarted
when: (openshift.master.ha is defined and openshift_master_ha | bool) and (not (master_controllers_service_status_changed | default(false) | bool)) and openshift.master.cluster_method == 'native'
when: (openshift.master.ha is defined and openshift.master.ha | bool) and (not (master_controllers_service_status_changed | default(false) | bool)) and openshift.master.cluster_method == 'native'

- name: Verify API Server
# Using curl here since the uri module requires python-httplib2 and
Expand Down
1 change: 1 addition & 0 deletions roles/openshift_master/templates/docker-cluster/atomic-openshift-master-api.service.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ LimitCORE=infinity
WorkingDirectory={{ openshift.common.data_dir }}
SyslogIdentifier={{ openshift.common.service_type }}-master-api
Restart=always
RestartSec=5s

[Install]
WantedBy=docker.service
Expand Down
1 change: 1 addition & 0 deletions .../openshift_master/templates/docker-cluster/atomic-openshift-master-controllers.service.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ LimitCORE=infinity
WorkingDirectory={{ openshift.common.data_dir }}
SyslogIdentifier={{ openshift.common.service_type }}-master-controllers
Restart=on-failure
RestartSec=5s

[Install]
WantedBy=docker.service
1 change: 1 addition & 0 deletions roles/openshift_master/templates/docker/master.docker.service.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ ExecStart=/usr/bin/docker run --rm --privileged --net=host --name {{ openshift.c
ExecStartPost=/usr/bin/sleep 10
ExecStop=/usr/bin/docker stop {{ openshift.common.service_type }}-master
Restart=always
RestartSec=5s

[Install]
WantedBy=docker.service
1 change: 1 addition & 0 deletions roles/openshift_master/templates/native-cluster/atomic-openshift-master-api.service.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ LimitNOFILE=131072
LimitCORE=infinity
WorkingDirectory={{ openshift.common.data_dir }}
SyslogIdentifier=atomic-openshift-master-api
RestartSec=5s

[Install]
WantedBy=multi-user.target
Expand Down
1 change: 1 addition & 0 deletions .../openshift_master/templates/native-cluster/atomic-openshift-master-controllers.service.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ LimitCORE=infinity
WorkingDirectory={{ openshift.common.data_dir }}
SyslogIdentifier={{ openshift.common.service_type }}-master-controllers
Restart=on-failure
RestartSec=5s

[Install]
WantedBy=multi-user.target
1 change: 1 addition & 0 deletions roles/openshift_node/templates/openshift.docker.node.service
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ ExecStartPost=/usr/bin/sleep 10
ExecStop=/usr/bin/docker stop {{ openshift.common.service_type }}-node
SyslogIdentifier={{ openshift.common.service_type }}-node
Restart=always
RestartSec=5s

[Install]
WantedBy=docker.service
1 change: 1 addition & 0 deletions roles/openshift_node/templates/openvswitch.docker.service
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ ExecStartPost=/usr/bin/sleep 5
ExecStop=/usr/bin/docker stop openvswitch
SyslogIdentifier=openvswitch
Restart=always
RestartSec=5s

[Install]
WantedBy=docker.service