Skip to content
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

Open
wants to merge 65 commits into
base: master
Choose a base branch
from
Open

Merge qe_common role #123

wants to merge 65 commits into from

Conversation

ayefimov-1
Copy link
Contributor

Merge QE Common role into master

Copy link
Collaborator

@elfiesmelfie elfiesmelfie left a 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
Copy link
Collaborator

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

cmd: |
oc get crd "{{ item }}"
register: output
failed_when: output.rc != 0
Copy link
Collaborator

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.

@@ -0,0 +1,64 @@
---
- when: container_list is defined
name: "Verify container - {{ container_polar_id}}"
Copy link
Collaborator

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.

Copy link
Collaborator

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.

loop: "{{ proj_list }}"


- when:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- when:
- when:

.ansible-lint Outdated
@@ -2,6 +2,8 @@
exclude_paths:
- ci/
- roles/telemetry_autoscaling
- roles/telemetry_logging
- roles/qe_common
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- roles/qe_common

@@ -0,0 +1,64 @@
---
- when: container_list is defined
name: "Verify container - {{ container_polar_id}}"
Copy link
Collaborator

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)

Comment on lines 32 to 35
- when: node_list is defined
name: "Verify OSP node - {{ node_polar_id }}"
ansible.builtin.include_tasks: node_tests.yml
loop: "{{ node_list }}"
Copy link
Collaborator

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

roles/qe_common/README.md Outdated Show resolved Hide resolved
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/838cb4e2e4644f569ca1f922fbe31a82

openstack-k8s-operators-content-provider FAILURE in 6m 06s
⚠️ functional-tests-on-osp18 SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

Copy link
Collaborator

@elfiesmelfie elfiesmelfie left a 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

Copy link
Contributor

@mgirgisf mgirgisf left a 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.

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/2fed2e1b296143c698fc94427414a526

✔️ feature-verification-tests-noop SUCCESS in 5s
✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 53m 37s
functional-tests-on-osp18 FAILURE in 1h 37m 21s

@elfiesmelfie
Copy link
Collaborator

Build failed (check pipeline). Post recheck (without leading slash) to rerun all jobs. Make sure the failure cause has been resolved before you rerun jobs.

https://review.rdoproject.org/zuul/buildset/2fed2e1b296143c698fc94427414a526

✔️ feature-verification-tests-noop SUCCESS in 5s ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 53m 37s ❌ functional-tests-on-osp18 FAILURE in 1h 37m 21s

This failure is unrelated to this change.
I synced with master which will trigger a new buildset


For subscription_tests.yml tasks:

subscription_polar_id
Copy link
Collaborator

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.
Copy link
Collaborator

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
Copy link
Collaborator

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.

Comment on lines +6 to +7
oc project openstack
kubectl exec openstackclient -- openstack endpoint list --service="{{ item[0] }}" --service="{{ item[1] }}" --interface="{{ item[2] }}"
Copy link
Collaborator

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

Copy link
Collaborator

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/container_tests.yml Outdated Show resolved Hide resolved
- name: Get container status
ansible.builtin.shell:
cmd: |
podman ps -a --format "{{ '{{.Names}} {{.Status}}' }}" | grep "{{ item }}" | awk '{print $2;}'
Copy link
Collaborator

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
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
Comment on lines +5 to +6
- roles/telemetry_logging
- roles/commmon
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- roles/telemetry_logging
- roles/commmon

hosts: controller
gather_facts: no
vars:
proj_out_file: verify_logging_projects_exist_lresults.log
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
proj_out_file: verify_logging_projects_exist_lresults.log


For subscription_tests.yml tasks:

subscription_polar_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.

@@ -0,0 +1,53 @@
telemetry_logging
=========
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
=========
=================

For journal_tests.yml

identifiers_test_id
- polarion id for test
Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/6fb59fd84b444f8e82a02eaf41d8cc6a

✔️ feature-verification-tests-noop SUCCESS in 4s
openstack-k8s-operators-content-provider FAILURE in 12m 52s
⚠️ functional-tests-on-osp18 SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

@@ -0,0 +1,119 @@
common
=========
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
=========
======

cmd: |
oc get crd "{{ item }}"
changed_when: false
register: output
Copy link
Collaborator

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?

Comment on lines +6 to +7
oc project openstack
kubectl exec openstackclient -- openstack endpoint list --service="{{ item[0] }}" --service="{{ item[1] }}" --interface="{{ item[2] }}"
Copy link
Collaborator

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?

Copy link
Collaborator

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.

roles/common/tasks/main.yml Outdated Show resolved Hide resolved
roles/common/tasks/main.yml Outdated Show resolved Hide resolved
- name: Verify Project exists - "{{ item }}"
ansible.builtin.shell:
cmd: |
oc project "{{ item }}"
Copy link
Collaborator

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.

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/74a17349039442d8a6732bc2f65be016

✔️ feature-verification-tests-noop SUCCESS in 4s
✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 29m 56s
functional-tests-on-osp18 FAILURE in 1h 42m 13s

- name: Verify subscription
ansible.builtin.shell:
cmd: |
oc get subscriptions -n "{{ subscription_nspace }}" "{{ item }}"
Copy link
Collaborator

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;}'
Copy link
Collaborator

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.

Suggested change
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 }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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

myadla added a commit that referenced this pull request Sep 27, 2024
Add service tests to the logging job

adding the tasks from #123 into a spearate PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants