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.

Sign up for GitHub

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

Jump to bottom

Added deploy-bm-hypervisor.yml to playbooks/infra #28

Merged
merged 1 commit into from
Jan 13, 2025
Merged

Added deploy-bm-hypervisor.yml to playbooks/infra #28

merged 1 commit into from
Jan 13, 2025

Conversation

kononovn
Copy link
Collaborator

@kononovn kononovn commented Jan 8, 2025

No description provided.

@openshift-ci openshift-ci bot requested review from ccardenosa and shaior January 8, 2025 21:08
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2025
@natifridman
Copy link
Collaborator

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2025
@natifridman
Copy link
Collaborator

Could you please drop the commit store external dependencies under collection directory?
It's really hard to review this with 5000 files. I think we should have a requirements file here instead of storing the files in this repo

shaior
shaior reviewed Jan 9, 2025
View reviewed changes
natifridman
natifridman reviewed Jan 9, 2025
View reviewed changes
@kononovn kononovn force-pushed the bm_deploymnet branch 3 times, most recently from 5745d04 to ad8cb02 Compare January 9, 2025 17:35
natifridman
natifridman reviewed Jan 9, 2025
View reviewed changes
playbooks/infra/deploy-bm-hypervisor.yml
hosts: bastion
gather_facts: true
vars:
iso_mount_path: "{{ dest_iso_dir }}/mount"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is dest_iso_dir declared?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's declared in inventory file of bastion and stored in our vault.

Copy link
Collaborator

Choose a reason for hiding this comment

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

WDYT about adding an example inventory file so new users will know what it should look like?

playbooks/infra/deploy-bm-hypervisor.yml

- name: Activate OS
ansible.builtin.command:
"{{ activate_system_cmd }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this declared?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In host variable file stored in our vault.

playbooks/infra/deploy-bm-hypervisor.yml
state: present
autoconnect: true

- name: Reload NetworkManager connections
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use nmcli module here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well it restarts external interface which is used by Ansible. I've tried multiple ways to restart it via nmcli module but Ansilbe lost connection and execution failed. Looks like the only way to restart external connection is to transfer script to the host and execute it. That's the root cause of using shell module.

@kononovn kononovn force-pushed the bm_deploymnet branch 2 times, most recently from 58ad59c to e661c4d Compare January 9, 2025 21:10
@kononovn kononovn requested a review from natifridman January 9, 2025 21:10
ccardenosa
ccardenosa reviewed Jan 10, 2025
View reviewed changes
Copy link
Collaborator

@ccardenosa ccardenosa left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2025
@ccardenosa
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2025
natifridman
natifridman reviewed Jan 12, 2025
View reviewed changes
Copy link
Collaborator

@natifridman natifridman left a comment

Choose a reason for hiding this comment

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

Looks good. Maybe we can add a small readme to playbooks/infra to explain what is this playbook for?

shaior
shaior reviewed Jan 12, 2025
View reviewed changes
playbooks/infra/deploy-bm-hypervisor.yml
changed_when: true

- name: Gather facts
ansible.builtin.gather_facts:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add true here, just for clarity and readability?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It causes linting error.

Copy link
Collaborator

@ccardenosa ccardenosa Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Correct. Adding gather_facts as task like here is true implicitly. The expected value is a dict, not a boolean.

@openshift-ci openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 13, 2025
@kononovn
Copy link
Collaborator Author

Looks good. Maybe we can add a small readme to playbooks/infra to explain what is this playbook for?

Added inline Readme to the top of playbook as comment. Please review.

@kononovn
Add hypervisor deployment on baremetal using Kickstart and iDRAC boot
6c8def1 
  - Implemented Kickstart automation for hypervisor installation.
  - Configured iDRAC boot for baremetal provisioning.
  - Added necessary configurations and templates for network and disk settings.
  - Integrated automation for seamless OS installation with zero intervention.
  - Updated .gitignore file
  - Updated requirements.yml
  - Added ansible.cfg file
natifridman
natifridman approved these changes Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@natifridman natifridman left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2025
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jan 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natifridman

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2025
@shaior
Copy link
Contributor

shaior commented Jan 13, 2025

/lgtm

@natifridman
Copy link
Collaborator

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 9efe0e7 into openshift-kni:main Jan 13, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaior shaior shaior left review comments

@ccardenosa ccardenosa ccardenosa left review comments

@natifridman natifridman natifridman approved these changes

Assignees

@natifridman natifridman

@shaior shaior

@ccardenosa ccardenosa

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kononovn @natifridman @ccardenosa @shaior