Introduce engine-setup role #1

KKoukiou · 2017-12-08T19:37:46Z

No description provided.

KKoukiou · 2017-12-08T19:39:48Z

Since oVirt.repositories role is dependency the docker tests will not run till this role is available from galaxy.
I verified however with local role path and 4.1 and 4.2 tests are working fine.
Pending to verify 3.6 and 4.0 but I need to use downstream RHEL images for these.
@mwperina @machacekondra WDYT?

KKoukiou · 2017-12-10T18:50:03Z

@didib thanks for review, I ll adjust comments and repost here.

ovirt-infra · 2017-12-10T18:50:06Z

Hello contributor, thanks for submitting a PR for an this project!

I am the bot who triggers "standard-CI" builds for this project.
As a security measure I will not run automated tests on PRs that are not from white-listed contributors.

In order to allow automated tests to run, please ask one of the project maintainers to review the code and then do one of the following:

Type ci test please on this PR to trigger automated tests for it.
Type ci add to whitelist on this PR to trigger automated tests for it and also add you to the contributor white-list so that your future PRs will be tested automatically.
If you are planning to contribute to more than one project, maybe it's better to ask them to add you to the project organization, so you'll be able to run tests for all the organisation's projects.

Note: You might see multiple messages like this showing up on a single PR, this is due to a known bug in the GitHub trigger plugin on Jenkins, the infra team is working on fixing this via new testing flow in the near future, so we'll be able to post a single message per PR.

machacekondra · 2017-12-11T10:46:30Z

ci add to whitelist

machacekondra · 2017-12-11T10:46:38Z

ci build please

mwperina · 2017-12-11T11:02:03Z

README.md

+|---------------------------------|-----------------------|-----------------------------------------------------------|
+| ovirt_engine_setup_version            | UNDEF                 | Allowed versions: [3.6, 4.0, 4.1, 4.2] |
+| ovirt_engine_setup_product_type       | ovirt-engine          | Type of product 'ovirt-engine' or 'rhvm'  | 
+| ovirt_engine_setup_package_list       | [ovirt-engine-dwh-setup,ovirt-engine-extension-aaa-ldap] |  List of extra packages to be installed on engine apart from ovirt-engine package. |


Why do we need ovirt-engine-dwh-setup and ovirt-engine-extension-aaa-ldap to be installed by default? I'd say that default should be provided by existing ovirt-*-setup-* RPM dependencies, so this list should be empty

Didn't read the code nor specs/etc., so not sure if this code should handle updates. If it does, then our docs usually say to:

yum update ovirt*setup*

Which does include ovirt-engine-dwh-setup.

ovirt-engine-setup already requires -dwh-setup, but not a very specific version. So should be updated manually. If user does not, engine-setup will fail, saying 'An update for the Setup packages {packages} was found'.

For clean installs it's enough to install ovirt-engine. That's true also for downstream, BTW, see https://bugzilla.redhat.com/show_bug.cgi?id=1251129

mwperina · 2017-12-11T11:04:55Z

README.md

+
+| Name                            | Default value         |  Description                                              |
+|---------------------------------|-----------------------|-----------------------------------------------------------|
+| ovirt_engine_setup_version            | UNDEF                 | Allowed versions: [3.6, 4.0, 4.1, 4.2] |


When we support all of those version, we should probably document which option below is valid for which version. But it can be done later in subsequent patch

Sounds good.

mwperina · 2017-12-11T11:06:27Z

README.md

+| ovirt_engine_setup_package_list       | [ovirt-engine-dwh-setup,ovirt-engine-extension-aaa-ldap] |  List of extra packages to be installed on engine apart from ovirt-engine package. |
+| ovirt_engine_setup_answer_file_path   | UNDEF                 | Path to custom answerfile for `engine-setup`. |
+| ovirt_engine_setup_db_host            | localhost             | IP address or host name of a PostgreSQL server for Engine database. By default the database will be configured on the same host as Engine. |
+| ovirt_engine_setup_db_port            | 5432                  | Engine database port. |


