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

[Issue #112] OS X Monterey and vbox 6.1.28 compatibility changes to Vagrantfile #114

Conversation

ProvenGuilty
Copy link

@ProvenGuilty ProvenGuilty commented Nov 7, 2021

Description

This is will change the default network from 192.168.50.0/24 to 192.168.56.0/24.

Why is this needed

This is suddenly broken in a recent version of Oracle virtualbox version 6.1.28 and greater. This can be observed when the host operating system is Linux, Mac OS X or Solaris follwing changes to allowable IP ranges on the hostonlyif of 192.168.56.0/21

Fixes: #112

How Has This Been Tested?

I have tested using an intel based MacBook Pro 16" 2019 Model running the latest Version of Mac OS X Monterey. It was discovered that the provisioner VM would not provision correctly without these changes.

Upon testing with these changes in the Vagrantfile using vbox, I was able to revert and verify it was no longer working again. There are other suggestions to manually create a networks.conf file in /etc/vbox to specify allowed RFC 1911 space. However, this pull request specifically targets the vbox config in the Vagrant file in a more desirable way, as it will work-around the issue without changing core functionality of the virtualbox installation and other user configurations that may be present.

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

Changes the default network the provisioner and ultimately PXE network runs in from 192.168.50.0/24 to 192.168.56.0/24 for all users (not limited to Mac OS X users). Existing users will likely need to re-provision their existing installations when upgrading to Monterey and VirtualBox 6.1.28

Checklist:

  • updated the documentation and/or roadmap (I did not see the network specified in the documentation)
  • added unit or e2e tests (tested using virtualbox 6.1.30 on OSX monterey and vagrant up provisioner)
  • provided instructions on how to upgrade

@ProvenGuilty ProvenGuilty changed the title Cryan/sandbox vagrantvboxpatchosxmonterey [Issue #112] OS X Monterey and vbox 6.1.28 compatibility changes to Vagrantfile Nov 7, 2021
@ProvenGuilty
Copy link
Author

Upon successful vagrant up, it appears the following configurations are also dynamically updated. Open to suggestions if this should also be included as a commit to this pull request or if allowing them to dynamically update is more desirable.

cryan@A-1051 sandbox % git status
On branch cryan/sandbox-vagrantvboxpatchosxmonterey
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   deploy/compose/manifests/hardware/hardware.json
	modified:   deploy/compose/manifests/template/ubuntu.yaml
	modified:   deploy/compose/tls/csr.json

Diff of the changed files:

cryan@A-1051 sandbox % git diff
diff --git a/deploy/compose/manifests/hardware/hardware.json b/deploy/compose/manifests/hardware/hardware.json
index 3e89242..0fc4496 100644
--- a/deploy/compose/manifests/hardware/hardware.json
+++ b/deploy/compose/manifests/hardware/hardware.json
@@ -15,7 +15,7 @@
         "dhcp": {
           "arch": "x86_64",
           "ip": {
-            "address": "192.168.50.43",
+            "address": "192.168.56.43",
             "netmask": "255.255.255.0"
           },
           "mac": "08:00:27:9e:f5:3a",
diff --git a/deploy/compose/manifests/template/ubuntu.yaml b/deploy/compose/manifests/template/ubuntu.yaml
index 6cac7d8..65fd757 100644
--- a/deploy/compose/manifests/template/ubuntu.yaml
+++ b/deploy/compose/manifests/template/ubuntu.yaml
@@ -14,7 +14,7 @@ tasks:
         timeout: 600
         environment:
           DEST_DISK: /dev/sda
-          IMG_URL: "http://192.168.50.4:8080/focal-server-cloudimg-amd64.raw.gz"
+          IMG_URL: "http://192.168.56.4:8080/focal-server-cloudimg-amd64.raw.gz"
           COMPRESSED: true
       - name: "install-openssl"
         image: cexec:v1.0.0
diff --git a/deploy/compose/tls/csr.json b/deploy/compose/tls/csr.json
index 2a896d2..c4a135e 100644
--- a/deploy/compose/tls/csr.json
+++ b/deploy/compose/tls/csr.json
@@ -1,6 +1,7 @@
 {
   "CN": "tinkerbell",
   "hosts": [
+    "192.168.56.4",
     "tinkerbell.registry",
     "tinkerbell.tinkerbell",
     "tinkerbell",

@ProvenGuilty ProvenGuilty force-pushed the cryan/sandbox-vagrantvboxpatchosxmonterey branch from 30c00cd to 0ce90fb Compare November 9, 2021 22:15
@mmlb
Copy link
Contributor

mmlb commented Nov 10, 2021

Hey @ProvenGuilty welcome. I've updated the PR description a bit so it doesn't seem to be vbox specific (since you did end up messing with libvirt config). Please do update the modified files in deploy/ directory.

@mmlb
Copy link
Contributor

mmlb commented Nov 10, 2021

Maybe only do the v.gui = true if the os is macos?

@ProvenGuilty ProvenGuilty force-pushed the cryan/sandbox-vagrantvboxpatchosxmonterey branch from a907a12 to 0637142 Compare November 29, 2021 04:10
@ProvenGuilty
Copy link
Author

Sorry for the sloppiness I almost never use public GitHub and there are so many things new to me on this platform. :)

@nshalman
Copy link
Member

@tstromberg linked me to kubernetes/minikube#12811 which may help in reviewing this.

@mmlb
Copy link
Contributor

mmlb commented Nov 30, 2021

Virtualbox 6.1.30 is out with fixes for the gui https://www.virtualbox.org/wiki/Changelog can you try that out @ProvenGuilty ?

@ProvenGuilty
Copy link
Author

Verified with 6.1.30 installed on OSX Monterey. This test was performed with commenting out the conditional I added in previous commit:
https://gist.github.com/ProvenGuilty/75f7430e74171bcbbd194b4d3c1c0a4a

When re-testing using the conditional, it still ran successfully with a gui window poppping up. I would conclude that virtualbox fixed the introduced bug and the conditional for darwin/macosx is no longer necessary.

@mmlb
Copy link
Contributor

mmlb commented Nov 30, 2021

Nice, can you drop the gui stuff from the PR then?

@ProvenGuilty ProvenGuilty force-pushed the cryan/sandbox-vagrantvboxpatchosxmonterey branch from 8ed88fa to 7f15229 Compare November 30, 2021 22:31
@ProvenGuilty
Copy link
Author

Closing PR, this doesn't seem to be an issue any more when switching back to 192.168.50.0/24 either..

@ProvenGuilty
Copy link
Author

I lied. It wasn't until I destroyed and re-created the provisioner that I found the error again:

allowed ranges. Please update the address used to be within the allowed
ranges and run the command again.

  Address: 192.168.50.4
  Ranges: 192.168.56.0/21

Valid ranges can be modified in the /etc/vbox/networks.conf file. For
more information including valid format see:

  https://www.virtualbox.org/manual/ch06.html#network_hostonly

@ProvenGuilty ProvenGuilty reopened this Nov 30, 2021
@ProvenGuilty ProvenGuilty force-pushed the cryan/sandbox-vagrantvboxpatchosxmonterey branch 2 times, most recently from b93a586 to 05d6b59 Compare November 30, 2021 23:11
@ProvenGuilty
Copy link
Author

I don't know why DCO is still unhappy, I tried to fix it like I was able to previously.

@ProvenGuilty
Copy link
Author

I dug further into this and found the netmask was embedded in many more files and is still an issue for Linux, OSX and Solaris users running VirtualBox 6.1.28 or higher. Reopening this once more to apply the changes uniform across tf, vbox and libvirtd.

@ProvenGuilty ProvenGuilty reopened this Dec 3, 2021
@ProvenGuilty ProvenGuilty force-pushed the cryan/sandbox-vagrantvboxpatchosxmonterey branch from 51629d9 to ee67d9b Compare December 3, 2021 06:59
@ProvenGuilty
Copy link
Author

ProvenGuilty commented Dec 10, 2021

@mmlb & @nshalman this still wanted? I recall discussion around if you were wanting to remove vbox support altogether.

@mmlb mmlb force-pushed the cryan/sandbox-vagrantvboxpatchosxmonterey branch 2 times, most recently from 33dbf2f to e91972b Compare December 10, 2021 17:03
@mmlb
Copy link
Contributor

mmlb commented Dec 10, 2021

Yes indeed, lgtm. I've rebased and squashed all the intermediate changes to get rid of things we didn't need.

@mmlb mmlb force-pushed the cryan/sandbox-vagrantvboxpatchosxmonterey branch from e91972b to 5968f41 Compare December 10, 2021 17:11
Change netmask from 192.168.50.0/24 to 192.168.56.0/24

Fixes #228

Signed-off-by: Christopher Ryan <cryan@llnw.com>
@mmlb mmlb force-pushed the cryan/sandbox-vagrantvboxpatchosxmonterey branch from 5968f41 to 5dee0ea Compare December 10, 2021 17:12
@mmlb mmlb added the ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ label Dec 10, 2021
@mmlb
Copy link
Contributor

mmlb commented Dec 10, 2021

I just remembered we have a label I can apply to get this tested in ci, so lets see how that goes.

@mmlb
Copy link
Contributor

mmlb commented Dec 10, 2021

Which failed, but I very much doubt its because of this PR. Looks like a vagrant issue.

@mmlb mmlb removed the request for review from jacobweinstock December 10, 2021 17:14
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Dec 10, 2021
@mmlb
Copy link
Contributor

mmlb commented Dec 10, 2021

Good stuff, thanks @ProvenGuilty !

@ProvenGuilty
Copy link
Author

Thank you! This is the first project I've ever contributed to!

@mergify mergify bot merged commit 456b738 into tinkerbell:main Dec 10, 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/ ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vagrant sandbox not working on Virtualbox 6.1.28
3 participants