Refactor: Use the `netlink` library instead of exec'ing out to `ip` #279

alexeldeib · 2019-08-06T06:52:06Z

Fixes #85

alexeldeib · 2019-08-06T07:47:35Z

docs/cli/ignite/ignite_create.md

@@ -14,8 +14,9 @@ the flags for this command.

 If the name flag (-n, --name) is not specified,
 the VM is given a random name. Using the copy files
-flag (-f, --copy-files), additional files can be added to
-the VM during creation with the syntax /host/path:/vm/path.
+flag (-f, --copy-files), additional files/directories


make tidy added this?

Yes, that's fine to include here, just forgot to update the docs in an earlier PR where I changed this 😅

twelho

Great work @alexeldeib! 🎉
I added some minor design changes that will simplify the code and prevent excess lookups by interface names. After those have been addressed, looks like we're ready to merge!

twelho · 2019-08-06T11:16:18Z

docs/cli/ignite/ignite_create.md

@@ -14,8 +14,9 @@ the flags for this command.

 If the name flag (-n, --name) is not specified,
 the VM is given a random name. Using the copy files
-flag (-f, --copy-files), additional files can be added to
-the VM during creation with the syntax /host/path:/vm/path.
+flag (-f, --copy-files), additional files/directories


Yes, that's fine to include here, just forgot to update the docs in an earlier PR where I changed this 😅

twelho · 2019-08-06T11:27:44Z

pkg/container/network.go

 		return err
 	}
-
-	return setLinkUp(tapName)
+	return setLinkUp(handle, tapName)


We should likely call handle.LinkSetUp(tuntap) here and get rid of setLinkUp entirely

twelho · 2019-08-06T11:28:36Z

pkg/container/network.go

 		return err
 	}
-
-	return setLinkUp(bridgeName)
+	return handle.LinkSetUp(bridge)


Just like this, we don't need setLinkUp anymore

twelho · 2019-08-06T11:29:08Z

pkg/container/network.go

