-
Notifications
You must be signed in to change notification settings - Fork 180
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
Epif cleanup #16
Epif cleanup #16
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,8 +85,6 @@ type EndpointDriver interface { | |
CreateEndpoint(id string) error | ||
DeleteEndpoint(value string) error | ||
MakeEndpointAddress() (*Address, error) | ||
GetEndpointContainerContext(id string) (*ContainerEpContext, error) | ||
GetContainerEpContextByContName(contName string) ([]ContainerEpContext, error) | ||
UpdateContainerId(id string, contId string) error | ||
} | ||
|
||
|
@@ -118,7 +116,7 @@ type ContainerEpContext struct { | |
DefaultGw string | ||
} | ||
|
||
type ContainerDriver interface { | ||
type ContainerIf interface { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest to move ContainerIf interface out of core as well (may be move it to crt.go?), so that core just contains the APIs the external code either uses (i.e. Plugin interface) or implements (Driver interface(s)). Also suggest to add some doc-comment strings explaining the use of this interface, as in how is it called; how will it differ between runtimes etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one was interesting. I did have it outside in crt/crt.go but circular dependency between crt/docker and crt made me move this to core. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Thinking more, crt is different from docker dir contents in a way that crt defines something like Plugin that makes use of implementation of a containerIf. May be we should add a separate directory for container-clients and move the ContainerIf (to a file like containerc/containerc.go and docker directory there (i.e. container/docker). Since the interface is specific to container-clients we can also name is ContainerC or ContainerClient instead of ContainerIf. Also while doing this may be we should also move our current implementation of netd under docker/directory as well, since the netplugin binary is docker specific. We can name the directory docker-net or something in that case. |
||
// Container driver provides a mechanism to interface with container | ||
// runtime to handle events create, start, die, stop, pause, etc. | ||
Driver | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
/*** | ||
Copyright 2014 Cisco Systems Inc. All rights reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package crt | ||
|
||
import ( | ||
"encoding/json" | ||
"errors" | ||
"reflect" | ||
|
||
"github.com/contiv/netplugin/core" | ||
"github.com/contiv/netplugin/crt/docker" | ||
) | ||
|
||
type Crt struct { | ||
ContainerIf core.ContainerIf | ||
} | ||
|
||
type CrtConfig struct { | ||
Crt struct { | ||
Type string | ||
} | ||
} | ||
|
||
type ContainerIfTypes struct { | ||
CrtType reflect.Type | ||
CrtConfigType reflect.Type | ||
} | ||
|
||
var ContainerIfRegistry = map[string]ContainerIfTypes{ | ||
"docker": ContainerIfTypes{ | ||
CrtType: reflect.TypeOf(docker.Docker{}), | ||
CrtConfigType: reflect.TypeOf(docker.DockerConfig{}), | ||
}, | ||
} | ||
|
||
type ContainerEpContext struct { | ||
NewContName string | ||
CurrContName string | ||
InterfaceId string | ||
IpAddress string | ||
SubnetLen uint | ||
DefaultGw string | ||
} | ||
|
||
func (c *Crt) AttachEndpoint(contEpContext *core.ContainerEpContext) error { | ||
return c.ContainerIf.AttachEndpoint(contEpContext) | ||
} | ||
|
||
func (c *Crt) DetachEndpoint(contEpContext *core.ContainerEpContext) error { | ||
return c.ContainerIf.DetachEndpoint(contEpContext) | ||
} | ||
|
||
func (c *Crt) GetContainerId(contName string) string { | ||
return c.ContainerIf.GetContainerId(contName) | ||
} | ||
|
||
func (c *Crt) GetContainerName(contId string) (string, error) { | ||
return c.ContainerIf.GetContainerName(contId) | ||
} | ||
|
||
func (c *Crt) Deinit() { | ||
c.ContainerIf.Deinit() | ||
} | ||
|
||
func (c *Crt) Init(configStr string) error { | ||
|
||
cfg := &CrtConfig{} | ||
err := json.Unmarshal([]byte(configStr), cfg) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if _, ok := ContainerIfRegistry[cfg.Crt.Type]; !ok { | ||
return errors.New("unregistered container run time") | ||
} | ||
|
||
crtConfigType := ContainerIfRegistry[cfg.Crt.Type].CrtConfigType | ||
crtConfig := reflect.New(crtConfigType).Interface() | ||
err = json.Unmarshal([]byte(configStr), crtConfig) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
config := &core.Config{V: crtConfig} | ||
crtType := ContainerIfRegistry[cfg.Crt.Type].CrtType | ||
crtif := reflect.New(crtType).Interface() | ||
c.ContainerIf = crtif.(core.ContainerIf) | ||
err = c.ContainerIf.Init(config) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
/*** | ||
Copyright 2014 Cisco Systems Inc. All rights reserved. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package crt | ||
|
||
import ( | ||
"testing" | ||
) | ||
|
||
func TestCrtInit(t *testing.T) { | ||
configStr := `{ | ||
"docker" : { | ||
"socket" : "unix:///var/run/docker.sock" | ||
}, | ||
"crt" : { | ||
"type": "docker" | ||
} | ||
}` | ||
crt := Crt{} | ||
err := crt.Init(configStr) | ||
if err != nil { | ||
t.Fatalf("crt init failed: Error: %s", err) | ||
} | ||
defer func() { crt.Deinit() }() | ||
} | ||
|
||
func TestCrtInitInvalidConfigMissingCrt(t *testing.T) { | ||
configStr := `{ | ||
"docker" : { | ||
"socket" : "unix:///var/run/docker.sock" | ||
} | ||
}` | ||
crt := Crt{} | ||
err := crt.Init(configStr) | ||
if err == nil { | ||
t.Fatalf("crt init succeeded!") | ||
} | ||
} | ||
|
||
func TestCrtInitInvalidConfigMissingCrtIf(t *testing.T) { | ||
configStr := `{ | ||
"crt" : { | ||
"type": "docker" | ||
} | ||
}` | ||
crt := Crt{} | ||
err := crt.Init(configStr) | ||
if err == nil { | ||
t.Fatalf("crt init succeeded!") | ||
} | ||
} | ||
|
||
func TestCrtInitInvalidConfigInvalidCrt(t *testing.T) { | ||
configStr := `{ | ||
"crt" : { | ||
"type": "rocket" | ||
} | ||
}` | ||
crt := Crt{} | ||
err := crt.Init(configStr) | ||
if err == nil { | ||
t.Fatalf("crt init succeeded!") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to move the UpdateContainerId() out as well? Please ignore, if you were planning it for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update ContainerID is just a name of the function, there is nothing specific to container runtime there. I was hoping to not do updates to Ep record from oustide drivers code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, it does imply that Endpoint driver Interface has something to do with containers, which is not the case.
Looks like this API updates the container-id in the EP oper-state in current implementation, should we just do it as part of attach/detach endpoint in the crt's logic? this looks more like some config meta for container runtime than network programming driver.