Why not add a special section Engine database similar to below ISO section?

mwperina · 2017-12-11T11:06:42Z

README.md

+| ovirt_engine_setup_db_name            | engine                | Engine database name. |
+| ovirt_engine_setup_db_user            | engine                | Engine database user. |
+| ovirt_engine_setup_db_password        | UNDEF                 | Engine database password. |
+| ovirt_engine_setup_dwh_db_host        | localhost             | IP address or host name of a PostgreSQL server for DWH database. By default the DWH database will be configured on the same host as Engine. |


Why not add a special section Data Warehouse database similar to below ISO section?

mwperina · 2017-12-11T11:07:08Z

README.md

+By default engine-setup uses answer file specific for version of oVirt,
+based on ``ovirt_engine_setup_version`` parameter. You can specify own answer file
+as ``ovirt_engine_setup_answer_file_path``.
+


Why not add a special section Common options similar to below ISO section?

mwperina · 2017-12-11T11:08:34Z

README.md

+| ovirt_engine_setup_iso_domain_name      | IDO_DOMAIN            | Name of ISO domain.                         |
+| ovirt_engine_setup_iso_domain_acl       | 0.0.0.0/0.0.0.0(rw)   | ACL permissions for ISO domain mount point. |
+
+* Configure firewall


I don't think that firewall with only one option deserves its own section, I'd move to common section

mwperina · 2017-12-11T11:08:53Z

README.md

+
+| Name                  | Default value         |  Description                                              |
+|-----------------------|-----------------------|-----------------------------------------------------------|
+| ovirt_engine_setup_update_packages   | False        | If `True`, setup packages will be updated before `engine-setup` will be executed. Makes sence if Engine is already installed. |


Same here, I'd move to common section

mwperina · 2017-12-11T11:09:55Z

defaults/main.yml

@@ -0,0 +1,33 @@
+---
+ovirt_engine_setup_dwh: True
+ovirt_engine_setup_version: '4.1'


Please use 4.2 as default version

mwperina · 2017-12-11T11:10:50Z

defaults/main.yml

+ovirt_engine_setup_configure_iso_domain: false
+ovirt_engine_setup_iso_domain_path: '/var/lib/exports/iso'
+ovirt_engine_setup_iso_domain_name: 'ISO_DOMAIN'
+ovirt_engine_setup_iso_domain_acl: '0.0.0.0/0.0.0.0(rw)'


ISO is no longer valid for 4.2, please remove those options from default

mwperina · 2017-12-11T11:19:35Z

README.md

+
+| Name                  | Default value         |  Description                                              |
+|-----------------------|-----------------------|-----------------------------------------------------------|
+| ovirt_engine_setup_update_packages   | False        | If `True`, setup packages will be updated before `engine-setup` will be executed. Makes sence if Engine is already installed. |


As mentioned before I'd change default to true, as the most common use cases to execute this role are to perform new installation (updating setup packages should not break anything) or upgrade (updating setup packages is a must for upgrade)

Makes thanks, done.

mwperina · 2017-12-11T11:21:19Z

@sandrobonazzola @didib

KKoukiou · 2017-12-11T15:16:47Z

Missed some comments, will repost again.

KKoukiou · 2017-12-11T16:50:33Z

I have some issue with the upgrade plays, and docker got it. I 'll take a look tomorrow and repost.

KKoukiou · 2017-12-14T20:06:38Z

@mwperina @machacekondra Can you please CR ?
I did some changes.

update all packages by default before running setup, it's specified in the DOCS
Added option to accept defaults. Needed for the upgrade because of open BZ
I created an docker image for the purpose of these tests because I couldn't find any that satisfies the requirements. You can see it here, https://hub.docker.com/r/katerinak/c7-systemd-utf8/
If you have some team account in Docker Hub I 'll be happy to push it there.

