Merge inventory-generator with origin-ansible image #5043

juanvallejo · 2017-08-09T14:06:44Z

Begin adding https://github.com/juanvallejo/dynamic-inventory as a new sub-pkg under images/.
the inventory-generator is based on this proposal

Trello card for the creation of this image: https://trello.com/c/314Df06O
Trello card for integrating this image with openshift-ansible: https://trello.com/c/ARPorrvv

Goal

The goal behind having this new image is to:

Create a script that runs in a pod in a cluster.
Script collects information about cluster.
Script uses collected information to create inventory file for that cluster.
Our health checks playbook (inside the pod in that same cluster) uses that inventory file to run our checks against the cluster.

Building

From the root of the openshift/ansible directory

$ docker build --rm -t openshift/inventory-gen images/inventory-generator

Implementation

The reason why (currently) both master-config.yaml and admin.kubeconfig are needed is because we use master-config.yaml to extract most of the information about hosts that are part of the current deployment, and then use admin.kubeconfig to establish a connection with the oc binary, to retrieve any additional information about any remaining nodes through the output of $ oc get nodes -o yaml.

I am hoping to use this PR and its feedback to mature the contents of this script.

cc @rhcarvalho @brenton @sosiouxme

rhcarvalho · 2017-08-10T08:34:06Z

@juanvallejo you have some YAML-lint errors: https://travis-ci.org/openshift/openshift-ansible/jobs/262695660#L669

rhcarvalho · 2017-08-10T08:36:37Z

images/inventory-generator/Dockerfile

+
+RUN INSTALL_PKGS="openssh-clients wget git" \
+    && yum install -y --setopt=tsflags=nodocs $INSTALL_PKGS \
+    && EPEL_PKGS="PyYAML ansible python2-boto" \


Why don't we enable EPEL first and then install all packages at once?

EPEL packages sometimes shadow official packages, which we would usually prefer. There are two basic ways to prevent this - this is one - the other is to define the EPEL repo but leave it inactive and only enable it for the yum transactions needed. Either way you end up separating out the EPEL dependencies.

The other reason to do this is because it identifies which packages are "gaps" in the official packages. In the downstream these tend to require some different treatment, either shipping packages in another channel, getting them via RHSCL, or making them unnecessary somehow. And if things change and the packages are no longer needed... it becomes obvious you can get rid of EPEL.

rhcarvalho · 2017-08-10T08:47:48Z

images/inventory-generator/Dockerfile

+ENV APP_ROOT=/opt/app-root \
+	HOME=/opt/app-root/src \
+	WORK_DIR=/opt/app-root \
+	USER_UID=1001


Bad indentation / alignment.

rhcarvalho · 2017-08-10T08:50:39Z

images/inventory-generator/root/usr/local/bin/user_setup

+chown ${USER_UID}:0 ${HOME}
+chmod ug+rwx -R ${HOME}
+
+# runtime user will need to be able to self-insert in /etc/passwd


Why does the user needs to edit /etc/passwd?

Ah, will word that comment a bit better. This comment justifies the need to make /etc/passwd writable, in order to tie a username to the given uid. This is required for ssh to work from within the container

rhcarvalho · 2017-08-10T08:51:45Z

images/inventory-generator/root/usr/local/bin/user_setup

+chmod g+rw /opt/app-root/src
+
+# ensure that the ansible content is accessible
+chmod -R g+r ${WORK_DIR} /bin


Why do we change the permissions of /bin too?

rhcarvalho · 2017-08-10T09:03:22Z

images/inventory-generator/root/usr/local/bin/user_setup

+find ${WORK_DIR} -type d -exec chmod g+x {} +
+
+# This will redirect us to the latest stable release.
+redirect=$(curl -k https://github.com/openshift/origin/releases/latest)


Why do we use of -k / --insecure?

Agreed, not needed

rhcarvalho · 2017-08-10T12:07:34Z

images/inventory-generator/root/usr/local/bin/run

+/usr/local/bin/generate ${OUTPUT_INVENTORY}
+
+if [[ -z "${PLAYBOOK}" ]]; then
+  echo


rhcarvalho · 2017-08-10T12:08:40Z

images/inventory-generator/root/usr/local/bin/run

+fi
+
+# run ansible playbook using generated inventory
+ansible-playbook -vvv -i ${OUTPUT_INVENTORY} ${PLAYBOOK}  --become --become-user root


Double space before --become; missing new line.

Why do we default to -vvv (and hard code it)?

Why do we set --become-user root? It is the default.

Why do we need --become?

rhcarvalho · 2017-08-10T12:18:50Z

images/inventory-generator/root/usr/local/bin/generate

@@ -0,0 +1,348 @@
+#!/bin/env python


Note: the "traditional" path/shebang is #!/usr/bin/env python, though this should also work on Fedora/RHEL/CentOS.

rhcarvalho · 2017-08-10T12:23:09Z

images/inventory-generator/root/usr/local/bin/entrypoint

@@ -0,0 +1,17 @@
+#!/bin/bash -e


Suggestion: set shell options in the script, not in the shebang -- if the script is executed like /path/to/shell path/to/script, the -e flag is suddenly gone.

Also, I recommend:

set -o errexit set -o nounset set -o pipefail

This is what we do most often (see shell scripts in Origin), each option for a good reason.

rhcarvalho · 2017-08-10T12:23:23Z

images/inventory-generator/root/usr/local/bin/entrypoint

+# This file serves as the main entrypoint to the inventory-generator image.
+#
+# For more information see the documentation:
+#	https://github.com/juanvallejo/dynamic-inventory/blob/master/README.md


Update URL?

rhcarvalho · 2017-08-10T12:31:55Z

I did a first pass, but did not review everything.

Is it worth having yet another image, what about simply adding a new script to the existing openshift-ansible image?

juanvallejo · 2017-08-10T15:33:53Z

@rhcarvalho

Is it worth having yet another image, what about simply adding a new script to the existing openshift-ansible image?

Was not sure to what extent we wanted to couple this with our existing image. But if it makes more sense to have this functionality as part of it, I can go ahead and update the PR.

cc @sosiouxme @brenton

sosiouxme · 2017-08-14T15:22:06Z

One image sounds good to me.

juanvallejo · 2017-08-15T16:31:42Z

@rhcarvalho @sosiouxme still testing, but went ahead and merged inventory-generator with our image. Its readme file has been updated (README_INVENTORY_GENERATOR.md).

sosiouxme

So I know this isn't supposed to be full-featured, but it needs a fair amount more before it will even handle my regular AWS test clusters passably...

(Sorry for the tardiness of my input here)

sosiouxme · 2017-08-15T19:34:39Z

images/installer/root/usr/local/bin/generate

+        openshift_hosted_logging_deploy = True
+
+    m = Host("masters")
+    m.host_alias(common_host_alias)


What if there is no alias? My hosts don't have aliases.

sosiouxme · 2017-08-15T19:34:59Z

images/installer/root/usr/local/bin/generate

+                continue
+
+            n = Host("nodes")
+            n.host_alias(common_host_alias)


Likewise; aliases should be optional and I don't think you're going to want to use the same one for all the nodes.

sosiouxme · 2017-08-15T19:37:44Z

images/installer/root/usr/local/bin/generate

+        exit(1)
+
+    # set inventory values based on user configuration
+    common_host_alias = user_config.get('common_host_alias', 'openshiftdevel')


all hosts default to the same alias? that seems unlikely to work if there is more than one host.

sosiouxme · 2017-08-15T19:42:07Z

images/installer/root/usr/local/bin/generate

+    m.host_alias(common_host_alias)
+    m.address(master_config["masterIP"])
+    m.public_host_name(master_public_url)
+    host_groups["masters"] = HostGroup([m])


What if there are multiple masters?

sosiouxme · 2017-08-15T20:05:52Z

images/installer/root/usr/local/bin/generate

+
+    # connect to remote host using `oc login...` and extract all possible node information
+    oc = OpenShiftClient()
+    oc.login(master_public_url, openshift_cluster_user, openshift_cluster_pass)


I'd really like to be able to just use an existing kubeconfig to connect.

This only handles the use case of a user with a login and password. It's can be a pain to set one of those up and it's likely they want it to be a service account with a revocable auth token. Plus it's possible the master_public_url isn't what they want to use, nor insecure TLS.

Simplest would probably be to use a kubeconfig. It's easy for the user to create one with oc login. We don't need to recreate oc login in python.

sosiouxme · 2017-08-15T20:48:26Z

images/installer/root/usr/local/bin/generate

+    try:
+        config_file_obj = open(USER_CONFIG, 'r')
+        raw_config_file = config_file_obj.read()
+        user_config = yaml.load(raw_config_file)


if the file is empty then this returns None and all the access below fails. Should check that the result is a dict or just make it an empty one.

sosiouxme · 2017-08-15T20:54:46Z

images/installer/README_INVENTORY_GENERATOR.md

+       -v /tmp/aws/master-config.yaml:/opt/app-root/src/master-config.yaml:Z \
+       -e OPTS="-v --become --become-user root" \
+       -e PLAYBOOK_FILE=playbooks/byo/config.yml \
+       -e GENERATE_INVENTORY=true \


need to mention where the inventory gets generated, and they need to specify where Ansible should look for it right?

sosiouxme · 2017-08-15T20:55:21Z

images/installer/root/usr/local/bin/generate

+
+INVY = 'generated_hosts'
+if len(sys.argv) > 1:
+    INVY = sys.argv[1]


should be able to specify full path, not just the name.

Also would like to be able to print to stdout.

sosiouxme · 2017-08-15T21:04:14Z

images/installer/root/usr/local/bin/generate

+
+    # open new inventory file for writing
+    try:
+        inv_file_obj = open(HOME + '/' + INVY, 'w+')


should be configurable

sosiouxme · 2017-08-15T21:06:50Z

images/installer/root/usr/local/bin/generate

+    inv_file_obj.write("ansible_become={}\n".format(ansible_become))
+    inv_file_obj.write("openshift_uninstall_images={}\n".format(str(openshift_uninstall_images)))
+    inv_file_obj.write("openshift_deployment_type={}\n".format(openshift_deployment_type))
+    inv_file_obj.write("openshift_install_examples={}\n".format(str(openshift_install_examples)))


What about all the other stuff from my config file?

openshift_release: 3.6 openshift_image_tag: v3.6.0 openshift_hosted_logging_deploy: True openshift_logging_image_version: v3.6.0 openshift_disable_check: disk_availability,docker_storage,memory_availability

etc.... i think we want to be able to generate as close to the original file as possible.

juanvallejo · 2017-08-16T20:35:59Z

@sosiouxme thanks for the feedback; review comments addressed.
Will only support one master in this iteration. In a future update it would
be nice to add support to multiple masters (and host aliases) through the
user config file.

juanvallejo · 2017-08-21T13:51:28Z

[test]

rhcarvalho · 2017-08-21T14:04:41Z

images/installer/root/usr/local/bin/user_setup

 # runtime user will need to be able to self-insert in /etc/passwd
 chmod g+rw /etc/passwd

+# make required config dirs writable
+chown ${USER_UID}:0 ${HOME}/.kube/config
+chmod a+rw ${HOME}/.kube/config


I suspect this file is bind-mounted, and thus this would have an impact on permissions outside of the container...

rhcarvalho · 2017-08-21T14:08:07Z

images/installer/README_INVENTORY_GENERATOR.md

+  - specifies where to look for the bind-mounted `master-config.yaml` file in the container
+  - if omitted or a `null` value is given, its value is defaulted to `/opt/app-root/src/master-config.yaml`
+
+- `admin_kubeconfig_path` (required):


How come it is both required and has a default value?!

rhcarvalho · 2017-08-21T14:08:08Z

images/installer/README_INVENTORY_GENERATOR.md

+  - username of account capable of listing nodes in a cluster
+  - used for querying the cluster using `oc` to retrieve additional node information.
+
+- `master_config_path` (required):


How come it is both required and has a default value?!

rhcarvalho · 2017-08-21T14:08:09Z

images/installer/root/usr/local/bin/user_setup

 # runtime user will need to be able to self-insert in /etc/passwd
 chmod g+rw /etc/passwd

+# make required config dirs writable
+chown ${USER_UID}:0 ${HOME}/.kube/config


These lines ignore admin_kubeconfig_path, and I think it will only work if admin_kubeconfig_path has its default value?

rhcarvalho · 2017-08-21T14:18:57Z

images/installer/README_INVENTORY_GENERATOR.md

+
+### Build
+
+`docker build --rm -t openshift/origin-ansible -f images/installer/Dockerfile .`


I suspect this must be run from the repository root (to give the correct meaning for .) and thus we need to note that?

Use triple-backticks for making it a block. Single backtick is used for inline code snippets.

``` docker build --rm -t openshift/origin-ansible -f images/installer/Dockerfile . ```

Hmm, on a second thought, the build process is already described elsewhere, the inventory generation does not need to talk about building images?!

rhcarvalho · 2017-08-21T14:25:45Z

images/installer/root/usr/local/bin/generate

+DEFAULT_MASTER_CONFIG_PATH = HOME + '/master-config.yaml'
+DEFAULT_ADMIN_KUBECONFIG_PATH = HOME + '/.kube/config'
+
+INVY_FULL_PATH = HOME + '/generated_hosts'


INVY? Sounds funny :-)

rhcarvalho · 2017-08-21T14:55:03Z

images/installer/inventory-generator-config.yaml

@@ -0,0 +1 @@
+../installer/root/etc/inventory-generator-config.yaml


Why the symlink?

rhcarvalho · 2017-08-21T14:56:45Z

images/installer/root/etc/inventory-generator-config.yaml

+openshift_image_tag: v3.6.0
+openshift_hosted_logging_deploy: null  # defaults to "true" if loggingPublicURL is set in master-config.yaml
+openshift_logging_image_version: v3.6.0
+openshift_disable_check: disk_availability,docker_storage,memory_availability


Won't this suggest people to disable these checks unintentionally?

Hm, so I had this for debugging purposes and had meant to take them out :) will update

rhcarvalho · 2017-08-21T15:03:34Z

images/installer/root/etc/inventory-generator-config.yaml

+---
+# meta config
+master_config_path: null  # defaults to /opt/app-root/src/master-config.yaml
+admin_kubeconfig_path: null  # defaults to /opt/app-root/src/.kube/config


How helpful is it to include these?

Feels like the default paths are repeated in many places.

rhcarvalho · 2017-08-21T15:03:39Z

images/installer/root/etc/inventory-generator-config.yaml

+
+openshift_release: 3.6
+openshift_image_tag: v3.6.0
+openshift_hosted_logging_deploy: null  # defaults to "true" if loggingPublicURL is set in master-config.yaml


