-
Notifications
You must be signed in to change notification settings - Fork 4
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
Merge qe_common role #123
base: master
Are you sure you want to change the base?
Merge qe_common role #123
Conversation
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 fixed the linter errors already, but there are still a few more changes that should be made
- name: Get Endpoint | ||
ansible.builtin.shell: | ||
cmd: | | ||
oc project openstack |
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.
oc rsh openstackclient ...
does the same as kubectl exec.
You don't need to do --
to separate the command
You can pass the -n flag to tell it which namespace to run in
roles/qe_common/tasks/cred_tests.yml
Outdated
cmd: | | ||
oc get crd "{{ item }}" | ||
register: output | ||
failed_when: output.rc != 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.
This failure condition is implicit for the shell module, so it's usually omitted.
roles/qe_common/tasks/main.yml
Outdated
@@ -0,0 +1,64 @@ | |||
--- | |||
- when: container_list is defined | |||
name: "Verify container - {{ container_polar_id}}" |
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 change the name of this var to container_test_id
, rather than container_polar_id
, since it's more intuitive, rather than needing to know that polar is short for Polarian and what Polarian is.
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.
Also, please add in defaults for these values in roles/qe_common/defaults/main.yml.
This is so that the test ID is optional, since you might want to use this for some pre-checks, without having a test_id.
roles/qe_common/tasks/main.yml
Outdated
loop: "{{ proj_list }}" | ||
|
||
|
||
- when: |
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.
- when: | |
- when: |
.ansible-lint
Outdated
@@ -2,6 +2,8 @@ | |||
exclude_paths: | |||
- ci/ | |||
- roles/telemetry_autoscaling | |||
- roles/telemetry_logging | |||
- roles/qe_common |
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 try not to skip linting this role.
I ran ansible-lint locally, and found some errors that prevent the roles from running
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.
ansible-lint is really useful for catching small, hard to see errors in syntax or layout, as well as being able to provide some additional feedback that keeps the content easier to grok and maintain
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.
FYI: If you un-skip the "command-instead-of-module" check in .ansible-lint, it gives you suggestions for what modules could be used instead of shell
for certain commands
.ansible-lint
Outdated
@@ -2,6 +2,8 @@ | |||
exclude_paths: | |||
- ci/ | |||
- roles/telemetry_autoscaling | |||
- roles/telemetry_logging | |||
- roles/qe_common |
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.
- roles/qe_common |
roles/qe_common/tasks/main.yml
Outdated
@@ -0,0 +1,64 @@ | |||
--- | |||
- when: container_list is defined | |||
name: "Verify container - {{ container_polar_id}}" |
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.
https://ansible.readthedocs.io/projects/lint/rules/jinja/
jinja[spacing]: Jinja2 spacing could be improved: Verify container - {{ container_polar_id}} -> Verify container - {{ container_polar_id }} (warning)
roles/qe_common/tasks/main.yml
Outdated
- when: node_list is defined | ||
name: "Verify OSP node - {{ node_polar_id }}" | ||
ansible.builtin.include_tasks: node_tests.yml | ||
loop: "{{ node_list }}" |
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.
key-order[task]: You can improve the task key order to: name, when, ansible.builtin.include_tasks, loop
roles/qe_common/tasks/main.yml:32 Task/Handler: Verify OSP node - {{ node_polar_id }}
https://ansible.readthedocs.io/projects/lint/rules/key-order/
(Same for the other tasks in this file
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/838cb4e2e4644f569ca1f922fbe31a82 ❌ openstack-k8s-operators-content-provider FAILURE in 6m 06s |
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.
It looks okay for now. This role is being called anywhere yet, so let's merge it, and we can resolve issues that come up later one this is hooked up to testing
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.
Its seems that it doesn't have any obvious syntax error to me, its okay to merged we are not triggering it yet.
Thanks Alex it contains a variety of common tasks that can be used in different other tests.
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/2fed2e1b296143c698fc94427414a526 ✔️ feature-verification-tests-noop SUCCESS in 5s |
This failure is unrelated to this change. |
|
||
For subscription_tests.yml tasks: | ||
|
||
subscription_polar_id |
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.
These var names need to be updated to match the updated var names (*_test_id)
Example Playbook | ||
---------------- | ||
|
||
Typically, for this role the tests should *not* use a "main.yml" and import or include all the tests in the role. On the contrary, a tests should explicitly include specific tests needed for a given job. |
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.
This can be removed.
The behaviour has changed to work by setting appropriate vars to select the tests that run. The vars are set and then used by the role on import.
Ideally, the vars would have the <role_name>_ prefix to them
hosts: controller | ||
gather_facts: no | ||
vars: | ||
proj_out_file: verify_logging_projects_exist_lresults.log |
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.
The project_out_file is not mentioned elsewhere.
oc project openstack | ||
kubectl exec openstackclient -- openstack endpoint list --service="{{ item[0] }}" --service="{{ item[1] }}" --interface="{{ item[2] }}" |
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.
You can use oc rsh openstackclient openstack endpoint list ....
the -n openstack
should be included to specify the project/namespace
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.
What happens if the endpoint doesn't exist? Does the openstackclient return a non-zero return code?
- name: Get container status | ||
ansible.builtin.shell: | ||
cmd: | | ||
podman ps -a --format "{{ '{{.Names}} {{.Status}}' }}" | grep "{{ item }}" | awk '{print $2;}' |
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.
For readability, I recommend using something instead of item.
You can use a useful variable name, and tell the loop in main to use that same var name for its loop var.
The syntax would be similar to: https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_loops.html#stacking-loops-via-include-tasks, but without the nested loops.
I would suggest container_name as the loop var here, so that reading this task is easier.
Here's an example playbook:
---
- hosts: localhost
tasks:
- ansible.builtin.debug:
msg: "{{ item }}"
loop:
- "first"
- "second"
- ansible.builtin.debug:
msg: "{{ my_var }}"
loop:
- "third"
- "fourth"
loop_control:
loop_var: my_var
New readme file for role
new playbook
duplicate file
added service test file
added for easy of use
fixed typo
removed redundant results file generation
remove redundant results file generation
remove redundant results file generation.
remove redundant results file generation.
remove redundant results file generation.
remove redundant results file generation.
remove redundant results file generation
remove redundant results file generation
remove redundant results file generation
remove redundant results file generation
updated test_id var name
updated role name
moved role to new dir
initial file
qe_common role name change to common
test id var change
remove qe_common role dir
roles/qe_common name change to roles/common
qe_common role rename to common
remove unneeded line
remove unneeded line
remove unneeded line
remove unneeded line
remove unneeded line
lint changes
7f6cb87
to
6e3283c
Compare
- roles/telemetry_logging | ||
- roles/commmon |
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.
- roles/telemetry_logging | |
- roles/commmon |
hosts: controller | ||
gather_facts: no | ||
vars: | ||
proj_out_file: verify_logging_projects_exist_lresults.log |
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.
proj_out_file: verify_logging_projects_exist_lresults.log |
|
||
For subscription_tests.yml tasks: | ||
|
||
subscription_polar_id |
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.
subscription_polar_id | |
subscription_test_id |
Example Playbook | ||
---------------- | ||
|
||
Typically, for this role the tests should *not* use a "main.yml" and import or include all the tests in the role. On the contrary, a tests should explicitly include specific tests needed for a given job. |
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.
Typically, for this role the tests should *not* use a "main.yml" and import or include all the tests in the role. On the contrary, a tests should explicitly include specific tests needed for a given job. |
roles/telemetry_logging/README.md
Outdated
@@ -0,0 +1,53 @@ | |||
telemetry_logging | |||
========= |
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.
========= | |
================= |
roles/telemetry_logging/README.md
Outdated
For journal_tests.yml | ||
|
||
identifiers_test_id | ||
- polarion id for test |
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.
Pleas update this description
ansible.builtin.shell: | ||
cmd: | ||
tstamp=$(date -d '30 minute ago' "+%Y-%m-%d %H:%M:%S") | ||
journalctl -t "{{ item }}" --no-pager -S "${tstamp}" | wc -l |
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.
You could convert this to a command
if you want, by removing the use of wc and making use of journal_output.stdout_lines to count the number of returned lines. This would also make debugging easier, since using verbose output would let you see what was returned from the journalctl command, rather than just seeing a number.
The failed_when
condition could become ``journal_wc.stdout_lines | length <= 1
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/6fb59fd84b444f8e82a02eaf41d8cc6a ✔️ feature-verification-tests-noop SUCCESS in 4s |
@@ -0,0 +1,119 @@ | |||
common | |||
========= |
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.
========= | |
====== |
cmd: | | ||
oc get crd "{{ item }}" | ||
changed_when: false | ||
register: output |
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 there a fail condition for this? Does it need one?
oc project openstack | ||
kubectl exec openstackclient -- openstack endpoint list --service="{{ item[0] }}" --service="{{ item[1] }}" --interface="{{ item[2] }}" |
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.
What happens if the endpoint doesn't exist? Does the openstackclient return a non-zero return code?
roles/common/tasks/node_tests.yml
Outdated
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 don't believe these tests are valid.
Zuul doesn't know or care that the nodes are VMs, and the executer, which run the playbooks for the job, does not have these VMs running.
We pass nodesets into the jobs, but these are for zuul to give to nodepool, which provides the test servers.
In the case of the Zuul instance on rdo, nodepool talks to various cloud providers, which then provision VMs, based on the nodeset labels.
The only information that Zuul has/needs about the hosts (crc, computes, controller, etc) is the ansible inv file, which has the hostname, IP address, etc that zuul needs to run the playbooks that we pass to it.
- name: Verify Project exists - "{{ item }}" | ||
ansible.builtin.shell: | ||
cmd: | | ||
oc project "{{ item }}" |
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.
This will switch to a different project. This may cause unexpected behaviour of subsequent commands are run without a --namespace/-n
argument.
A better check would be
oc project list | grep "{{ item}}"
(please verify that this command is correct)
A more efficient approach might be to get the project list and then loop through the output to make sure that all the expected project are there.
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/74a17349039442d8a6732bc2f65be016 ✔️ feature-verification-tests-noop SUCCESS in 4s |
- name: Verify subscription | ||
ansible.builtin.shell: | ||
cmd: | | ||
oc get subscriptions -n "{{ subscription_nspace }}" "{{ item }}" |
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.
IMHO, this should have a similar format to the endpoint tests, so that a single loop can cover multiple namespaces.
- name: Get Pod Instance "{{ pod_status_str }}" | ||
ansible.builtin.shell: | ||
cmd: | | ||
oc get pods -n "{{ pod_nspace }}" | grep "{{ item }}" | grep "{{ pod_status_str }}" | awk '{print $1;}' |
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.
If this fails, there is no indication until the next task.
There is also no helpful information to see what might have caused the error.
The next task will fail if there is nothing returned from here.
oc get pods -n "{{ pod_nspace }}" | grep "{{ item }}" | grep "{{ pod_status_str }}" | awk '{print $1;}' | |
oc get pods -n "{{ pod_nspace }}" | grep "{{ item }}" | grep "{{ pod_status_str }}" | awk '{print $1;}' | |
failed_when: | |
- podinstance.stdout_lines | length == 0 |
- name: Check terminated pod | ||
ansible.builtin.shell: | ||
cmd: | | ||
oc get pod -n "{{ pod_nspace }} {{ podinstance.stdout }}" |
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.
oc get pod -n "{{ pod_nspace }} {{ podinstance.stdout }}" | |
oc get pod -n "{{ pod_nspace }}" "{{ podinstance.stdout }}" |
Please don't apply these suggestions, they are reflective of what is going on in PR#149
Add service tests to the logging job adding the tasks from #123 into a spearate PR
Merge QE Common role into master