NO-ISSUE: Add ansible plays for e2e tests #95
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@CrystalChun: This pull request explicitly references no jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
approved
|
Nice idea, however can we extract steps in reusable roles? That way if we change one, we do not need to propagate the change in multiple files. No need to do all at once, if we can split into reusable pieces we can tackle bit by bit, but let's try not to introduce duplicate tasks |
|
Definitely! This was just a first pass that I did so I can perform these easily and I thought I could push it up so we both can use it, but will come back to this to create reusable roles! |
Adds the following ansible plays - cleanup.yaml: cleans up the environment by removing all hosts, vms and the kind cluster - deploy_controllers.yaml: allows for redeploying the controllers in an already setup test environment. Useful for testing code changes - hosts.yaml: Creates the host VMs and BMHs
CrystalChun
force-pushed
the
additional-e2e-ansible-plays
branch
from
0a54ea2 to
e72a5d3
Compare
WalkthroughThis pull request introduces several new files under the Changes
Sequence Diagram(s)Deploy Controllers FlowsequenceDiagram
participant U as User
participant AR as Ansible Runner
participant SC as Source Code
participant DC as Docker/Images
participant KC as Kind Cluster
participant K8s as Kubernetes API
U->>AR: Run deploy_controllers playbook
AR->>SC: Synchronize source files
AR->>DC: Execute Makefile to build images
AR->>KC: Load images into Kind cluster
AR->>SC: Copy distribution files
AR->>SC: Modify YAML (imagePullPolicy)
AR->>K8s: Apply YAML for deployment
K8s-->>AR: Acknowledge application changes
AR-->>U: Return deployment status
Cleanup FlowsequenceDiagram
participant U as User
participant AR as Ansible Runner
participant TE as Test Environment
U->>AR: Run cleanup playbook
AR->>TE: Copy VM destruction script
AR->>TE: Execute script to destroy VMs and volumes
AR->>TE: Remove network and pod containers
AR->>TE: Delete Kind cluster
TE-->>AR: Confirm cleanup actions
AR-->>U: Return cleanup status
Hosts Deployment FlowsequenceDiagram
participant U as User
participant AR as Ansible Runner
participant TE as Test Environment
U->>AR: Run hosts playbook
AR->>TE: Sync source files and set environment variables
AR->>TE: Execute VM creation and BMH scripts
AR->>TE: Configure SSH and pull secrets
TE-->>AR: Acknowledge host deployment
AR-->>U: Return host deployment status
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
test/ansible/cleanup.yaml (2)
15-19: Trailing Spaces in Shell Script Function
There are trailing spaces on line 16 within thedestroy_vmfunction. Removing these extra spaces will help conform to YAML linting standards and improve readability.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 16-16: trailing spaces
(trailing-spaces)
45-45: Remove Extra Blank Line
An extra blank line is present at line 45. Removing it will improve the overall formatting of the YAML file.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 45-45: too many blank lines
(1 > 0) (empty-lines)
test/ansible/hosts.yaml (1)
65-65: Remove Trailing Spaces in VM Creation Script
There are trailing spaces at the end of line 65 in the VM creation script block. Please remove them to adhere to YAML style guidelines and prevent potential issues with script execution.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 65-65: trailing spaces
(trailing-spaces)
test/ansible/README.md (2)
5-5: Improve Grammar in Documentation
In the documentation under "Deploy controllers," the phrase "in an already setup test environment" would be clearer as "in an already set-up test environment" or "in an established test environment."🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: You used an adverb (‘already’) instead of an adjective, or a noun (‘setup’) instead of another adjective.
Context: ...Shift Agent Bootstrap controllers in an already setup test environment. Useful for testing co...(A_RB_NN)
22-22: Enhance Conciseness in Documentation
In the "Environment Cleanup" section, consider rephrasing "removing all of the hosts (VMs and network)" to "removing all hosts (VMs and network)" for improved readability.🧰 Tools
🪛 LanguageTool
[style] ~22-~22: Consider removing “of” to be more concise
Context: ...destroy the e2e environment by removing all of the hosts (VMs and network), the sushy tool...(ALL_OF_THE)
test/ansible/deploy_controllers.yaml (3)
23-24: Fix Indentation in Loop List
The list under theloop:key (lines 24–25) seems to have incorrect indentation (expected 4 spaces but found 6). Please correct the indentation to follow proper YAML formatting.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 24-24: wrong indentation: expected 4 but found 6
(indentation)
32-32: Remove Trailing Spaces
There are trailing spaces at the end of line 32 in the "copy distfiles" task. Removing these spaces will help maintain consistent formatting across the playbook.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 32-32: trailing spaces
(trailing-spaces)
38-38: Remove Extra Blank Line
An unexpected blank line is present at line 38. Please remove it to ensure consistent formatting in the playbook.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 38-38: too many blank lines
(1 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/ansible/README.md(1 hunks)test/ansible/cleanup.yaml(1 hunks)test/ansible/deploy_controllers.yaml(1 hunks)test/ansible/hosts.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
test/ansible/deploy_controllers.yaml
[warning] 24-24: wrong indentation: expected 4 but found 6
(indentation)
[error] 32-32: trailing spaces
(trailing-spaces)
[warning] 38-38: too many blank lines
(1 > 0) (empty-lines)
test/ansible/cleanup.yaml
[error] 16-16: trailing spaces
(trailing-spaces)
[warning] 45-45: too many blank lines
(1 > 0) (empty-lines)
test/ansible/hosts.yaml
[error] 65-65: trailing spaces
(trailing-spaces)
🪛 LanguageTool
test/ansible/README.md
[grammar] ~5-~5: You used an adverb (‘already’) instead of an adjective, or a noun (‘setup’) instead of another adjective.
Context: ...Shift Agent Bootstrap controllers in an already setup test environment. Useful for testing co...
(A_RB_NN)
[style] ~22-~22: Consider removing “of” to be more concise
Context: ...destroy the e2e environment by removing all of the hosts (VMs and network), the sushy tool...
(ALL_OF_THE)
| virsh vol-list --pool default | grep -e qcow2 -e img | while read volume ; do | ||
| volumeName=$(echo volume | awk '{print $1}') | ||
| virsh vol-delete --pool default $volumeName | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Variable Expansion in destroy_volumes Function
Within the destroy_volumes function, the command
volumeName=$(echo volume | awk '{print $1}')
appears to be missing a dollar sign for the variable reference. It likely should be:
volumeName=$(echo "$volume" | awk '{print $1}')
to properly capture the volume name.
| # destroy vm volumes | ||
| - name: destroy libvirt network | ||
| shell: "(virsh net-destroy bmh || true) && (virsh net-undefine bmh || true)" |
There was a problem hiding this comment.
Duplicate Task and Incorrect Command for Volume Destruction
The task at lines 35–37 is named “destroy vm volumes” but its command duplicates the libvirt network destruction logic. It appears that this task was intended to invoke the destroy_volumes function rather than repeating the network destruction. Please update the task name and command accordingly.
| - name: Escape SSH auth | ||
| shell: echo {{ ssh_authorized_key }} | sed 's/\ /\\ /g' | ||
| register: ssh_authorized_key | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote and Correctly Reference SSH Authorized Key
In the "Escape SSH auth" task, the shell command uses:
echo {{ ssh_authorized_key }} | sed 's/\ /\\ /g'
To avoid issues with spaces or special characters, it’s recommended to wrap the variable in quotes (e.g. echo "{{ ssh_authorized_key }}"). Also, since the task registers its output, subsequent references (e.g., when setting environment variables) should use {{ ssh_authorized_key.stdout }}.
| - name: Define SSH_AUTHORIZED_KEY | ||
| lineinfile: | ||
| path: ~/.test-config | ||
| line: "export SSH_AUTHORIZED_KEY='{{ ssh_authorized_key }}'" | ||
| create: true | ||
| state: present | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reference the Registered Command Output Properly
In the "Define SSH_AUTHORIZED_KEY" task, the line is set using:
export SSH_AUTHORIZED_KEY='{{ ssh_authorized_key }}'
Since ssh_authorized_key is a registered variable from a shell command, please reference its output using {{ ssh_authorized_key.stdout }} to ensure that only the intended string value is used.
| # create BMHs | ||
| - name: Copy BMH creation script | ||
| copy: | ||
| dest: "/tmp/create_bmhs" | ||
| content: | | ||
| #!/bin/bash | ||
| i=0 | ||
| for systemid in $(curl -s 192.168.222.1:8000/redfish/v1/Systems | jq -r '.Members[]."@odata.id"'); do | ||
| echo "starting VM BMH..." | ||
| i=$((i+1)) | ||
| name=$(curl -s 192.168.222.1:8000${systemid} | jq -r '.Name') | ||
| sed -r "s%redfish-virtualmedia.*REPLACE_ID%redfish-virtualmedia+http://192.168.222.1:8000${systemid}%" {{ src_dir}}/test/e2e/bmh/bmh.yaml.tpl | sed -r "s/REPLACE_NAME/${name}/g" | sed -r "s/REPLACE_MAC/00:60:2f:31:81:0${name:0-1}/g" | kubectl -n {{ test_namespace }} apply -f - | ||
| echo "done" | ||
| done |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Validate Shell Substring Extraction Syntax
Within the BMH creation script, the sed command
sed -r "s/REPLACE_MAC/00:60:2f:31:81:0${name:0-1}/g"
uses a substring extraction syntax (${name:0-1}) that may not work as expected in bash. Verify that this syntax correctly extracts the intended portion of the variable. Adjust if necessary (for example, using ${name: -1} for the last character or ${name:0:1} for the first).
Action Required: Fix Invalid Substring Extraction Syntax in BMH Creation Script
After reviewing the code, the substring extraction syntax used in the sed command is problematic. The expression
${name:0-1}will not correctly extract the intended substring in bash. If the goal is to obtain the last character of the variable name, it should be adjusted to:
${name: -1}(note the space before the minus sign)
Alternatively, if you need the first character instead, consider using ${name:0:1}.
Please update the sed command in test/ansible/hosts.yaml at lines 75-88 accordingly to ensure the script functions as expected.
|
@CrystalChun: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@CrystalChun can this be closed? The e2e tests have been now refactored, it would be much easier to create different playbooks. |
|
/close |
|
@CrystalChun: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Separates the main play into the following ansible plays for easily running one section of the test:
Summary by CodeRabbit