Is it helpful to include this?

I'm thinking with this and admin_kubeconfig_path above, I could at least default them to their default values instead of null. I think having these present in the default config file gives a better glance of what can be tweaked

juanvallejo · 2017-08-31T21:03:00Z

@sosiouxme thanks for the feedback, review comments addressed. Let's see how ci tests fare this time

rhcarvalho · 2017-09-01T12:51:57Z

@juanvallejo please rebase and if there are no other changes we can start aos-ci-test and then tag for merge ;)

juanvallejo · 2017-09-05T13:27:40Z

@rhcarvalho thanks, done!

juanvallejo · 2017-09-06T14:11:00Z

re[test]

juanvallejo · 2017-09-13T13:57:35Z

flaked on openshift/origin#8571 re[test]

sosiouxme · 2017-09-13T16:54:55Z

aos-ci-test

openshift-bot · 2017-09-13T18:48:14Z

Evaluated for openshift ansible test up to 19ea800

sosiouxme · 2017-09-13T18:49:07Z

I found a few things that still needed addressing, so I did that... works for me now.

openshift-bot · 2017-09-13T20:40:07Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/659/) (Base Commit: 665c5c2) (PR Branch Commit: 19ea800)

openshift-bot · 2017-09-13T23:31:24Z

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for c032e4c (logs)

openshift-bot · 2017-09-13T23:32:52Z

success: "aos-ci-jenkins/OS_3.6_containerized, aos-ci-jenkins/OS_3.6_containerized_e2e_tests" for c032e4c (logs)

sosiouxme · 2017-09-14T02:25:38Z

... why are the test statuses still yellow?

sdodson · 2017-09-14T02:50:53Z