machacekondra

In general looks good to me, some minor comments.

machacekondra · 2017-12-15T13:36:55Z

build.sh

+  mkdir -p $PKG_DOC_DIR
+
+  cp -pR defaults/ $PKG_DATA_DIR
+  cp -pR tasks/ $PKG_DATA_DIR


Add here also:

cp -pR templates/ $PKG_DATA_DIR

machacekondra · 2017-12-15T13:40:07Z

meta/main.yml

+
+  license: Apache License 2.0
+
+  min_ansible_version: 2.3


Right, include_tasks for example.

machacekondra · 2017-12-15T13:41:06Z

ovirt-ansible-engine-setup.spec.in

+Requires: ansible >= 2.3
+
+%description
+This Ansible role to install required packages for oVirt Engine deployment,


s/This Ansible role to/This Ansible role

machacekondra · 2017-12-15T13:41:49Z

meta/main.yml

+
+  galaxy_tags: [ovirt, rhv, rhev, virtualization]
+
+dependencies: []


In readme you say dependecies oVirt.repostiories so I am just wondering whether we should depends on it really... I think we shouldn't but I am open to suggestion why we should.

Not really dependent, just this role expect oVirt repositories enabled on the target machine. I 'd rather remove the dependency from README.

Is there a way how to verify if oVirt.repositories are properly installed as a dependency if you install oVirt.engine-setup from Galaxy?

It's installed as a depency in case you specify it in meta/main.yml as dependency, but that's something we don't want. no ?

I think we don't want it, target machine's repos can be preconfigured.

I agree, let's see what Martin thinks.

machacekondra · 2017-12-15T13:42:54Z

ovirt-ansible-engine-setup.spec.in

+BuildArch:      noarch
+Url:            http://www.ovirt.org
+
+Requires: ansible >= 2.3


machacekondra · 2017-12-15T13:43:50Z

tasks/engine_setup.yml

+    when: ovirt_engine_setup_answer_file_path is defined
+
+  - name: Update setup packages
+    shell: 'yum -y update ovirt*setup*'


Is there a reason to not use yum module?

yum module doesn't never run yum update, even if you specify latest. I always runs yum install + params.
When I used the ansible yum module, I was hitting dependency issues, when updating setup packages, and it was complaining about missing dependecies that I could see they are in repository.
I can rerun with this change and check again, however. Maybe there was other issue.

machacekondra · 2017-12-15T13:44:03Z

tasks/engine_setup.yml

+      - skip_ansible_lint
+
+  - name: Update all packages
+    shell: 'yum -y update'


Why not use yum module?

Same ^.
I 'll change and rerun tests I said. Maybe there was non related issue.

machacekondra · 2017-12-15T13:44:48Z

tasks/engine_setup.yml

+    shell: 'yum -y update'
+    when: ovirt_engine_setup_update_all_packages
+    tags:
+      - skip_ansible_lint


What this tag means?

https://github.com/willthames/ansible-lint#false-positives

It's from ansible-lint checking.

Because I think linters will compain I am not using yum module :) Will confirm, I it will be needed to keep this instead of yum module.

Well, if possible we shouldn't use it, and use correct format. And if we need to use it we should document why.

I already removed and I am using yum module. I think you see outdated page.

KKoukiou · 2017-12-18T11:46:33Z

@mwperina @machacekondra Anything else I can do for this one?

mwperina · 2017-12-18T12:42:44Z

README.md

+* OVN related options:
+
+| Name                            | Default value         |  Description                                              |
+|---------------------------------|-----------------------|-----------------------------------------------------------|


How about option to enable/disable OVN configuration during engine-setup?

mwperina · 2017-12-18T12:45:47Z

README.md

+| ovirt_engine_setup_provider_ovn_username | admin@internal     | Username for OVN. |
+| ovirt_engine_setup_provider_ovn_password | UNDEF              | Password for OVN. |
+
+* ISO domain related options: (This following options are deprecated for versions > 3.6)


