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

Fixes for vagrant/libvirt #264

Merged
merged 1 commit into from
Aug 21, 2020
Merged

Fixes for vagrant/libvirt #264

merged 1 commit into from
Aug 21, 2020

Conversation

detiber
Copy link
Contributor

@detiber detiber commented Aug 20, 2020

Description

Fixes some issues with running the vagrant setup using libvirt:

  • Adds network name for libvirt private network
  • Adds a host_ip for libvirt private network, to allow creation of the network if it doesn't already exist

Why is this needed

Without these changes, vagrant-libvirt fails to provision the hosts or the private network.

How Has This Been Tested?

Successfully ran through the local setup with vagrant instructions here: https://tinkerbell.org/docs/setup/local-with-vagrant/

Host OS: Fedora 32
Vagrant version: 2.2.9 (installed from RPM)
vagrant-libvirt version: 0.1.2 (installed using vagrant plugin install)

How are existing users impacted? What migration steps/scripts do we need?

I don't expect any impact to existing users, unless those users were also running into issues standing up the vagrant stack with libvirt

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #264 into master will decrease coverage by 0.62%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
- Coverage   12.98%   12.36%   -0.63%     
==========================================
  Files           7        7              
  Lines        1186     1165      -21     
==========================================
- Hits          154      144      -10     
+ Misses       1021     1010      -11     
  Partials       11       11              
Impacted Files Coverage Δ
grpc-server/tinkerbell.go 90.83% <0.00%> (-0.65%) ⬇️
grpc-server/workflow.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bed1c6...b292e2f. Read the comment docs.

@detiber detiber added the ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ label Aug 20, 2020
displague
displague previously approved these changes Aug 20, 2020
Copy link
Member

@displague displague left a comment

Choose a reason for hiding this comment

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

I verified this by running through the https://tinkerbell.org/docs/setup/local-with-vagrant/ docs on Ubuntu 20.04.

To fulfill the prerequisites, I ran:

sudo apt-get install qemu-system-x86 libvirt-daemon-driver-qemu

It was helpful to use virt-manager to view the Worker. (I attempted to use virsh console vagrant_worker but never caught meaningful output).

The build failure seems to have hit the 5m timeout, it does appear that vagrant was otherwise successful in provisioning Virtualbox nodes. Breaking virtualbox based vagrant provisioning is the only risk that I could imagine this PR doing, and that does not appear to be the case.

@gianarb gianarb added ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Aug 21, 2020
- Adds network name for libvirt private network
- Adds a host_ip for libvirt private network, to allow creation of the network if it doesn't already exist+

Signed-off-by: Jason DeTiberus <[email protected]>
@gianarb gianarb added ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Aug 21, 2020
@gianarb gianarb self-requested a review August 21, 2020 15:28
@gianarb gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Aug 21, 2020
@mergify mergify bot merged commit 1ba20b8 into tinkerbell:master Aug 21, 2020
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants