-
Notifications
You must be signed in to change notification settings - Fork 181
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
netmaster cli refactor #1088
netmaster cli refactor #1088
Changes from all 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 |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package core | ||
|
||
// Plugin modes | ||
const ( | ||
Docker = "docker" | ||
Kubernetes = "kubernetes" | ||
SwarmMode = "swarm-mode" | ||
Test = "test" | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ import ( | |
"github.com/contiv/netplugin/netmaster/resources" | ||
"github.com/contiv/netplugin/objdb" | ||
"github.com/contiv/netplugin/utils" | ||
"github.com/contiv/netplugin/utils/netutils" | ||
"github.com/contiv/ofnet" | ||
"github.com/gorilla/mux" | ||
|
||
|
@@ -46,10 +47,14 @@ const leaderLockTTL = 30 | |
// MasterDaemon runs the daemon FSM | ||
type MasterDaemon struct { | ||
// Public state | ||
ListenURL string // URL where netmaster listens for ext requests | ||
ControlURL string // URL where netmaster listens for ctrl pkts | ||
ClusterStore string // state store URL | ||
ClusterMode string // cluster scheduler used docker/kubernetes/mesos etc | ||
ListenURL string // URL where netmaster listens for ext requests | ||
ControlURL string // URL where netmaster listens for ctrl pkts | ||
ClusterStoreDriver string // state store driver name | ||
ClusterStoreURL string // state store endpoint | ||
ClusterMode string // cluster scheduler used docker/kubernetes/mesos etc | ||
NetworkMode string // network mode (vlan or vxlan) | ||
NetForwardMode string // forwarding mode (bridge or routing) | ||
NetInfraType string // infra type (aci or default) | ||
|
||
// Private state | ||
currState string // Current state of the daemon | ||
|
@@ -70,13 +75,13 @@ func (d *MasterDaemon) Init() { | |
// set cluster mode | ||
err := master.SetClusterMode(d.ClusterMode) | ||
if err != nil { | ||
log.Fatalf("Failed to set cluster-mode. Error: %s", err) | ||
log.Fatalf("Failed to set cluster-mode %q. Error: %s", d.ClusterMode, err) | ||
} | ||
|
||
// initialize state driver | ||
d.stateDriver, err = initStateDriver(d.ClusterStore) | ||
d.stateDriver, err = utils.NewStateDriver(d.ClusterStoreDriver, &core.InstanceInfo{DbURL: d.ClusterStoreURL}) | ||
if err != nil { | ||
log.Fatalf("Failed to init state-store. Error: %s", err) | ||
log.Fatalf("Failed to init state-store: driver %q, URLs %q. Error: %s", d.ClusterStoreDriver, d.ClusterStoreURL, err) | ||
} | ||
|
||
// Initialize resource manager | ||
|
@@ -86,9 +91,9 @@ func (d *MasterDaemon) Init() { | |
} | ||
|
||
// Create an objdb client | ||
d.objdbClient, err = objdb.NewClient(d.ClusterStore) | ||
d.objdbClient, err = objdb.InitClient(d.ClusterStoreDriver, []string{d.ClusterStoreURL}) | ||
if err != nil { | ||
log.Fatalf("Error connecting to state store: %v. Err: %v", d.ClusterStore, err) | ||
log.Fatalf("Error connecting to state store: driver %q, URLs %q. Err: %v", d.ClusterStoreDriver, d.ClusterStoreURL, err) | ||
} | ||
} | ||
|
||
|
@@ -404,7 +409,11 @@ func (d *MasterDaemon) runLeader() { | |
router := mux.NewRouter() | ||
|
||
// Create a new api controller | ||
d.apiController = objApi.NewAPIController(router, d.objdbClient, d.ClusterStore) | ||
apiConfig := &objApi.APIControllerConfig{ | ||
NetForwardMode: d.NetForwardMode, | ||
NetInfraType: d.NetInfraType, | ||
} | ||
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. AFACT this isn't supposed to be modifiable, why not have NewAPIController accept APIControllerConfig by value? 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 object only got used here (and integ testings), I used object was because of intending to add more parameters from configs .
this is troublesome in golang, especially when having the parameters in different type 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 think the struct is fine, just dunno why you need a pointer to it |
||
d.apiController = objApi.NewAPIController(router, d.objdbClient, apiConfig) | ||
|
||
//Restore state from clusterStore | ||
d.restoreCache() | ||
|
@@ -447,6 +456,7 @@ func (d *MasterDaemon) startListeners(router *mux.Router, stopChan chan bool) { | |
server := &http.Server{Handler: router} | ||
server.SetKeepAlivesEnabled(false) | ||
|
||
// bind on external address | ||
listener, err := net.Listen("tcp", d.ListenURL) | ||
if nil != err { | ||
log.Fatalln(err) | ||
|
@@ -457,20 +467,25 @@ func (d *MasterDaemon) startListeners(router *mux.Router, stopChan chan bool) { | |
|
||
go server.Serve(listener) | ||
|
||
listenURL := strings.Split(d.ListenURL, ":") | ||
controlURL := strings.Split(d.ControlURL, ":") | ||
if d.ControlURL != d.ListenURL { | ||
externalAddr := strings.Split(d.ListenURL, ":") | ||
internalAddr := strings.Split(d.ControlURL, ":") | ||
if externalAddr[0] == "0.0.0.0" && externalAddr[1] == internalAddr[1] { | ||
// ignore internal bind if external and internal are on the same port and external bind on 0.0.0.0 | ||
log.Infof("Ignore creating API listener on %q because %q covers it", d.ControlURL, d.ListenURL) | ||
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. seems we could have external and internal on same IP as well? 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. if they are on the same ip but different port will go to 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. oh nvm, I wasn't looking at the outer if restriction |
||
} else { | ||
// it should fail-fast if ControlURL and ListenURL have other overlapping | ||
ctrlListener, err := net.Listen("tcp", d.ControlURL) | ||
if nil != err { | ||
log.Fatalln(err) | ||
} | ||
log.Infof("Netmaster listening on %s for control packets", d.ControlURL) | ||
ctrlListener = utils.ListenWrapper(ctrlListener) | ||
defer ctrlListener.Close() | ||
|
||
if (strings.Compare(listenURL[1], controlURL[1]) != 0) || (len(listenURL[0]) != 0 && strings.Compare(listenURL[0], "0.0.0.0") != 0 && strings.Compare(listenURL[0], controlURL[0]) != 0) { | ||
ctrlListener, err := net.Listen("tcp", d.ControlURL) | ||
if nil != err { | ||
log.Fatalln(err) | ||
// start server | ||
go server.Serve(ctrlListener) | ||
} | ||
log.Infof("Netmaster listening on %s for control packets", d.ControlURL) | ||
ctrlListener = utils.ListenWrapper(ctrlListener) | ||
defer ctrlListener.Close() | ||
|
||
// start server | ||
go server.Serve(ctrlListener) | ||
} | ||
|
||
// Wait till we are asked to stop | ||
|
@@ -584,7 +599,7 @@ func (d *MasterDaemon) getMasterInfo() (map[string]interface{}, error) { | |
info := make(map[string]interface{}) | ||
|
||
// get local ip | ||
localIP, err := GetLocalAddr() | ||
localIP, err := netutils.GetDefaultAddr() | ||
if err != nil { | ||
return nil, errors.New("error getting local IP address") | ||
} | ||
|
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.
i think we should check for ':' in ControlURL and ListenURL since those are parsed later in startListeners
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.
it actually got checked in the CLI part
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.
ah I see it now