Fixes #541 #578
Fixes #541 #578
Conversation
| @@ -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) | |||
There was a problem hiding this comment.
I think its time to make this a debug log. .
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
If possible you can remove the printf's
| @@ -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) | |||
| @@ -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 "": | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@srampal: Like I mentioned, event.Action should help here
There was a problem hiding this comment.
@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.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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().
| @@ -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) { | |||
There was a problem hiding this comment.
This will need to be move to util_test.go in test/systemtest/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think i am sure :) The way test suite is designed is
- 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.
- 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).
- 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.
| } | ||
|
|
||
| // First endpoint in network inspect should incude dns in the container name | ||
| if !strings.Contains(netInspect.Oper.Endpoints[0].ContainerName, "dns") { |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| // First endpoint in network inspect should incude dns in the container name | ||
| if !strings.Contains(netInspect.Oper.Endpoints[0].ContainerName, "dns") { | ||
| return false, errors.New("testNetworkInspectDNS has no endpoint with dns in containerName!") |
There was a problem hiding this comment.
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
| @@ -89,6 +121,10 @@ func (s *systemtestSuite) TestNetworkAddDeleteVLAN(c *C) { | |||
| s.testNetworkAddDelete(c, "vlan") | |||
| } | |||
|
|
|||
| func (s *systemtestSuite) TestNetworkAddDeleteWithDns(c *C) { | |||
There was a problem hiding this comment.
Convention used is TestNetworkAddDeleteWithDnsVlan and TestNetworkAddDeleteWithDnsVxlan. So we have tested both networks
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -279,6 +367,10 @@ func (s *systemtestSuite) TestNetworkAddDeleteTenantVLAN(c *C) { | |||
| s.testNetworkAddDeleteTenant(c, "vlan", s.fwdMode) | |||
| } | |||
|
|
|||
| func (s *systemtestSuite) TestNetworkAddDeleteTenantWithDns(c *C) { | |||
| @@ -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 "": | |||
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
there are some differences but yes a lot can be commonized so will do that.
| @@ -1003,6 +1003,36 @@ func (s *systemtestSuite) verifyIPs(ipaddrs []string) error { | |||
| return err | |||
| } | |||
|
|
|||
| /* 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) { | |||
There was a problem hiding this comment.
very very very minor nit - can we change this to check instead of test.
|
LGTM other than minor comments. Thanks for taking care of the comments. Please squash all your commits into one. |
|
LGTM. Please squash and resubmit. |
| @@ -14,7 +14,7 @@ import ( | |||
| // EPSpec for aci-gw | |||
| type EPSpec struct { | |||
| Tenant string `json:"tenant,omitempty"` | |||
| App string `json:"app,omitempty"` | |||
There was a problem hiding this comment.
@srampal i hope this change was intentional ?
There was a problem hiding this comment.
@abhinandanpb : This is not related to the issue we are seeing here. This was found in my aci-gw feature. As it was online change I asked Sanjeev to incorporate in this PR.
|
This should not have been merged. It failed the test. Who merged it and how? |
|
My bad .. I intended to do squash-commit and apparently ended up doing squash-commit-merge or something like that … I had already tested my changes several times. The additional 1-line change was from Gaurav. Rgds, From: jojimt <[email protected]mailto:[email protected]> This should not have been merged. It failed the test. Who merged it and how? — |
|
@srampal the top of the tree is broken with compile error. Can you please take care of it ? |
No description provided.