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

VLan support for SR-IOV #708

Merged
merged 8 commits into from
Jul 6, 2018
Merged

VLan support for SR-IOV #708

merged 8 commits into from
Jul 6, 2018

Conversation

jellonek
Copy link
Contributor

@jellonek jellonek commented Jul 2, 2018

This change is Reviewable

@jellonek jellonek force-pushed the jell/vfvlan branch 2 times, most recently from fc46ce1 to e480caf Compare July 3, 2018 14:06
@jellonek jellonek changed the title VLan support for SR-IOV [WIP] VLan support for SR-IOV Jul 3, 2018
@jellonek
Copy link
Contributor Author

jellonek commented Jul 3, 2018

Tests on real hardware (with mellanox NIC) showed that netlink library has still issues even after increasing receive msg buffer so for time of fixing that in proper way on library side i'll provide there a workaround based on invocation of ip link command and parsing its text output.

@jellonek jellonek force-pushed the jell/vfvlan branch 2 times, most recently from 81390bc to 6d2af5b Compare July 4, 2018 14:41
@jellonek jellonek changed the title [WIP] VLan support for SR-IOV VLan support for SR-IOV Jul 4, 2018
Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained


pkg/nettools/nettools.go, line 678 at r2 (raw file):

// setMacAndVlanOnVf uses VF pci address to locate its parent device and uses
// it to set mac address and Vlan id on VF.  It needs to be called from host netns.

VLAN...
from the host netns


pkg/nettools/nettools.go, line 707 at r2 (raw file):

		return 0, err
	}
	iplinkOutput, err := exec.Command("ip", "link", "show", masterLink.Attrs().Name).CombinedOutput()

pls add a comment why ip link show command needs to be used here. It's due to a bug in netlink library related to the netlink message size, right?


pkg/nettools/nettools.go, line 781 at r2 (raw file):

			// Jump into host netns to get vlan id of VF using its
			// master device.

Switch to the host netns to get VLAN ID of the VF using its master device.


pkg/nettools/nettools.go, line 825 at r2 (raw file):

						glog.V(3).Infof("Warning, error during umount of /sys: %v", err)
					}
				}()

This code is repeated. Maybe consider adding function like withHostNetnsAndSysfsRemounted(toCall func() error) ?


pkg/nettools/nettools.go, line 941 at r2 (raw file):

			// devices as was used before reboot, but in meantime there is small chance that they
			// were used already by sriov cni plugin (for which reboot means it's starting everything
			// from clean situation) by other containers, before even virtlet was started

"for some other pods" not "by other containers" maybe?

Copy link
Contributor Author

@jellonek jellonek left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained


pkg/nettools/nettools.go, line 678 at r2 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

VLAN...
from the host netns

Done.


pkg/nettools/nettools.go, line 707 at r2 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

pls add a comment why ip link show command needs to be used here. It's due to a bug in netlink library related to the netlink message size, right?

I had same EINVAL even after increasing message size buffer in netlink, so the real reason is unknown for the moment.
Comment added.


pkg/nettools/nettools.go, line 825 at r2 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

This code is repeated. Maybe consider adding function like withHostNetnsAndSysfsRemounted(toCall func() error) ?

I planed to do that at the beginning of q3, as a part of bigger cleanup in this packager. I plan to extract all related to sriov things to separate file, as this currently contains imo too much spagetti code.
But ok, can add that there now.


pkg/nettools/nettools.go, line 941 at r2 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

"for some other pods" not "by other containers" maybe?

Done.

Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 7 files at r1, 3 of 4 files at r2, 5 of 5 files at r3.
Reviewable status: 0 of 2 LGTMs obtained

Copy link
Contributor

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 LGTMs obtained, and 1 stale

Copy link
Contributor

@pigmej pigmej left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 7 files at r1, 3 of 4 files at r2, 5 of 5 files at r3.
Reviewable status: 1 of 2 LGTMs obtained

Copy link
Contributor

@pigmej pigmej left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 2 LGTMs obtained, and 1 stale

@pigmej pigmej merged commit d2a1c7d into master Jul 6, 2018
@pigmej pigmej deleted the jell/vfvlan branch July 6, 2018 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants