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.

Sign up for GitHub

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 #541 #578

Merged
merged 9 commits into from
Oct 28, 2016
Merged

Fixes #541 #578

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6f44dbf
Merge pull request #1 from contiv/master
srampal Oct 11, 2016
d94c6ad
Fixes #541
srampal Oct 13, 2016
14ed835
Triggering CI
srampal Oct 14, 2016
eb216fe
3rd try retriggering CI
srampal Oct 14, 2016
0a6b2ee
4th try retriggering CI
srampal Oct 14, 2016
e809168
Updates based on PR review comments
srampal Oct 25, 2016
859b19b
re-triggering CI
srampal Oct 25, 2016
1ceb3b4
Merge from main contiv project
srampal Oct 25, 2016
434986e
Final tiny tweaks based on review
srampal Oct 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions netctl/netctl.go
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func inspectPolicy(ctx *cli.Context) {
tenant := ctx.String("tenant")
policy := ctx.Args()[0]

fmt.Printf("Inspeting policy: %s tenant: %s\n", policy, tenant)
fmt.Printf("Inspecting policy: %s tenant: %s\n", policy, tenant)
Copy link
Member

Choose a reason for hiding this comment

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

I think its time to make this a debug log. .

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do that but not sure if someone expects to scrape the CLI output instead of using the API. If yes, removing some lines from CLI output could impact their screen scraping logic … but maybe there aren’t any customers that do that OR we anyway do not support anyone to do CLI screen scraping since they should only be using API for automation. Also, this is not the only place … fmt.Printf() is used throughout this file in places where a debug log may have been appropriate. Should there be a separate issue opened to track which all Printf()s should be changed to logs ?

Copy link
Member

Choose a reason for hiding this comment

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

If possible you can remove the printf's


pol, err := getClient(ctx).PolicyInspect(tenant, policy)
errCheck(ctx, err)
Expand Down Expand Up @@ -471,7 +471,7 @@ func inspectNetwork(ctx *cli.Context) {
tenant := ctx.String("tenant")
network := ctx.Args()[0]

fmt.Printf("Inspeting network: %s tenant: %s\n", network, tenant)
fmt.Printf("Inspecting network: %s tenant: %s\n", network, tenant)

net, err := getClient(ctx).NetworkInspect(tenant, network)
errCheck(ctx, err)
Expand Down Expand Up @@ -661,7 +661,7 @@ func inspectEndpointGroup(ctx *cli.Context) {
tenant := ctx.String("tenant")
endpointGroup := ctx.Args()[0]

fmt.Printf("Inspeting endpointGroup: %s tenant: %s\n", endpointGroup, tenant)
fmt.Printf("Inspecting endpointGroup: %s tenant: %s\n", endpointGroup, tenant)

epg, err := getClient(ctx).EndpointGroupInspect(tenant, endpointGroup)
errCheck(ctx, err)
Expand Down
102 changes: 94 additions & 8 deletions netplugin/agent/docker_event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (
// container start and die event*/
func handleDockerEvents(event *dockerclient.Event, ec chan error, args ...interface{}) {

log.Debugf("Received Docker event: {%#v}\n", *event)
log.Infof("Received Docker event: {%#v}\n", *event)
Copy link
Member

Choose a reason for hiding this comment

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

Please make this debug log.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

endpointUpdReq := &master.UpdateEndpointRequest{}
switch event.Status {
case "start":
Expand All @@ -52,11 +52,12 @@ func handleDockerEvents(event *dockerclient.Event, ec chan error, args ...interf
if event.ID != "" {
labelMap := getLabelsFromContainerInspect(&containerInfo)
containerTenant := getTenantFromContainerInspect(&containerInfo)
networkName, ipAddress, err := getEpNetworkInfoFromContainerInspect(&containerInfo)
networkName, ipAddress, err := getEpNetworkInfoFromContainerInspect(&containerInfo, "")
if err != nil {
log.Errorf("Error getting container network info for %v.Err:%s", event.ID, err)
}
endpoint := getEndpointFromContainerInspect(&containerInfo)
endpoint := getEndpointFromContainerInspect(&containerInfo,
networkName, containerTenant)

if ipAddress != "" {
//Create provider info
Expand Down Expand Up @@ -94,7 +95,69 @@ func handleDockerEvents(event *dockerclient.Event, ec chan error, args ...interf
if err != nil {
log.Errorf("Event:'die' Http error posting endpoint update, Error:%s", err)
}
}

case "":
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it will be right to handle every docker event case. We might have to get to root cause. If we have to start container(samalba) with additional parameters

Copy link
Member Author

Choose a reason for hiding this comment

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

We aren't handling every docker event case. We are specifically looking for events that have nil status + action of "connect". Given that different users may be using different versions of api to create containers, the docker call backs could be formatted differently. With this check, we are now including the call back which we were ignoring when skydns container was created.

So I think we are good here unless you are seeing something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

@srampal: Like I mentioned, event.Action should help here

Copy link
Member Author

Choose a reason for hiding this comment

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

@DivyaVavili yes, I am checking for event.Action in the ..case "".. switch case. But wasn't sure if all existing events can also now move to testing event.action instead of event.status. Since the current code was already tested to work with event.action, its not clear if there are some scenarios that would break if we only started checking event.action instead of event.status. Hence the code currently preserves the logic used for existing scenarios and just adds the case for some of the new events. We can discuss more on slack to be clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

@srampal: You could change the switch case from event.Status to event.Action, which is more specific. And even for the start case, you could fetch the container info from event.Actor.Attributes["container"] instead of event.ID. Having made these changes, both the connect and start events seem to be doing the same thing. I suggest abstracting it out(preferred) or making a fall-through case statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.. Looks like the start event doesn't have event.Actor.Attributes["container"], so event.ID should be good here. Anyway, abstracting this out would be good.

Copy link
Member Author

@srampal srampal Oct 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

there are some differences but yes a lot can be commonized so will do that.


/* Handles the case when we get a docker event with null event status
such as when we proactively create containers ourselves rather than the end user
Look for the "connect" action in such cases.
*/

containerID := event.Actor.Attributes["container"]
networkID := event.Actor.ID

if event.Action == "connect" && containerID != "" {

endpointUpdReq.Event = "connect"
defaultHeaders := map[string]string{"User-Agent": "engine-api-cli-1.0"}
cli, err := client.NewClient("unix:///var/run/docker.sock", "v1.21", nil, defaultHeaders)
if err != nil {
panic(err)
Copy link
Member

Choose a reason for hiding this comment

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

Any Reason to panic if we cant reach the docker client ? I would think logging an error may be the right way to go about it instead of exiting netplugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I duplicated the logic from the "create" case above this was doing (it did a panic() there as well). But will change both to logging an error and exiting instead of panic().

}

containerInfo, err := cli.ContainerInspect(context.Background(), containerID)

if err != nil {
log.Errorf("Container Inspect failed :%s", err)
return
}

labelMap := getLabelsFromContainerInspect(&containerInfo)
containerTenant := getTenantFromContainerInspect(&containerInfo)
networkName, ipAddress, err := getEpNetworkInfoFromContainerInspect(&containerInfo, networkID)
if err != nil {
log.Errorf("Error getting container network info for %v.Err:%s", containerID, err)
}

endpoint := getEndpointFromContainerInspect(&containerInfo,
networkName, containerTenant)

if ipAddress != "" {
//Create provider info
endpointUpdReq.IPAddress = ipAddress
endpointUpdReq.ContainerID = containerID
endpointUpdReq.Tenant = containerTenant
endpointUpdReq.Network = networkName
endpointUpdReq.Event = "start"
endpointUpdReq.EndpointID = endpoint
endpointUpdReq.ContainerName = containerInfo.Name
endpointUpdReq.Labels = make(map[string]string)

for k, v := range labelMap {
endpointUpdReq.Labels[k] = v
}
}

var epUpdResp master.UpdateEndpointResponse

log.Infof("Sending Endpoint update request to master: {%+v}", endpointUpdReq)

err = cluster.MasterPostReq("/plugin/updateEndpoint", endpointUpdReq, &epUpdResp)
if err != nil {
log.Errorf("Event: 'start' , Http error posting endpoint update, Error:%s", err)
}
}
} /*switch() */
}

//getLabelsFromContainerInspect returns the labels associated with the container
Expand All @@ -120,14 +183,21 @@ func getTenantFromContainerInspect(containerInfo *types.ContainerJSON) string {
}

/*getEpNetworkInfoFromContainerInspect inspects the network info from containerinfo returned by dockerclient*/
func getEpNetworkInfoFromContainerInspect(containerInfo *types.ContainerJSON) (string, string, error) {
func getEpNetworkInfoFromContainerInspect(containerInfo *types.ContainerJSON,
networkID string) (string, string, error) {

var networkName string
var IPAddress string
var networkUUID string

networkName = ""
IPAddress = ""

if containerInfo != nil && containerInfo.NetworkSettings != nil {
for _, endpoint := range containerInfo.NetworkSettings.Networks {
IPAddress = endpoint.IPAddress
networkUUID = endpoint.NetworkID

_, network, serviceName, err := dockplugin.GetDockerNetworkName(networkUUID)
if err != nil {
log.Errorf("Error getting docker networkname for network uuid : %s", networkUUID)
Expand All @@ -138,8 +208,13 @@ func getEpNetworkInfoFromContainerInspect(containerInfo *types.ContainerJSON) (s
} else {
networkName = network
}

if networkID != "" && strings.EqualFold(networkID, networkUUID) {
break
}
}
}

return networkName, IPAddress, nil
}

Expand All @@ -155,14 +230,25 @@ func getContainerFromContainerInspect(containerInfo *types.ContainerJSON) string

}

func getEndpointFromContainerInspect(containerInfo *types.ContainerJSON) string {
func getEndpointFromContainerInspect(containerInfo *types.ContainerJSON,
networkName string,
tenantName string,
) string {

endpointID := ""
qualifiedName := ""

if 0 == strings.Compare(tenantName, "default") {
qualifiedName = networkName
} else {
qualifiedName = networkName + "/" + tenantName
}

if containerInfo != nil && containerInfo.NetworkSettings != nil {
for _, endpoint := range containerInfo.NetworkSettings.Networks {
endpoint, ok := containerInfo.NetworkSettings.Networks[qualifiedName]
if ok {
endpointID = endpoint.EndpointID
}
}
return endpointID

}
Empty file modified test/systemtests/docker_test.go
100755 → 100644
View file
Open in desktop
Empty file.
158 changes: 158 additions & 0 deletions test/systemtests/network_test.go
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package systemtests
import (
"fmt"

"errors"
"strings"
"sync"
"time"

Expand Down Expand Up @@ -81,6 +83,36 @@ func (s *systemtestSuite) testInfraNetworkAddDelete(c *C, encap string) {
}
}

/* Confirm network inspect includes a dns endpoint (assumes this is the only endpoint on the network currently) */
func (s *systemtestSuite) testNetworkInspectDNS(tenant, network string) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be move to util_test.go in test/systemtest/

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure ? All other similar functions are in this file (network_test.go)
For example testNetworkAddDelete() and testNetworkAddDeleteTenant() are also in the same file.

Copy link
Member

Choose a reason for hiding this comment

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

I think i am sure :) The way test suite is designed is

  1. Testcase - basically consits of configs usage and calls util functions to help execute/verify/ the case. Test cases are supposed to be in respective test files.
  2. Util functions: like the one you have added need to be in utils. This creates an abstraction on top of the kind of test environment you are running your test case. Your code will break k8s sanity (currently turned off due to a vagrant version issue).
  3. Actual test environment executors: Util functions will call these function depending on the test environment. If its k8s then calls k8s specific executors.If its docker then it calls docker specific executors.

So you will need to move this to util all the way to executors. This will be a stub for k8s. I hope i have clarified the intention.


netInspect, err := s.cli.NetworkInspect(tenant, network)
// Network inspect must succeed
if err != nil {
return false, err
}

// logrus.Infof(" %+v \n", netInspect)

// Network inspect should show exactly 1 endpoint
if len(netInspect.Oper.Endpoints) != 1 {
logrus.Infof("testNetworkInspectDNS has %d endpoints (should have 1)!", len(netInspect.Oper.Endpoints))
return false, errors.New("testNetworkInspectDNS has incorrect # of endpoints (should have 1)!")
}

// First endpoint in network inspect should not have a null container ID
if netInspect.Oper.Endpoints[0].ContainerID == "" {
return false, errors.New("testNetworkInspectDNS endpoint has null containerID!")
}

// First endpoint in network inspect should incude dns in the container name
if !strings.Contains(netInspect.Oper.Endpoints[0].ContainerName, "dns") {
Copy link
Member

Choose a reason for hiding this comment

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

we have a convention to name skydns container. Which is a combination of tenant and skydns. It might be better to check that instead of just "dns"

Copy link
Member Author

Choose a reason for hiding this comment

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

Could easily do that ... however with the current code, its a bit more robust to allow for multiple types of dns containers that we might create in future (not just skydns).

We do this test only when there are no other containers so we know for sure that only a skydns container (or some other dns container). If there is a strong need to check specifically skydns, we could do that else leave it as is.

Copy link
Member

Choose a reason for hiding this comment

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

The reason i am suggesting the change is - its good to verify that the right skydns container is attached to the network/tenant. If we are validating skydns. We might as well validate the right skydns container is attached. We should use the same mechanism to test other dns containers (if and whenever we decide to do that) so that tenant name is embedded in the dns name.

return false, errors.New("testNetworkInspectDNS has no endpoint with dns in containerName!")
Copy link
Member

@abhi abhi Oct 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

might be good add info to error like which network and tenant is this error for. It will be easy to check errors in sanity logs

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

}

return true, nil
}

func (s *systemtestSuite) TestNetworkAddDeleteVXLAN(c *C) {
s.testNetworkAddDelete(c, "vxlan")
}
Expand All @@ -89,6 +121,10 @@ func (s *systemtestSuite) TestNetworkAddDeleteVLAN(c *C) {
s.testNetworkAddDelete(c, "vlan")
}

func (s *systemtestSuite) TestNetworkAddDeleteWithDns(c *C) {
Copy link
Member

Choose a reason for hiding this comment

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

Convention used is TestNetworkAddDeleteWithDnsVlan and TestNetworkAddDeleteWithDnsVxlan. So we have tested both networks

Copy link
Member Author

Choose a reason for hiding this comment

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

This particular issue was just to make sure the skydns container shows up in netctl network inspect. So the encap on the network side doesnt really matter. If we want to create a separate dedicated set of tests for DNS operation itself, that should probably a new issue and there we can test both VLAN and VXLAN mode. So maybe testing separately for VLAN and VXLAN can be skipped here since it is independent of this particular issue.

Copy link
Member

Choose a reason for hiding this comment

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

I would think a good test case captures all possible issues pertaining to the feature. If we have decided to add a case to verify dns attachement to a network. We should verify for all kinds of networks we create. We should be preventing any other issues that we havent found in addition to issues we have found.

s.testNetworkAddDeleteWithDns(c, "vlan")
}

func (s *systemtestSuite) testNetworkAddDelete(c *C, encap string) {

if s.fwdMode == "routing" && encap == "vlan" {
Expand Down Expand Up @@ -190,6 +226,58 @@ func (s *systemtestSuite) testNetworkAddDelete(c *C, encap string) {
}
}

func (s *systemtestSuite) testNetworkAddDeleteWithDns(c *C, encap string) {

var err error

for i := 0; i < s.basicInfo.Iterations; i++ {
var (
netNames = []string{}
)

for networkNum := 0; networkNum < 3; networkNum++ {
var v6subnet, v6gateway string
if networkNum%2 == 0 {
v6subnet = ""
v6gateway = ""
} else {
v6subnet = fmt.Sprintf("1001:%d::/120", networkNum)
v6gateway = fmt.Sprintf("1001:%d::254", networkNum)
}
network := &client.Network{
TenantName: "default",
NetworkName: fmt.Sprintf("net%d-%d", networkNum, i),
Subnet: fmt.Sprintf("10.1.%d.0/24", networkNum),
Gateway: fmt.Sprintf("10.1.%d.254", networkNum),
Ipv6Subnet: v6subnet,
Ipv6Gateway: v6gateway,
PktTag: 1001 + networkNum,
Encap: encap,
}

c.Assert(s.cli.NetworkPost(network), IsNil)
netNames = append(netNames, network.NetworkName)
}

// Wait 5 seconds for the call backs to come through

time.Sleep(5 * time.Second)

for _, netName := range netNames {

/*
Now that the networks are created, check that there is a dns endpoint on each network
*/
_, err = s.testNetworkInspectDNS("default", netName)
c.Assert(err, IsNil)
}

for _, netName := range netNames {
c.Assert(s.cli.NetworkDelete("default", netName), IsNil)
}
}
}

func (s *systemtestSuite) TestNetworkAddDeleteNoGatewayVXLAN(c *C) {
s.testNetworkAddDeleteNoGateway(c, "vxlan")
}
Expand Down Expand Up @@ -279,6 +367,10 @@ func (s *systemtestSuite) TestNetworkAddDeleteTenantVLAN(c *C) {
s.testNetworkAddDeleteTenant(c, "vlan", s.fwdMode)
}

func (s *systemtestSuite) TestNetworkAddDeleteTenantWithDns(c *C) {
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above

Copy link
Member Author

Choose a reason for hiding this comment

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

See above

s.testNetworkAddDeleteTenantWithDns(c, "vlan", s.fwdMode)
}

func (s *systemtestSuite) testNetworkAddDeleteTenant(c *C, encap, fwdmode string) {
mutex := sync.Mutex{}

Expand Down Expand Up @@ -371,6 +463,72 @@ func (s *systemtestSuite) testNetworkAddDeleteTenant(c *C, encap, fwdmode string

}

func (s *systemtestSuite) testNetworkAddDeleteTenantWithDns(c *C, encap, fwdmode string) {

var err error

for i := 0; i < s.basicInfo.Iterations; i++ {
var (
tenantNames = map[string][]string{}
netNames = []string{}
pktTag = 0
)

for tenantNum := 0; tenantNum < 3; tenantNum++ {
tenantName := fmt.Sprintf("tenant%d", tenantNum)
c.Assert(s.cli.TenantPost(&client.Tenant{TenantName: tenantName}), IsNil)
tenantNames[tenantName] = []string{}

for networkNum := 0; networkNum < 3; networkNum++ {
var v6subnet, v6gateway string
if networkNum%2 == 0 {
v6subnet = ""
v6gateway = ""
} else {
v6subnet = fmt.Sprintf("1001:%d:%d::/120", tenantNum, networkNum)
v6gateway = fmt.Sprintf("1001:%d:%d::254", tenantNum, networkNum)
}
network := &client.Network{
TenantName: tenantName,
NetworkName: fmt.Sprintf("net%d-%d", networkNum, i),
Subnet: fmt.Sprintf("10.%d.%d.0/24", tenantNum, networkNum),
Gateway: fmt.Sprintf("10.%d.%d.254", tenantNum, networkNum),
Ipv6Subnet: v6subnet,
Ipv6Gateway: v6gateway,
PktTag: pktTag + 1000,
Encap: encap,
}

logrus.Infof("Creating network %s on tenant %s", network.NetworkName, network.TenantName)

c.Assert(s.cli.NetworkPost(network), IsNil)
netNames = append(netNames, network.NetworkName)
tenantNames[tenantName] = append(tenantNames[tenantName], network.NetworkName)
pktTag++
}
}

// Wait 10 seconds for the call backs to come through

time.Sleep(10 * time.Second)

for tenant, networks := range tenantNames {
for _, network := range networks {
_, err = s.testNetworkInspectDNS(tenant, network)
c.Assert(err, IsNil)
}
}

for tenant, networks := range tenantNames {
for _, network := range networks {
c.Assert(s.cli.NetworkDelete(tenant, network), IsNil)
}
c.Assert(s.cli.TenantDelete(tenant), IsNil)
}
}

}

func (s *systemtestSuite) TestNetworkAddDeleteTenantFwdModeChangeVXLAN(c *C) {
fwdMode := s.fwdMode
for i := 0; i < s.basicInfo.Iterations; i++ {
Expand Down
Loading