I think that ISO domain config was removed from engine-setup in 4.2. Am I right Didi? If so, please change the text inside brackets to: "Following options are valid only for version < 4.2

@didib

Looks it's in the docs for 4.1. Fixed.

mwperina · 2017-12-18T12:46:30Z

README.md

+    # Contains encrypted `ovirt_engine_setup_admin_password` variable using ansible-vault
+    - passwords.yml
+  vars:
+    ovirt_engine_setup_version: '4.1'


Please change to 4.2

mwperina · 2017-12-18T12:46:35Z

README.md

+    ovirt_engine_setup_version: '4.1'
+    ovirt_engine_setup_organization: 'of.ovirt.engine.com'
+    ovirt_engine_setup_admin_password: "{{ ovirt_engine_setup_admin_password }}"
+    ovirt_repositories_ovirt_release_rpm: 'http://resources.ovirt.org/pub/yum-repo/ovirt-release41.rpm'


Please change to 4.2 (right now it does not exist, but it should exist before this roles is officially released)

mwperina · 2017-12-18T12:48:23Z

examples/engine-deploy.yml

+    - passwords.yml
+  vars:
+    ovirt_engine_setup_type: "ovirt-engine"
+    ovirt_engine_setup_version: "4.1"


Please change to 4.2

mwperina · 2017-12-18T12:48:58Z

examples/engine-deploy.yml

+    ovirt_engine_setup_dwh_db_host: "localhost"
+    ovirt_engine_setup_configure_iso_domain: true
+    ovirt_engine_setup_firewall_manager: null
+    ovirt_repositories_ovirt_release_rpm: "http://plain.resources.ovirt.org/pub/yum-repo/ovirt-release41.rpm"


Please change to 4.2 (right now it does not exist, but it should exist before this roles is officially released)

mwperina · 2017-12-18T12:55:44Z

meta/main.yml

+
+  galaxy_tags: [ovirt, rhv, rhev, virtualization]
+
+dependencies: []


Is there a way how to verify if oVirt.repositories are properly installed as a dependency if you install oVirt.engine-setup from Galaxy?

mwperina · 2017-12-18T12:56:25Z

templates/answerfile_4.2_basic.txt.j2

@@ -0,0 +1,9 @@
+{% include "./templates/basic_answerfile.txt.j2" %},
+[environment:default]
+OVESETUP_OVN/ovirtProviderOvn=bool:True


This should be provided as role parameter

@mwperina oVrt.repositories is not really a dependency, since if the target machines has the repositories by some other way (user brought them manually) role will work fine.

mwperina · 2017-12-18T12:56:43Z

templates/answerfile_4.2_upgrade.txt.j2

@@ -0,0 +1,11 @@
+{% include "./templates/basic_answerfile.txt.j2" %},
+OVESETUP_OVN/ovirtProviderOvn=bool:True


This should be provided as role parameter

mwperina · 2017-12-18T13:03:10Z

ovirt-ansible-engine-setup.spec.in

+BuildArch:      noarch
+Url:            http://www.ovirt.org
+
+Requires: ansible >= 2.4


We need to add dependency to oVirt.respositories role:

Requires: ovirt-ansible-repositories

Do we really want this package is required ? isn't it better if it's optional ?

Right, let's leave it optional

mwperina

Except minor typo and resolving both RPM and Galaxy dependencies it looks good to me

mwperina · 2017-12-19T06:34:17Z

README.md

+| ovirt_engine_setup_provider_ovn_username | admin@internal     | Username for OVN. |
+| ovirt_engine_setup_provider_ovn_password | UNDEF              | Password for OVN. |
+
+* ISO domain related options: (This following options are valied for versions < 4.2)


valied -> valid

And maybe without "This":
Following options are valid for versions < 4.2

