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 Network changes in VBox 7.x #31

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

chombium
Copy link
Contributor

@chombium chombium commented Jul 4, 2023

The output of the VBoxManage list natnetworks CLI command was changed in VirtualBox v7.x. This change adjusts the parameter names and uses the --long format of the command's output so that the Enabled parameter is present.

The change is compatible with the older VirtualBox version and does not break the CPI.

Fixes #30

The output of the `VBoxManage list natnetworks` CLI command was changed in
VirtualBox v7.x. This change adjusts the parameter names and uses the
`--long` format of the command's output so that the `Enabled` parameter is
present.

The change is compatible with the older VirtualBox version and does not
break the CPI.

Fixes cloudfoundry#30

Co-authored-by: Pascal Zimmermann <pascal.zimmermann@theiotstudio.com>
@chombium chombium marked this pull request as draft July 4, 2023 11:31
@wayneeseguin
Copy link

Hi, I would love to help test this out also. Are you able to make the blobs available?

@chombium
Copy link
Contributor Author

chombium commented Jul 28, 2023

Hi @wayneeseguin,

Thank you very much for the offered help. What OS are you using? We have tested the changes on Linux and we're currently refactoring the Mac OS creation of networks as it has drastically changed and some relatively bigger changes are needed.

I can build and upload the current state of the cpi release, if you want to test it on Linux or Windows. We still haven't tested anything on Windows and we don't have a clue what might be possibly wrong there.

@wayneeseguin
Copy link

wayneeseguin commented Jul 28, 2023

My main workstation is Linux, and I have Windows and Mac (M1) laptops.
You can message me on CF slack under the same username.

@chombium
Copy link
Contributor Author

chombium commented Aug 3, 2023

Hi @wayneeseguin, sorry for the late reply. Here is a link to the latest version of the cpi package. Check the following comment for details on how to test. You will need to adjust the patht to the .tgz package in the cpi.yml file used the bosh create-env command.

Thank you for your patience and the offered help.

@wayneeseguin
Copy link

OK, I will try to test it on my windows and Mac boxes this weekend

@ZPascal
Copy link
Contributor

ZPascal commented Aug 3, 2023

Hi @wayneeseguin, I am working with Jovan on this issue. I assume that the tar archive does not contain the Mac version yet, since I just implemented it today and have not tested it.

@chombium
Copy link
Contributor Author

chombium commented Aug 3, 2023

I will build and upload the latest versiot tomorrow

@ZPascal ZPascal force-pushed the network-param-name branch 2 times, most recently from 289eddf to 7d7a047 Compare August 4, 2023 10:04
*feat: Add the IP adress calculation
*feat: Adjust the documentation and err handling
*fix: Adjust the code base to solve a few issues, identified by the test execution of the Bosh lite setup
*fix: Adjust the code base to solve the hostonly vm issues
@ZPascal
Copy link
Contributor

ZPascal commented Aug 8, 2023

Hi @wayneeseguin, I've modified the corresponding code base to fix the MAC OS X related issues. I've published the new tar archive here. Feel free to use it for your tests.

@wayneeseguin
Copy link

I have downloaded the updated one thanks Pascal!

@chombium chombium marked this pull request as ready for review August 9, 2023 13:52
@chombium
Copy link
Contributor Author

chombium commented Aug 9, 2023

We (me and @ZPascal) are done with the changes and the PR is ready for review. We have tested everything on Linux and MacOS. We are waiting for @wayneeseguin to test a bit more and we can merge it.

@chombium chombium changed the title [WIP] Fixes NAT Network parsing in VBox 7.x Fixes NAT Network parsing in VBox 7.x Aug 9, 2023
@chombium chombium changed the title Fixes NAT Network parsing in VBox 7.x Fixes Network changes in VBox 7.x Aug 9, 2023
@beyhan beyhan requested review from a team, mvach, cunnie and ramonskie and removed request for a team and mvach August 10, 2023 14:43
@ramonskie
Copy link
Contributor

tested it with

  • virtualbox 7 it works.
  • virtualbox 6.1 it fails

@ZPascal
Copy link
Contributor

ZPascal commented Aug 11, 2023

Hi @ramonskie, have you performed the test on Mac or Linux?

@chombium
Copy link
Contributor Author

chombium commented Aug 14, 2023

@ramonskie Thanks for testing. I can confirm that it broken on VBox 6.1 on Linux(Ubuntu 20.04 in my case). I will do some tests and fix it this week.

The error doesn't say much, but it look similar to the initial one:

Deploying:
  Creating instance 'bosh/0':
    Creating VM:
      Creating vm with stemcell cid 'sc-07c91907-3a6e-4e19-539b-3fa054203ef8':
        CPI 'create_vm' method responded with error: CmdError{"type":"Bosh::Clouds::CloudError","message":"Creating VM with agent ID '{{37cd10ed-32cf-4689-58ed-24d7a689177f}}': Enabling networks: Expected to find network 'NatNetwork'","ok_to_retry":false}


@lnguyen
Copy link
Member

lnguyen commented Aug 15, 2023

This is great. Thank you @chombium

@chombium
Copy link
Contributor Author

@ramonskie Can you please test again? I've fixed the problem and now the CPI works on Linux with VBox 6.1 and 7. You can download the package here.

@ramonskie
Copy link
Contributor

just tested it on virtualbox 6.1 and 7.0 and it works now

@chombium
Copy link
Contributor Author

We've done some tests on Windows as well and have seen that the bosh cli has a Go flag UseIsolatedEnv hardcoded which is not supported on Windows. @ZPascal has written some more details. Hence, I would say Windows is out of the question for this PR.

@ZPascal
Copy link
Contributor

ZPascal commented Aug 31, 2023

FYI @chombium I've also opened an follow up issue on BOSH CLI side, to further discuss the root cause.

@rkoster rkoster merged commit 9a263ed into cloudfoundry:master Aug 31, 2023
@rkoster
Copy link
Contributor

rkoster commented Aug 31, 2023

Thanks! @chombium & @ZPascal

@ZPascal ZPascal deleted the network-param-name branch August 31, 2023 15:00
@ZPascal
Copy link
Contributor

ZPascal commented Aug 31, 2023

@rkoster Can you please create a new release version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Small breaking change in vbox 7
6 participants