Looks like they were run against an old commit, the commit is recorded at the time the aos-ci-test job is queued rather than at the start of the job. So it ran against c032e4c which has since been updated?

sdodson · 2017-09-14T02:50:57Z

aos-ci-test

sdodson · 2017-09-14T02:52:47Z

If that isn't complete tomorrow i'm fine to merge this manually based on the results of https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/659/

sdodson · 2017-09-14T13:24:28Z

sigh I just noticed no one had added a merge tag, let me know if this should be reverted.

sosiouxme · 2017-09-14T13:31:55Z

@sdodson only reason there was no merge tag was because waiting for aos-ci-test. This is quite low risk so I wouldn't worry about reverting.

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch 6 times, most recently from c3c7e45 to cae4164 Compare August 9, 2017 14:43

juanvallejo requested review from sosiouxme, abhgupta and rhcarvalho and removed request for abhgupta August 9, 2017 18:19

rhcarvalho reviewed Aug 10, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch from cae4164 to 5cee9db Compare August 10, 2017 13:55

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch from 00345ba to 009e570 Compare August 15, 2017 16:31

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch 2 times, most recently from 45c37d0 to 70e2725 Compare August 15, 2017 17:13

juanvallejo changed the title ~~[WIP] add inventory-generator under new sub pkg~~ [WIP] merge inventory-generator with origin-ansible image Aug 15, 2017

sosiouxme requested changes Aug 15, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch from 11f10f3 to 81a7439 Compare August 16, 2017 20:34

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch 3 times, most recently from 40ac162 to a821120 Compare August 18, 2017 14:00

rhcarvalho reviewed Aug 21, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch from a821120 to b769288 Compare August 21, 2017 21:23

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch from b411e30 to daf81fc Compare August 31, 2017 21:02

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch from daf81fc to a705552 Compare September 5, 2017 13:27

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 5, 2017

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch from a705552 to 032858a Compare September 5, 2017 21:01

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch from 323c429 to 31dd1b4 Compare September 11, 2017 21:45

add inventory-generator under new sub pkg

c032e4c

juanvallejo force-pushed the jvallejo/add-inventory-generator-image branch from 31dd1b4 to c032e4c Compare September 12, 2017 13:41

sosiouxme added 3 commits September 13, 2017 14:46

inventory generate: always use kubeconfig, no login

0ee022a

inventory generate: remove refs to openshift_cluster_user

a638d23

inventory generate: fix config doc

19ea800

sosiouxme approved these changes Sep 13, 2017

View reviewed changes

sdodson merged commit 7df0b64 into openshift:master Sep 14, 2017

juanvallejo deleted the jvallejo/add-inventory-generator-image branch September 14, 2017 18:52


		### Build

		`docker build --rm -t openshift/origin-ansible -f images/installer/Dockerfile .`

		@@ -0,0 +1 @@
		../installer/root/etc/inventory-generator-config.yaml

Merge inventory-generator with origin-ansible image #5043

Merge inventory-generator with origin-ansible image #5043

Conversation

juanvallejo commented Aug 9, 2017 • edited Loading

Goal

Building

Implementation

rhcarvalho commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhcarvalho commented Aug 10, 2017

juanvallejo commented Aug 10, 2017

sosiouxme commented Aug 14, 2017

juanvallejo commented Aug 15, 2017

sosiouxme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Aug 16, 2017

juanvallejo commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Aug 31, 2017

rhcarvalho commented Sep 1, 2017

juanvallejo commented Sep 5, 2017

juanvallejo commented Sep 6, 2017

juanvallejo commented Sep 13, 2017

sosiouxme commented Sep 13, 2017

openshift-bot commented Sep 13, 2017

sosiouxme commented Sep 13, 2017

openshift-bot commented Sep 13, 2017

openshift-bot commented Sep 13, 2017

openshift-bot commented Sep 13, 2017

sosiouxme commented Sep 14, 2017

sdodson commented Sep 14, 2017

sdodson commented Sep 14, 2017

sdodson commented Sep 14, 2017

sdodson commented Sep 14, 2017

sosiouxme commented Sep 14, 2017

juanvallejo commented Aug 9, 2017 •

edited

Loading