KKoukiou · 2018-01-04T10:45:11Z

@didib This role is used to deploy also engine for 3.6 and there ISO prameters are not deprecated IIUC.

mykaul · 2018-01-04T10:54:16Z

I don't see a reason to support deploying 3.6.

didib · 2018-01-04T10:54:50Z

Re ISO domain - as you wish, but that bug affects also 3.6. I'd personally set it to False and not mention it in README.

didib · 2018-01-04T10:59:56Z

Not that important, but also in RHV 'yum install ovirt-engine' should work just as well, at least since 4.0 - no need to make ovirt_engine_setup_product_type a parameter. See also: https://bugzilla.redhat.com/show_bug.cgi?id=1251129 .

didib · 2018-01-04T11:03:42Z

tasks/engine_setup.yml

+- block:
+  - name: Set answer file path
+    set_fact:
+      answer_file_path: "/tmp/answerfile-{{ lookup('pipe', 'date +%Y%m%d%H%M%SZ') }}.txt"


This is insecure. Please use mktemp or ansible tempfile module. If you require root, you can also use /root/something .

Why is this insecure?

didib · 2018-01-04T11:14:09Z

tasks/engine_setup.yml

+  - name: Run engine-setup with answerfile
+    command: "engine-setup --config-append={{ answer_file_path }} {{ accept_defaults }}"
+    tags:
+      - skip_ansible_lint


Why do you need this? BTW, you can also set acceptdefaults in the answer file, no need to pass it on the command line. See also: https://bugzilla.redhat.com/1270719#8

Nice one. Didn't know, I'll change.

I tried to change to use this answerfile variable but it didn't work.. It failed on the question to auomatically upgrade the postgresql with EOF, which means the default answer was not passed. Moved back to command line option.

didib · 2018-01-04T11:21:12Z

tasks/engine_setup.yml

+
+  - name: Check if Engine health page is up
+    uri:
+      url: "http://{{ ansible_fqdn }}/ovirt-engine/services/health"


The health service is considered deprecated and might be removed in the future, see also: https://bugzilla.redhat.com/show_bug.cgi?id=1027210

If you decide to keep the code as-is, please open an issue to replace this and comment on that bug - so that we know that ovirt-ansible-engine-setup uses it too.

Ok, I'll try to replace and make it point to the api

didib · 2018-01-04T11:25:00Z

templates/answerfile_3.6_basic.txt.j2

@@ -0,0 +1,4 @@
+{% include "./templates/basic_answerfile.txt.j2" %},
+OVESETUP_DB/upgradeWithHeEl6Hosts=none:None


You do not really need these. Generally speaking, almost all vars default to None, so setting them to None in the answerfile almost always does nothing. But no harm in keeping them anyway.

I removed this file.

didib · 2018-01-04T12:42:22Z

README.md

+| ovirt_engine_setup_update_setup_packages | False              | If `True`, setup packages will be updated before `engine-setup` will be executed. Makes sense if Engine is already installed. |
+| ovirt_engine_setup_update_all_packages | True                 | If `True`, all packages will be updated before `engine-setup` will be executed. |
+| ovirt_engine_setup_accept_defaults    | False                 | If `True` use option `--accept-defaults` when executing `engine-setup`. |
+| ovirt_engine_setup_require_rollback   | UNDEF                 | If `True` setup will require to be able to rollback new packages in case of a failure. If not passed the default answer from `engine-setup` will be chosen. Valid for updating/upgrading.


I do not understand this. Isn't the default answer chosen only if using "accept_defaults"? If not, engine-setup should prompt. Do we then let the user answer?

Well, I have in the ansewr fille, OSETUP_RPMDISTRO/requireRollback=none:None in case user doesn't give answer, so default value will be chosen, without the use of accept-defaults.
What we have decided is that all answers should exists in the answer file.
Accept defaults makes sense for example when custom answer file will be used.

didib · 2018-01-04T12:45:00Z