Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions filter_plugins/oo_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,13 @@ def oo_split(string, separator=','):
return string.split(separator)


def oo_list_to_dict(lst, separator='='):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not do this. Let's have the user give us a dictionary and create a new variable. Since this isn't actually used by docker, no need to re-use docker's variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is used by docker. it is used by daemon.json is the template for /etc/docker/container-daemon.json which is used by docker when running in a container. So in this case log-opts is exactly the same as openshift_docker_log_options

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that sounds good then. I think in the future we should do the reverse, we've talked about the current inflexibility of the docker options as impemented. I think it would be better as a list of kv pairs, and we convert a deprecated list k=v.

""" This converts a list of ["k=v"] to a dictionary {k: v}.
"""
kvs = [i.split(separator) for i in lst]
return {k: v for k, v in kvs}


def oo_haproxy_backend_masters(hosts, port):
""" This takes an array of dicts and returns an array of dicts
to be used as a backend for the haproxy role
Expand Down Expand Up @@ -969,6 +976,7 @@ def filters(self):
"oo_combine_dict": oo_combine_dict,
"oo_dict_to_list_of_dict": oo_dict_to_list_of_dict,
"oo_split": oo_split,
"oo_list_to_dict": oo_list_to_dict,
"oo_filter_list": oo_filter_list,
"oo_parse_heat_stack_outputs": oo_parse_heat_stack_outputs,
"oo_parse_named_certificates": oo_parse_named_certificates,
Expand Down
4 changes: 4 additions & 0 deletions roles/container_runtime/defaults/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ docker_default_storage_path: /var/lib/docker
# Set local versions of facts that must be in json format for container-daemon.json
# NOTE: When jinja2.9+ is used the container-daemon.json file can move to using tojson
l_docker_log_options: "{{ l2_docker_log_options | to_json }}"
l_docker_log_options_dict: "{{ l2_docker_log_options | oo_list_to_dict | to_json }}"
l_docker_additional_registries: "{{ l2_docker_additional_registries | to_json }}"
l_docker_blocked_registries: "{{ l2_docker_blocked_registries | to_json }}"
l_docker_insecure_registries: "{{ l2_docker_insecure_registries | to_json }}"
Expand All @@ -81,6 +82,7 @@ l_insecure_crio_registries: "{{ '\"{}\"'.format('\", \"'.join(l2_docker_insecure
l_crio_registries: "{{ l2_docker_additional_registries + ['docker.io'] }}"
l_additional_crio_registries: "{{ '\"{}\"'.format('\", \"'.join(l_crio_registries)) }}"


openshift_crio_image_tag_default: "latest"

l_crt_crio_image_tag_dict:
Expand Down Expand Up @@ -127,3 +129,5 @@ l_docker_image_tag: "{{ l_crt_docker_image_tag_dict[openshift_deployment_type] }

l_docker_image_default: "{{ l_docker_image_prepend }}/{{ openshift_docker_service_name }}:{{ l_docker_image_tag }}"
l_docker_image: "{{ openshift_docker_systemcontainer_image_override | default(l_docker_image_default) }}"

l_is_node_system_container: "{{ (openshift_use_node_system_container | default(openshift_use_system_containers | default(false)) | bool) }}"
2 changes: 1 addition & 1 deletion roles/container_runtime/tasks/systemcontainer_crio.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
fail: msg='Cannot use CRI-O with node configured as a Docker container'
when:
- openshift.common.is_containerized | bool
- not openshift.common.is_node_system_container | bool
- not l_is_node_system_container | bool

- include_tasks: common/pre.yml

Expand Down
4 changes: 2 additions & 2 deletions roles/container_runtime/templates/daemon.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
"disable-legacy-registry": false,
"exec-opts": ["native.cgroupdriver=systemd"],
"insecure-registries": {{ l_docker_insecure_registries }},
{% if openshift_docker_log_driver is defined %}
{% if openshift_docker_log_driver %}
"log-driver": "{{ openshift_docker_log_driver }}",
{%- endif %}
"log-opts": {{ l_docker_log_options }},
"log-opts": {{ l_docker_log_options_dict }},
"runtimes": {
"oci": {
"path": "/usr/libexec/docker/docker-runc-current"
Expand Down
9 changes: 9 additions & 0 deletions roles/openshift_node/tasks/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,15 @@
- name: include node installer
include_tasks: install.yml

- name: Restart cri-o
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be restarted here? There aren't any changes being made to crio in this role as far as I can tell.

Copy link
Member

Choose a reason for hiding this comment

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

This was required when the sdn was installed just prior to this section. Since sdn-ovs installation was moved to install.yml the restart is likely still required.

Ref: #6362 (review)

systemd:
name: cri-o
enabled: yes
state: restarted
when: openshift_use_crio
register: task_result
failed_when: task_result|failed and 'could not find the requested service' not in task_result.msg|lower

- name: restart NetworkManager to ensure resolv.conf is present
systemd:
name: NetworkManager
Expand Down
2 changes: 2 additions & 0 deletions roles/openshift_version/defaults/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ openshift_service_type_dict:
openshift-enterprise: atomic-openshift

openshift_service_type: "{{ openshift_service_type_dict[openshift_deployment_type] }}"

openshift_use_crio_only: False
2 changes: 2 additions & 0 deletions roles/openshift_version/meta/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,5 @@ galaxy_info:
- cloud
dependencies:
- role: lib_utils
- role: container_runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

Please checkout my other patch to see the general direction that we're moving.

Container runtime will be setup as a prerequisite before openshift is deployed. It cannot be a meta-dependency of any other role.

- role: openshift_facts
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this is necessary here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried without openshift_facts and it was still failing

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 happy to openshift_facts here if you like, that's not really going to change anything in this role, and it's essentially a 0 task import.