-func setLinkUp(adapterName string) error {
-	_, err := util.ExecuteCommand("ip", "link", "set", adapterName, "up")
-	return err
+func setLinkUp(handle *netlink.Handle, adapterName string) error {


Can be removed in favor of handle.LinkSetUp

twelho · 2019-08-06T11:30:35Z

pkg/container/network.go

+	la.Name = bridgeName
+	bridge := &netlink.Bridge{LinkAttrs: la}
+	err := netlink.LinkAdd(bridge)
+	if err != nil {


Concatenate into if err := netlink.LinkAdd(bridge); err != nil {

twelho · 2019-08-06T11:36:16Z

pkg/container/network.go

 		return nil, err
 	}

-	if err := createBridge(bridgeName); err != nil {
+	if err := createBridge(handle, bridgeName); err != nil {


Return the *netlink.Bridge from createBridge

twelho · 2019-08-06T11:36:44Z

pkg/container/network.go

+		return nil, err
+	}
+
+	if err := createTAPAdapter(handle, tapName); err != nil {


Return the *netlink.Tuntap from createTAPAdapter

twelho · 2019-08-06T11:40:15Z

pkg/container/network.go

 		return nil, err
 	}

-	if err := connectAdapterToBridge(tapName, bridgeName); err != nil {
+	if err := connectAdapterToBridge(handle, tapName, bridgeName); err != nil {


Using the Bridge and Tuntap devices returned above, call handle.LinkSetMaster here instead of connectAdapterToBridge

twelho · 2019-08-06T11:42:18Z

pkg/container/network.go

 		return nil, err
 	}

-	if err := connectAdapterToBridge(iface.Name, bridgeName); err != nil {
+	if err := connectAdapterToBridge(handle, iface.Name, bridgeName); err != nil {


Same as above, use netlink.LinkByName(iface.Name) to retrieve the device for handle.LinkSetMaster

twelho · 2019-08-06T11:43:58Z

pkg/container/network.go

-func connectAdapterToBridge(adapterName, bridgeName string) error {
-	_, err := util.ExecuteCommand("ip", "link", "set", adapterName, "master", bridgeName)
-	return err
+func connectAdapterToBridge(handle *netlink.Handle, adapterName, bridgeName string) error {


Can be removed in favor of calling handle.LinkSetMaster directly

luxas

Thanks very much for this contribution @alexeldeib 🎉!
Very appreciated. Sorry that the master branch had some outstanding issues
with the doc generation, something I just fixed at HEAD.
When you rebase those unnecessary changes should go away.

alexeldeib · 2019-08-06T20:20:32Z

Hmm...something in this PR breaks ignite ssh <my-vm>. Debugging.

alexeldeib · 2019-08-06T21:45:51Z

oh, several things broke it! 😄 Should be better now.

alexeldeib · 2019-08-06T21:55:09Z

@luxas glad I can lend a hand 😃 Thank you and @twelho for this project! Firecracker is very cool but the UX is not great. Ignite removes a lot of that pain. Being able to see the image + vm + networking setup in this project has been super helpful for my own learning process.

twelho

Thanks for the changes @alexeldeib!
LGTM 👍

alexeldeib commented Aug 6, 2019

View reviewed changes

alexeldeib force-pushed the ace/netlink branch from e79e4c3 to 99731dd Compare August 6, 2019 07:50

twelho reviewed Aug 6, 2019

View reviewed changes

luxas reviewed Aug 6, 2019

View reviewed changes

luxas assigned twelho Aug 6, 2019

luxas added this to the v0.5.0 milestone Aug 6, 2019

luxas added the kind/enhancement Categorizes issue or PR as related to improving an existing feature. label Aug 6, 2019

alexeldeib force-pushed the ace/netlink branch 2 times, most recently from a9cb096 to cbd2183 Compare August 6, 2019 20:20

alexeldeib force-pushed the ace/netlink branch from cbd2183 to c59fe94 Compare August 6, 2019 20:42

refactor: use netlink instead of exec'ing

69d33ff

alexeldeib force-pushed the ace/netlink branch from c59fe94 to 69d33ff Compare August 6, 2019 21:46

twelho approved these changes Aug 7, 2019

View reviewed changes

twelho merged commit 9a3f845 into weaveworks:master Aug 7, 2019

twelho mentioned this pull request Aug 7, 2019

Clean up ignite-spawn networking, don't depend on errors package #285

Merged

luxas changed the title ~~refactor: use netlink instead of exec'ing~~ Refactor: Use the netlink library instead of exec'ing out to ip Aug 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Use the `netlink` library instead of exec'ing out to `ip` #279

Refactor: Use the `netlink` library instead of exec'ing out to `ip` #279

alexeldeib commented Aug 6, 2019

alexeldeib Aug 6, 2019

twelho Aug 6, 2019

twelho left a comment

twelho Aug 6, 2019

twelho Aug 6, 2019

twelho Aug 6, 2019

twelho Aug 6, 2019

twelho Aug 6, 2019

twelho Aug 6, 2019

twelho Aug 6, 2019

twelho Aug 6, 2019

twelho Aug 6, 2019

twelho Aug 6, 2019

luxas left a comment

alexeldeib commented Aug 6, 2019

alexeldeib commented Aug 6, 2019 •

edited

Loading

alexeldeib commented Aug 6, 2019

twelho left a comment

Refactor: Use the netlink library instead of exec'ing out to ip #279

Refactor: Use the netlink library instead of exec'ing out to ip #279

Conversation

alexeldeib commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twelho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

alexeldeib commented Aug 6, 2019

alexeldeib commented Aug 6, 2019 • edited Loading

alexeldeib commented Aug 6, 2019

twelho left a comment

Choose a reason for hiding this comment

Refactor: Use the `netlink` library instead of exec'ing out to `ip` #279

Refactor: Use the `netlink` library instead of exec'ing out to `ip` #279

alexeldeib commented Aug 6, 2019 •

edited

Loading