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

Add functionality to ensure the control host-only interface (vboxnet0) with a configured DHCP server exists before proceeding to deploy a virtualbox. Create the interface and the DHCP servers as needed. Added unittests #202

Closed
wants to merge 5 commits into from

Conversation

@goodwillcoding
Copy link

@goodwillcoding goodwillcoding commented Jun 21, 2014

No description provided.

…) with a configured DHCP server exists before proceeding to deploy a virtualbox. Create the interface and the DHCP servers as needed. Added unittests for this functionality
@aszlig

This comment has been minimized.

Copy link

@aszlig aszlig commented on d64dbb6 Jun 21, 2014

Although I'm personally not a big fan of the # ~* # comments, why not move the tests to where the other tests reside?

This comment has been minimized.

Copy link
Owner Author

@goodwillcoding goodwillcoding replied Jun 21, 2014

The reason I put these tests under backends/virtualbox is because nixops.backend is a provided namespace which tells me other backends can be installed in other packages and would have tests in their respective backend folders.

@aszlig

This comment has been minimized.

Indentation mismatch. Although there are things that @edolstra disagrees (like line length wrap at 79 characters) I'd at least aim to follow PEP-8, even though quite a lot of code in the project doesn't do it. Because if we want to some day merge/redo NixOS#116 it might be easier if we don't add new code that needs to be fixed afterwards.

This comment has been minimized.

Copy link
Owner Author

@goodwillcoding goodwillcoding replied Jun 21, 2014

I did not follow PEP8 cause the rest of the code did not. I can fix this though if you really want.

@aszlig

This comment has been minimized.

Copy link

@aszlig aszlig commented on nixops/backends/virtualbox.py in d64dbb6 Jun 21, 2014

Unless I've missed something I guess this should be ensure_control_hostonly_interface without the underscore, right?

This comment has been minimized.

Copy link
Owner Author

@goodwillcoding goodwillcoding replied Jun 21, 2014

thanks. typo, will fix.

@aszlig

This comment has been minimized.

Copy link

@aszlig aszlig commented on nixops/backends/virtualbox.py in d64dbb6 Jun 21, 2014

If I understand this function correctly, if there are for example three blocks separated by \n\n, only two blocks would be processed, except if there is a trailing \n\n after the last block. Not sure whether this is desired behaviour.

This comment has been minimized.

Copy link
Owner Author

@goodwillcoding goodwillcoding replied Jun 21, 2014

VBoxManage list hostonlyifs / dhcpserver does have a trailing \n\n so this works well.

$ echo "---begin---";VBoxManage list hostonlyifs;echo "---end---"
---begin---
Name:            vboxnet0
GUID:            786f6276-656e-4074-8000-0a0027000000
DHCP:            Disabled
IPAddress:       192.168.56.1
NetworkMask:     255.255.255.0
IPV6Address:     fe80:0000:0000:0000:0800:27ff:fe00:0000
IPV6NetworkMaskPrefixLength: 64
HardwareAddress: 0a:00:27:00:00:00
MediumType:      Ethernet
Status:          Up
VBoxNetworkName: HostInterfaceNetworking-vboxnet0

Name:            vboxnet1
GUID:            786f6276-656e-4174-8000-0a0027000001
DHCP:            Disabled
IPAddress:       192.168.57.1
NetworkMask:     255.255.255.0
IPV6Address:     
IPV6NetworkMaskPrefixLength: 0
HardwareAddress: 0a:00:27:00:00:01
MediumType:      Ethernet
Status:          Down
VBoxNetworkName: HostInterfaceNetworking-vboxnet1

Name:            vboxnet2
GUID:            786f6276-656e-4274-8000-0a0027000002
DHCP:            Disabled
IPAddress:       192.168.58.1
NetworkMask:     255.255.255.0
IPV6Address:     
IPV6NetworkMaskPrefixLength: 0
HardwareAddress: 0a:00:27:00:00:02
MediumType:      Ethernet
Status:          Down
VBoxNetworkName: HostInterfaceNetworking-vboxnet2

---end---
@aszlig

This comment has been minimized.

Copy link

@aszlig aszlig commented on nixops/backends/virtualbox.py in d64dbb6 Jun 21, 2014

Something missing here?

This comment has been minimized.

Copy link
Owner Author

@goodwillcoding goodwillcoding replied Jun 21, 2014

fixing to better wording

@aszlig

This comment has been minimized.

Copy link

@aszlig aszlig commented on nixops/backends/virtualbox.py in d64dbb6 Jun 21, 2014

If there is a limitation to only use vboxnet0 and it's also hardcoded in the constructor, why do we need these checks (almost) twice?

This comment has been minimized.

Copy link
Owner Author

@goodwillcoding goodwillcoding replied Jun 21, 2014

there will be a follow up PR where the user can specify the interface using the command line. This is just prep and I do not see it really hurting.

@aszlig

This comment has been minimized.

Copy link

@aszlig aszlig commented on nixops/backends/virtualbox.py in d64dbb6 Jun 21, 2014

Looks like a lot of repetition, especially in the tests, maybe create a VBoxManage class that provides these results as methods so they can be easily overridden by the tests as well?

This comment has been minimized.

Copy link
Owner Author

@goodwillcoding goodwillcoding replied Jun 21, 2014

I prefer my mocks to never share functionality as they are usually subtle differences in use cases being tested.

This comment has been minimized.

Copy link
Owner Author

@goodwillcoding goodwillcoding replied Jun 21, 2014

As far as I can tell nixops does not have a documented testing strategy for unittests.

@aszlig

This comment has been minimized.

If vboxnet0 is hardcoded, why are we even testing for vboxnet1 here? I'm not sure whether I understand your comment in ensure_control_hostonly_interface, what exactly is the limitation you're speaking about there? According to what I've found on the net, even vboxnet0 seems to be a fixed value anyway, or is it really something that could and/or will be changed?

This comment has been minimized.

Copy link
Owner Author

@goodwillcoding goodwillcoding replied Jun 21, 2014

I should have included it in the commit description but I intend to follow up with a PR with a command line option to pass in a "vboxnetX" interface so it is not a fixed value.

@edolstra
Copy link
Member

@edolstra edolstra commented Jun 24, 2014

Hm, this is a lot of code for what I assume would just be a one-line call to VBoxManage hostonlyif create... I mean, the VirtualBox backend is currently ~1300 lines, and this adds almost a thousand lines just for this feature.

@goodwillcoding
Copy link
Author

@goodwillcoding goodwillcoding commented Jun 24, 2014

@edolstra

It only looks big
Its 104 lines of code, 78 if I remove the "fit with the screen formatting" thats there for readability.

There rest is unittests (in its own file), comments and docstrings,

Just for clarity, the funcitonality is as follows:

  • create a "vboxnet0" interface with a DHCP server if one does not exist, after asking the user
  • if "vboxnet0" exists but does not have a DHCP server, create it and attach it to "vboxnet0" after asking the user
  • if "vboxnet0" exists but does but its DHCP server is disabled, enable it after asking the user
  • if Host-Only interface is not "vboxnet0" then throw some errors. This last part is in prep to make the interface configurable via nix expressions. I talked to @shlevy about it and looking into how it can be done in a coherent manner.

The unittests are larger because of need to monkeypatch some bits to test the functionality in isolation. I am open to suggestions on this, but there were no current unittests for most of this code for me to really model it well

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Feb 10, 2016

@goodwillcoding are you still interested into this change?

@goodwillcoding
Copy link
Author

@goodwillcoding goodwillcoding commented Feb 10, 2016

It's been awhile and I forgot about this. It's definately useful functionality cause I did not have vboxnet0 on my machines. Any concern about merging it?

@grahamc
Copy link
Member

@grahamc grahamc commented Mar 26, 2020

Hello!

Thank you for this PR.

In the past several months, some major changes have taken place in
NixOps:

  1. Backends have been removed, preferring a plugin-based architecture.
    Here are some of them:

  2. NixOps Core has been updated to be Python 3 only, and at the
    same time, MyPy type hints have been added and are now strictly
    required during CI.

This is all accumulating in to what I hope will be a NixOps 2.0
release
. There is a tracking issue for that:
#1242 . It is possible that
more core changes will be made to NixOps for this release, with a
focus on simplifying NixOps core and making it easier to use and work
on.

My hope is that by adding types and more thorough automated testing,
it will be easier for contributors to make improvements, and for
contributions like this one to merge in the future.

However, because of the major changes, it has become likely that this
PR cannot merge right now as it is. The backlog of now-unmergable PRs
makes it hard to see which ones are being kept up to date.

If you would like to see this merge, please bring it up to date with
master and reopen it
. If the or mypy type checking fails, please
correct any issues and then reopen it. I will be looking primarily at
open PRs whose tests are all green.

Thank you again for the work you've done here, I am sorry to be
closing it now.

Graham

@grahamc grahamc closed this Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.