-
Notifications
You must be signed in to change notification settings - Fork 381
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.
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
[Kogito-2195] Kogito examples with ansible automation #258
base: main
Are you sure you want to change the base?
Conversation
@danielezonca , @krisv , @ricardozanini , @radtriste , who should we add as dev reviewers and QE reviewers (if applicable) for this to get it worked in and approved for the examples? Max and I apparently don't have permissions to add reviewers officially. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@desmax74
Hello! Thank you for your PR. I've put the first review here. Unfortunately I was not able to test it because the setup is for Ubuntu/Debian. Can you please add setup also for some RedHat like systems such as Fedora?
kogito-ansible/README.md
Outdated
Ansible scripts to automate creation of CRC cluster and the deploy of one of the Kogito examples with just one command line. | ||
|
||
CRC 1.10.0, and Ansible must be installed. | ||
No previous CRC setup (no /home/{user}/.crc folder), otherwise the create script will fail, delete .crc if you want run more than once the create playbook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this optional. User may want to use his already CRC deployed cluster. It is probably not good to force user to remove all his previous CRC setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
kogito-ansible/README.md
Outdated
### Install CRC | ||
(If you haven't already installed) | ||
|
||
Pre requisite on Debian/Ubuntu: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide version of Ubuntu and Debian on which you have tested these scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
kogito-ansible/README.md
Outdated
```sh | ||
sudo ansible-playbook ./playbook_etc_hosts.yaml | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide setup also for Fedora.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create two sections in the doc since is all for fedora and only the lib virt installation is the first step on ubuntu
kogito-ansible/playbook_libs.yaml
Outdated
tasks: | ||
|
||
- name: install | ||
shell: "sudo apt install qemu-kvm libvirt-daemon libvirt-daemon-system network-manager -y" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use "apt" module instead of shell. https://docs.ansible.com/ansible/latest/modules/apt_module.html#apt-module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
kogito-ansible/playbook_libs.yaml
Outdated
- name: install | ||
shell: "sudo apt install qemu-kvm libvirt-daemon libvirt-daemon-system network-manager -y" | ||
become: yes | ||
when: ansible_os_family == 'Debian' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check that os family is supported using assert module instead of silently skipping installation of libvirt libraries.
https://docs.ansible.com/ansible/latest/modules/assert_module.html#assert-module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
kogito-ansible/playbook_create.yaml
Outdated
- name: Copy Kogito cli | ||
get_url: | ||
url: "https://github.com/kiegroup/kogito-cloud-operator/releases/download/{{ kogito_tag }}/kogito-cli-{{ kogito_tag }}-linux-amd64.tar.gz" | ||
dest: "/tmp/kogito-{{ kogito_tag }}-linux-amd64.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again please create variable for this temporary destination with default value. Or you can also consider to use "tempfile" module. https://docs.ansible.com/ansible/latest/modules/tempfile_module.html#tempfile-module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
|
||
tasks: | ||
|
||
- name: Check api in hosts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't DNS reconfigured by CRC itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to check with the latest version, because with old version of crc without this manual addition on /etc/hosts the cluster was unreachable
kogito-ansible/playbook_create.yaml
Outdated
kogito_tag: "0.10.0" | ||
ram: "16384" #MB | ||
libvirt_version: "4.4.3" | ||
ip_on_etc_hosts: 192.168.130.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use this value which can probably change on different machine etc. It would be much better to use "virt" or "virt_net" modules to obtain info about IP address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
kogito-ansible/playbook_create.yaml
Outdated
--- | ||
- name: Start CRC 1.10.0 and deploy Kogito Operator and CLI | ||
hosts: localhost | ||
gather_facts: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to explicitly run this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, was just to describe the version used, we could use a placeholder and print the version used instead to updated this on every update
kogito-ansible/playbook_crc.yaml
Outdated
--- | ||
- name: Install CRC | ||
hosts: localhost | ||
gather_facts: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this done by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, is disabled by default to save time, but to read the current user infos is needed
@sterobin Hello I did the first QE review for this. |
@jiripetrlik These scripts are developed and tested on Fedora 31, only the playbook_scripts is for ubuntu if is the system used. |
@desmax74 It seems to me that "playbook_libs.yaml" is developed for Ubuntu isn't it? And as libvirts setup is necessary for whole setup we probably need alternative for Fedora. Do you agree? |
@jiripetrlik yep, only playbook_libs.yaml is for ubuntu, if isn't clear the description "Pre requisite on Debian/Ubuntu: |
@desmax74 See documentation for Fedora virtualization packages: https://docs.fedoraproject.org/en-US/quick-docs/getting-started-with-virtualization/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR :)
Some comments
kogito-ansible/playbook_create.yaml
Outdated
vars: | ||
kogito_tag: "0.10.0" | ||
ram: "16384" #MB | ||
libvirt_version: "4.4.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this value hardcoded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because is used in the path and you discover the version only when the crc is downloaded and unpacked, or readed from the lates json info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've writed a version to use the latest kogito tag, but the 0.10.1 doesn't work because the cli and the operator aren't aligned (kogito 0.10.1 - kogito-cloud-operator 0.10.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm I expect this could happen in the future too so is it possible to add a kogito-operator
variable to support also this use case?
kogito-ansible/README.md
Outdated
|
||
|
||
|
||
## Ubuntu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this section actually different from the Fedora
one?
They are really similar (the same?) from what I can see. Is it possible to create a single Fedora/Ubuntu
section and just highlight different steps (if any).
This can reduce maintenance cost of this doc :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously only one section plus the ubuntu lib virt installation was present but at the reviewer seems a document for ubuntu deploy and asked a fedora version, for this reason you see explicit configurations for both systems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree to have the instructions for Ubuntu
and Fedora
too but if they are the same just mention that the instructions are tested on both configuration instead of duplicate them :)
kogito-ansible/playbook_libs.yaml
Outdated
when: ansible_os_family == 'Debian' | ||
|
||
- name: Install Virtualization on Fedora | ||
shell: "sudo dnf install @virtualization -y" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Ansible DNF module instead https://docs.ansible.com/ansible/latest/modules/dnf_module.html#dnf-module
Something like this:
- name: Install Virtualization on Fedora
dnf:
name: "@Virtualization"
become: yes
when: ansible_os_family == 'RedHat'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@desmax74
|
|
@desmax74 |
In the Ubuntu 20.04 bashrc isn't present the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving 👍
@desmax74 can you please rebase from master to make the build green?
…older removed in the 20.04
@jiripetrlik dropped ubuntu until we found a workaround, please review |
@desmax74 Hello currently I'm getting following error on Fedora 32 : ""changed": true, "cmd": "crc setup", "delta": "0:03:00.977265", "end": "2020-06-16 10:31:41.176060", "msg": "non-zero return code", "rc": 1, "start": "2020-06-16 10:28:40.198795", "stderr": "level=fatal msg="Failed to start libvirt 'crc' network"", "stderr_lines"... |
@desmax74 It also seems that there are some commits unrelated to PR and PR check failed. Can you please rebase properly? |
@krisv @mdproctor @ricardozanini do you have any information about this? |
I'd close it. |
https://issues.redhat.com/browse/KOGITO-2195
@sterobin @danielezonca