[IPVS] Implement GetServices(),GetService() and GetDestinations() #1770

dhilipkumars · 2017-05-19T20:23:58Z

@mavenugo This is an Inital checkin to get the approach validated. If you are okay with this i will improve/add coverage for the package.

Also i though we could introduce a file utils.go in ipvs package and moave all the utility related funcs there? what do you think?

GordonTheTurtle · 2017-05-19T20:24:00Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "ipvs-getServices-dev" git@github.com:dhilipkumars/libnetwork.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: dhilipkumars <dhilip.kumar.s@huawei.com>

dhilipkumars · 2017-05-20T18:02:45Z

Hi @mavenugo
This PR has the following

Implements GetServices(), GetDestinations(*Services) and GetService(*Service)
Has tests for all the new code written (the coverage is maintained 80%)
Uses the newer APIs in the UT and hence removes the dependency from ipvsadm command.

…ependency from ipvsadm Signed-off-by: dhilipkumars <dhilip.kumar.s@huawei.com>

fcrisciani · 2017-05-23T16:11:29Z

ipvs/ipvs.go

+	return i.doGetDestinationsCmd(s, nil)
+}
+
+//GetService gets details of a specific IPVS services, useful in updating statisics etc.,


all the comments will start with a space

fcrisciani · 2017-05-23T16:11:37Z

ipvs/ipvs.go

+		return nil, err
+	}
+
+	//We are looking for exactly one service otherwise error out


fcrisciani · 2017-05-23T16:11:49Z

ipvs/netlink.go

@@ -19,6 +19,8 @@ import (
 	"github.com/vishvananda/netns"
 )

+//For Quick Reference IPVS related netlink message is described at the end of this file.


same here, remove extra line below

fcrisciani · 2017-05-23T16:12:04Z

ipvs/netlink.go

@@ -72,6 +74,7 @@ func setup() {

 func fillService(s *Service) nl.NetlinkRequestData {
 	cmdAttr := nl.NewRtAttr(ipvsCmdAttrService, nil)
+


I saw around other extra spaces, are they needed?

fcrisciani · 2017-05-23T17:38:06Z

ipvs/netlink.go

+	case syscall.AF_INET6:
+		resIP = (net.IP)(ip[:16])
+	default:
+		return resIP, fmt.Errorf("parseIP Error ip=%v", ip)


nil maybe is better?

fcrisciani · 2017-05-23T17:41:45Z

ipvs/netlink.go

+		return nil, err
+	}
+	if len(NetLinkAttrs) == 0 {
+		return nil, fmt.Errorf("Error No valid net link message found while Parsing service record")


spelling: netlink is with no space.
We are usually also starting the error messages with the lowercase so in this case:
fmt.Errorf("error no valid netlink message found while Parsing service record")

fcrisciani · 2017-05-23T17:43:05Z

ipvs/netlink.go

+}
+
+//IPVS related netlink message format explained
+


Nice section below, even though this is the generic netlink data structure (looks like ipvs does not have a custom netlink message structure), it will be useful to have it here for other developers

yes if we could keep them it would be helpful for future developers, i spent a nice few hours to figure it out (as im new to netlink format)

dhilipkumars · 2017-05-23T19:17:26Z

Hi @fcrisciani , all the comments have been addressed PTAL. Please let us know if you have more comments. we will fix it ASAP.

Thanks in Advance, we would love to help to get this PR merged ASAP.

dhilipkumars · 2017-05-23T19:17:44Z

BTW, Thanks for the quick review

fcrisciani · 2017-05-23T20:29:12Z

ipvs/netlink.go

+		return nil, err
+	}
+	if len(NetLinkAttrs) == 0 {
+		return nil, fmt.Errorf("Error No valid netlink message found while Parsing service record")


fmt.Errorf("error no valid netlink message found while Parsing service record")

fcrisciani · 2017-05-23T20:29:46Z

just posted another 2 minor comments, rest looks good to me

dhilipkumars · 2017-05-24T01:40:46Z

@fcrisciani Thanks again PTAL.

Signed-off-by: dhilipkumars <dhilip.kumar.s@huawei.com>

fcrisciani · 2017-05-24T23:01:24Z

LGTM

abhi · 2017-05-25T03:43:20Z

ipvs/netlink.go

+	for _, msg := range msgs {
+		srv, err := i.parseService(msg)
+		if err != nil {
+			return res, err


should this be return nil, err

abhi · 2017-05-25T03:43:27Z

ipvs/netlink.go

@@ -217,6 +235,7 @@ done:
 			if m.Header.Type == syscall.NLMSG_DONE {
 				break done
 			}
+


white space

abhi · 2017-05-25T03:43:29Z

ipvs/netlink.go

@@ -208,6 +225,7 @@ done:
 			return nil, err
 		}
 		for _, m := range msgs {
+


white space

abhi · 2017-05-25T03:43:39Z

ipvs/ipvs_test.go


-			return
+		if svc.Protocol == s.Protocol && svc.Address.String() == s.Address.String() && svc.Port == s.Port {
+			svcFound = true


can't we just error/return here instead of additional switch statement below ?

The tests sometimes expect the service to be present and sometimes expect the service to be absent. We error out based on that expectation. I dont think we can take that decision inside the loop? we have to loop all the elements to figure out the service is not available.

it just that we could have avoided svcFound flag.

If the service was found { if !checkPresent{ //error } //implicit else return } //outside the loop if checkPresent{ //error }

abhi · 2017-05-25T03:43:42Z

ipvs/ipvs_test.go

-			assert.Equal(t, protocol, parts[0])
-			assert.Equal(t, serviceAddress, parts[2])
-			assert.Equal(t, schedMethod, parts[3])
+	svcFound := false


better convention would be var svcFound bool

abhi · 2017-05-25T03:43:45Z

ipvs/ipvs_test.go


-			realServers = append(realServers, o)
+	for _, dst := range dstArray {
+		if dst.Address.String() == d.Address.String() && dst.Port == d.Port && lookupFwMethod(dst.ConnectionFlags) == lookupFwMethod(d.ConnectionFlags) {


we can just use net.Equal(dst,d)

abhi · 2017-05-25T03:43:48Z

ipvs/ipvs_test.go

-			continue
-		}
+func checkDestination(t *testing.T, i *Handle, s *Service, d *Destination, checkPresent bool) {
+	dstFound := false


var dstFound bool

Signed-off-by: dhilipkumars <dhilip.kumar.s@huawei.com>

dhilipkumars · 2017-05-25T05:55:58Z

@abhinandanpb PTAL.

mavenugo · 2017-05-25T15:41:27Z

Thanks @dhilipkumars. LGTM.

dhilipkumars · 2017-05-25T15:48:35Z

Thanks for the quik merge

abhi · 2017-05-25T15:59:11Z

@dhilipkumars please squash your review fix commits from next time.

dhilipkumars · 2017-05-25T16:22:37Z

Certainly. Thanks for being considerate this time.

GordonTheTurtle added the dco/no label May 19, 2017

dhilipkumars force-pushed the ipvs-getServices-dev branch from 1e5f110 to a82c3f4 Compare May 19, 2017 20:29

GordonTheTurtle removed the dco/no label May 19, 2017

Initial Checkin GetServices and GetDestinations

a82c3f4

Signed-off-by: dhilipkumars <dhilip.kumar.s@huawei.com>

dhilipkumars mentioned this pull request May 20, 2017

IPVS in libnetwork query interfaces for services and destnations #1761

Closed

dhilipkumars changed the title ~~[IPVS] Initial Checkin GetServices and GetDestinations~~ [IPVS] Implement GetServices(),GetService() and GetDestinations() May 21, 2017

This was referenced May 22, 2017

[WIP]implement first version of kube-proxy via ipvs kubernetes/kubernetes#30134

Closed

Implement IPVS-based in-cluster service load balancing kubernetes/kubernetes#44063

Closed

Update GetService(*Service) api. Update UT with new apis and remove d…

ce0066b

…ependency from ipvsadm Signed-off-by: dhilipkumars <dhilip.kumar.s@huawei.com>

dhilipkumars force-pushed the ipvs-getServices-dev branch from 246b5dd to ce0066b Compare May 23, 2017 13:17

fcrisciani reviewed May 23, 2017

View reviewed changes

Fix typos and review comments

635b898

Signed-off-by: dhilipkumars <dhilip.kumar.s@huawei.com>

dhilipkumars force-pushed the ipvs-getServices-dev branch from 7da9af5 to 635b898 Compare May 24, 2017 02:27

abhi reviewed May 25, 2017

View reviewed changes

ipvs/netlink.go Outdated

@@ -217,6 +235,7 @@ done:

if m.Header.Type == syscall.NLMSG_DONE {

break done

}

Copy link

Contributor

abhi May 25, 2017

Choose a reason for hiding this comment

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

white space

abhi reviewed May 25, 2017

View reviewed changes

ipvs/netlink.go Outdated

@@ -208,6 +225,7 @@ done:

return nil, err

}

for _, m := range msgs {

Copy link

Contributor

abhi May 25, 2017

Choose a reason for hiding this comment

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

white space

abhi reviewed May 25, 2017

View reviewed changes

fix review comments

104e2f9

Signed-off-by: dhilipkumars <dhilip.kumar.s@huawei.com>

mavenugo merged commit 6a0dfa1 into moby:master May 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IPVS] Implement GetServices(),GetService() and GetDestinations() #1770

[IPVS] Implement GetServices(),GetService() and GetDestinations() #1770

dhilipkumars commented May 19, 2017

GordonTheTurtle commented May 19, 2017

dhilipkumars commented May 20, 2017 •

edited

fcrisciani May 23, 2017

fcrisciani May 23, 2017

fcrisciani May 23, 2017

fcrisciani May 23, 2017

fcrisciani May 23, 2017

fcrisciani May 23, 2017

fcrisciani May 23, 2017

fcrisciani May 23, 2017

dhilipkumars May 23, 2017

dhilipkumars commented May 23, 2017

dhilipkumars commented May 23, 2017

fcrisciani May 23, 2017

fcrisciani commented May 23, 2017

dhilipkumars commented May 24, 2017

fcrisciani commented May 24, 2017

abhi May 25, 2017

abhi May 25, 2017

abhi May 25, 2017

abhi May 25, 2017

dhilipkumars May 25, 2017

abhi May 25, 2017 •

edited

abhi May 25, 2017

abhi May 25, 2017

abhi May 25, 2017

dhilipkumars commented May 25, 2017

mavenugo commented May 25, 2017

dhilipkumars commented May 25, 2017

abhi commented May 25, 2017

dhilipkumars commented May 25, 2017

		@@ -72,6 +74,7 @@ func setup() {

		func fillService(s *Service) nl.NetlinkRequestData {
		cmdAttr := nl.NewRtAttr(ipvsCmdAttrService, nil)

[IPVS] Implement GetServices(),GetService() and GetDestinations() #1770

[IPVS] Implement GetServices(),GetService() and GetDestinations() #1770

Conversation

dhilipkumars commented May 19, 2017

GordonTheTurtle commented May 19, 2017

dhilipkumars commented May 20, 2017 • edited

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

dhilipkumars commented May 23, 2017

dhilipkumars commented May 23, 2017

Choose a reason for hiding this comment

fcrisciani commented May 23, 2017

dhilipkumars commented May 24, 2017

fcrisciani commented May 24, 2017

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

abhi May 25, 2017 • edited

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

dhilipkumars commented May 25, 2017

mavenugo commented May 25, 2017

dhilipkumars commented May 25, 2017

abhi commented May 25, 2017

dhilipkumars commented May 25, 2017

dhilipkumars commented May 20, 2017 •

edited

abhi May 25, 2017 •

edited