diff options
author | IWASE Yusuke <iwase.yusuke0@gmail.com> | 2018-06-26 22:03:31 +0900 |
---|---|---|
committer | FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> | 2018-06-27 20:31:12 +0900 |
commit | 29d0e59af48c843b686f6a77c3cb7f34fc59568d (patch) | |
tree | 8aa580579050e5c1c1a3e62b86b04238d0bc4935 | |
parent | 72bbb9678381686acf14b0177dbf442a6761f41b (diff) |
server: Avoid calling os.Exit() in BgpServer.Shutdown()
In case that GoBGP is used as Go native library, calling os.Exit() can
cause unexpected termination including application which uses GoBGP.
This patch removes calls of os.Exit() derived from BgpServer.Shutdown().
Signed-off-by: IWASE Yusuke <iwase.yusuke0@gmail.com>
-rw-r--r-- | gobgpd/main.go | 463 | ||||
-rw-r--r-- | server/server.go | 28 |
2 files changed, 252 insertions, 239 deletions
diff --git a/gobgpd/main.go b/gobgpd/main.go index 13daa9d7..cf6e2da3 100644 --- a/gobgpd/main.go +++ b/gobgpd/main.go @@ -169,134 +169,110 @@ func main() { go config.ReadConfigfileServe(opts.ConfigFile, opts.ConfigType, configCh) } - var c *config.BgpConfigSet - for { - select { - case newConfig := <-configCh: - var added, deleted, updated []config.Neighbor - var addedPg, deletedPg, updatedPg []config.PeerGroup - var updatePolicy bool - - if c == nil { - c = newConfig - if _, err := apiServer.StartServer(context.Background(), &api.StartServerRequest{ - Global: api.NewGlobalFromConfigStruct(&c.Global), - }); err != nil { - log.Fatalf("failed to set global config: %s", err) - } - - if newConfig.Zebra.Config.Enabled { - tps := c.Zebra.Config.RedistributeRouteTypeList - l := make([]string, 0, len(tps)) - for _, t := range tps { - l = append(l, string(t)) - } - if _, err := apiServer.EnableZebra(context.Background(), &api.EnableZebraRequest{ - Url: c.Zebra.Config.Url, - RouteTypes: l, - Version: uint32(c.Zebra.Config.Version), - NexthopTriggerEnable: c.Zebra.Config.NexthopTriggerEnable, - NexthopTriggerDelay: uint32(c.Zebra.Config.NexthopTriggerDelay), + loop := func() { + var c *config.BgpConfigSet + for { + select { + case <-sigCh: + apiServer.Shutdown(context.Background(), &api.ShutdownRequest{}) + return + case newConfig := <-configCh: + var added, deleted, updated []config.Neighbor + var addedPg, deletedPg, updatedPg []config.PeerGroup + var updatePolicy bool + + if c == nil { + c = newConfig + if _, err := apiServer.StartServer(context.Background(), &api.StartServerRequest{ + Global: api.NewGlobalFromConfigStruct(&c.Global), }); err != nil { - log.Fatalf("failed to set zebra config: %s", err) + log.Fatalf("failed to set global config: %s", err) } - } - if len(newConfig.Collector.Config.Url) > 0 { - if _, err := apiServer.AddCollector(context.Background(), &api.AddCollectorRequest{ - Url: c.Collector.Config.Url, - DbName: c.Collector.Config.DbName, - TableDumpInterval: c.Collector.Config.TableDumpInterval, - }); err != nil { - log.Fatalf("failed to set collector config: %s", err) + if newConfig.Zebra.Config.Enabled { + tps := c.Zebra.Config.RedistributeRouteTypeList + l := make([]string, 0, len(tps)) + for _, t := range tps { + l = append(l, string(t)) + } + if _, err := apiServer.EnableZebra(context.Background(), &api.EnableZebraRequest{ + Url: c.Zebra.Config.Url, + RouteTypes: l, + Version: uint32(c.Zebra.Config.Version), + NexthopTriggerEnable: c.Zebra.Config.NexthopTriggerEnable, + NexthopTriggerDelay: uint32(c.Zebra.Config.NexthopTriggerDelay), + }); err != nil { + log.Fatalf("failed to set zebra config: %s", err) + } } - } - for _, c := range newConfig.RpkiServers { - if _, err := apiServer.AddRpki(context.Background(), &api.AddRpkiRequest{ - Address: c.Config.Address, - Port: c.Config.Port, - Lifetime: c.Config.RecordLifetime, - }); err != nil { - log.Fatalf("failed to set rpki config: %s", err) - } - } - for _, c := range newConfig.BmpServers { - if _, err := apiServer.AddBmp(context.Background(), &api.AddBmpRequest{ - Address: c.Config.Address, - Port: c.Config.Port, - Type: api.AddBmpRequest_MonitoringPolicy(c.Config.RouteMonitoringPolicy.ToInt()), - }); err != nil { - log.Fatalf("failed to set bmp config: %s", err) - } - } - for _, vrf := range newConfig.Vrfs { - rd, err := bgp.ParseRouteDistinguisher(vrf.Config.Rd) - if err != nil { - log.Fatalf("failed to load vrf rd config: %s", err) + if len(newConfig.Collector.Config.Url) > 0 { + if _, err := apiServer.AddCollector(context.Background(), &api.AddCollectorRequest{ + Url: c.Collector.Config.Url, + DbName: c.Collector.Config.DbName, + TableDumpInterval: c.Collector.Config.TableDumpInterval, + }); err != nil { + log.Fatalf("failed to set collector config: %s", err) + } } - importRtList, err := marshalRouteTargets(vrf.Config.ImportRtList) - if err != nil { - log.Fatalf("failed to load vrf import rt config: %s", err) + for _, c := range newConfig.RpkiServers { + if _, err := apiServer.AddRpki(context.Background(), &api.AddRpkiRequest{ + Address: c.Config.Address, + Port: c.Config.Port, + Lifetime: c.Config.RecordLifetime, + }); err != nil { + log.Fatalf("failed to set rpki config: %s", err) + } } - exportRtList, err := marshalRouteTargets(vrf.Config.ExportRtList) - if err != nil { - log.Fatalf("failed to load vrf export rt config: %s", err) + for _, c := range newConfig.BmpServers { + if _, err := apiServer.AddBmp(context.Background(), &api.AddBmpRequest{ + Address: c.Config.Address, + Port: c.Config.Port, + Type: api.AddBmpRequest_MonitoringPolicy(c.Config.RouteMonitoringPolicy.ToInt()), + }); err != nil { + log.Fatalf("failed to set bmp config: %s", err) + } } + for _, vrf := range newConfig.Vrfs { + rd, err := bgp.ParseRouteDistinguisher(vrf.Config.Rd) + if err != nil { + log.Fatalf("failed to load vrf rd config: %s", err) + } - if _, err := apiServer.AddVrf(context.Background(), &api.AddVrfRequest{ - Vrf: &api.Vrf{ - Name: vrf.Config.Name, - Rd: api.MarshalRD(rd), - Id: uint32(vrf.Config.Id), - ImportRt: importRtList, - ExportRt: exportRtList, - }, - }); err != nil { - log.Fatalf("failed to set vrf config: %s", err) - } - } - for _, c := range newConfig.MrtDump { - if len(c.Config.FileName) == 0 { - continue - } - if _, err := apiServer.EnableMrt(context.Background(), &api.EnableMrtRequest{ - DumpType: int32(c.Config.DumpType.ToInt()), - Filename: c.Config.FileName, - Interval: c.Config.DumpInterval, - }); err != nil { - log.Fatalf("failed to set mrt config: %s", err) - } - } - p := config.ConfigSetToRoutingPolicy(newConfig) - rp, err := api.NewAPIRoutingPolicyFromConfigStruct(p) - if err != nil { - log.Warn(err) - } else { - apiServer.UpdatePolicy(context.Background(), &api.UpdatePolicyRequest{ - Sets: rp.DefinedSet, - Policies: rp.PolicyDefinition, - }) - } + importRtList, err := marshalRouteTargets(vrf.Config.ImportRtList) + if err != nil { + log.Fatalf("failed to load vrf import rt config: %s", err) + } + exportRtList, err := marshalRouteTargets(vrf.Config.ExportRtList) + if err != nil { + log.Fatalf("failed to load vrf export rt config: %s", err) + } - added = newConfig.Neighbors - addedPg = newConfig.PeerGroups - if opts.GracefulRestart { - for i, n := range added { - if n.GracefulRestart.Config.Enabled { - added[i].GracefulRestart.State.LocalRestarting = true + if _, err := apiServer.AddVrf(context.Background(), &api.AddVrfRequest{ + Vrf: &api.Vrf{ + Name: vrf.Config.Name, + Rd: api.MarshalRD(rd), + Id: uint32(vrf.Config.Id), + ImportRt: importRtList, + ExportRt: exportRtList, + }, + }); err != nil { + log.Fatalf("failed to set vrf config: %s", err) + } + } + for _, c := range newConfig.MrtDump { + if len(c.Config.FileName) == 0 { + continue + } + if _, err := apiServer.EnableMrt(context.Background(), &api.EnableMrtRequest{ + DumpType: int32(c.Config.DumpType.ToInt()), + Filename: c.Config.FileName, + Interval: c.Config.DumpInterval, + }); err != nil { + log.Fatalf("failed to set mrt config: %s", err) } } - } - - } else { - addedPg, deletedPg, updatedPg = config.UpdatePeerGroupConfig(c, newConfig) - added, deleted, updated = config.UpdateNeighborConfig(c, newConfig) - updatePolicy = config.CheckPolicyDifference(config.ConfigSetToRoutingPolicy(c), config.ConfigSetToRoutingPolicy(newConfig)) - - if updatePolicy { - log.Info("Policy config is updated") p := config.ConfigSetToRoutingPolicy(newConfig) rp, err := api.NewAPIRoutingPolicyFromConfigStruct(p) if err != nil { @@ -307,139 +283,168 @@ func main() { Policies: rp.PolicyDefinition, }) } - } - // global policy update - if !newConfig.Global.ApplyPolicy.Config.Equal(&c.Global.ApplyPolicy.Config) { - a := newConfig.Global.ApplyPolicy.Config - toDefaultTable := func(r config.DefaultPolicyType) table.RouteType { - var def table.RouteType - switch r { - case config.DEFAULT_POLICY_TYPE_ACCEPT_ROUTE: - def = table.ROUTE_TYPE_ACCEPT - case config.DEFAULT_POLICY_TYPE_REJECT_ROUTE: - def = table.ROUTE_TYPE_REJECT + + added = newConfig.Neighbors + addedPg = newConfig.PeerGroups + if opts.GracefulRestart { + for i, n := range added { + if n.GracefulRestart.Config.Enabled { + added[i].GracefulRestart.State.LocalRestarting = true + } } - return def } - toPolicies := func(r []string) []*table.Policy { - p := make([]*table.Policy, 0, len(r)) - for _, n := range r { - p = append(p, &table.Policy{ - Name: n, + + } else { + addedPg, deletedPg, updatedPg = config.UpdatePeerGroupConfig(c, newConfig) + added, deleted, updated = config.UpdateNeighborConfig(c, newConfig) + updatePolicy = config.CheckPolicyDifference(config.ConfigSetToRoutingPolicy(c), config.ConfigSetToRoutingPolicy(newConfig)) + + if updatePolicy { + log.Info("Policy config is updated") + p := config.ConfigSetToRoutingPolicy(newConfig) + rp, err := api.NewAPIRoutingPolicyFromConfigStruct(p) + if err != nil { + log.Warn(err) + } else { + apiServer.UpdatePolicy(context.Background(), &api.UpdatePolicyRequest{ + Sets: rp.DefinedSet, + Policies: rp.PolicyDefinition, }) } - return p } + // global policy update + if !newConfig.Global.ApplyPolicy.Config.Equal(&c.Global.ApplyPolicy.Config) { + a := newConfig.Global.ApplyPolicy.Config + toDefaultTable := func(r config.DefaultPolicyType) table.RouteType { + var def table.RouteType + switch r { + case config.DEFAULT_POLICY_TYPE_ACCEPT_ROUTE: + def = table.ROUTE_TYPE_ACCEPT + case config.DEFAULT_POLICY_TYPE_REJECT_ROUTE: + def = table.ROUTE_TYPE_REJECT + } + return def + } + toPolicies := func(r []string) []*table.Policy { + p := make([]*table.Policy, 0, len(r)) + for _, n := range r { + p = append(p, &table.Policy{ + Name: n, + }) + } + return p + } + + def := toDefaultTable(a.DefaultImportPolicy) + ps := toPolicies(a.ImportPolicyList) + apiServer.ReplacePolicyAssignment(context.Background(), &api.ReplacePolicyAssignmentRequest{ + Assignment: api.NewAPIPolicyAssignmentFromTableStruct(&table.PolicyAssignment{ + Name: "", + Type: table.POLICY_DIRECTION_IMPORT, + Policies: ps, + Default: def, + }), + }) + + def = toDefaultTable(a.DefaultExportPolicy) + ps = toPolicies(a.ExportPolicyList) + apiServer.ReplacePolicyAssignment(context.Background(), &api.ReplacePolicyAssignmentRequest{ + Assignment: api.NewAPIPolicyAssignmentFromTableStruct(&table.PolicyAssignment{ + Name: "", + Type: table.POLICY_DIRECTION_EXPORT, + Policies: ps, + Default: def, + }), + }) - def := toDefaultTable(a.DefaultImportPolicy) - ps := toPolicies(a.ImportPolicyList) - apiServer.ReplacePolicyAssignment(context.Background(), &api.ReplacePolicyAssignmentRequest{ - Assignment: api.NewAPIPolicyAssignmentFromTableStruct(&table.PolicyAssignment{ - Name: "", - Type: table.POLICY_DIRECTION_IMPORT, - Policies: ps, - Default: def, - }), - }) - - def = toDefaultTable(a.DefaultExportPolicy) - ps = toPolicies(a.ExportPolicyList) - apiServer.ReplacePolicyAssignment(context.Background(), &api.ReplacePolicyAssignmentRequest{ - Assignment: api.NewAPIPolicyAssignmentFromTableStruct(&table.PolicyAssignment{ - Name: "", - Type: table.POLICY_DIRECTION_EXPORT, - Policies: ps, - Default: def, - }), - }) - - updatePolicy = true + updatePolicy = true + } + c = newConfig } - c = newConfig - } - for _, pg := range addedPg { - log.Infof("PeerGroup %s is added", pg.Config.PeerGroupName) - if _, err := apiServer.AddPeerGroup(context.Background(), &api.AddPeerGroupRequest{ - PeerGroup: api.NewPeerGroupFromConfigStruct(&pg), - }); err != nil { - log.Warn(err) + for _, pg := range addedPg { + log.Infof("PeerGroup %s is added", pg.Config.PeerGroupName) + if _, err := apiServer.AddPeerGroup(context.Background(), &api.AddPeerGroupRequest{ + PeerGroup: api.NewPeerGroupFromConfigStruct(&pg), + }); err != nil { + log.Warn(err) + } } - } - for _, pg := range deletedPg { - log.Infof("PeerGroup %s is deleted", pg.Config.PeerGroupName) - if _, err := apiServer.DeletePeerGroup(context.Background(), &api.DeletePeerGroupRequest{ - PeerGroup: api.NewPeerGroupFromConfigStruct(&pg), - }); err != nil { - log.Warn(err) + for _, pg := range deletedPg { + log.Infof("PeerGroup %s is deleted", pg.Config.PeerGroupName) + if _, err := apiServer.DeletePeerGroup(context.Background(), &api.DeletePeerGroupRequest{ + PeerGroup: api.NewPeerGroupFromConfigStruct(&pg), + }); err != nil { + log.Warn(err) + } } - } - for _, pg := range updatedPg { - log.Infof("PeerGroup %v is updated", pg.State.PeerGroupName) - if u, err := apiServer.UpdatePeerGroup(context.Background(), &api.UpdatePeerGroupRequest{ - PeerGroup: api.NewPeerGroupFromConfigStruct(&pg), - }); err != nil { - log.Warn(err) - } else { - updatePolicy = updatePolicy || u.NeedsSoftResetIn + for _, pg := range updatedPg { + log.Infof("PeerGroup %v is updated", pg.State.PeerGroupName) + if u, err := apiServer.UpdatePeerGroup(context.Background(), &api.UpdatePeerGroupRequest{ + PeerGroup: api.NewPeerGroupFromConfigStruct(&pg), + }); err != nil { + log.Warn(err) + } else { + updatePolicy = updatePolicy || u.NeedsSoftResetIn + } } - } - for _, pg := range updatedPg { - log.Infof("PeerGroup %s is updated", pg.Config.PeerGroupName) - if _, err := apiServer.UpdatePeerGroup(context.Background(), &api.UpdatePeerGroupRequest{ - PeerGroup: api.NewPeerGroupFromConfigStruct(&pg), - }); err != nil { - log.Warn(err) + for _, pg := range updatedPg { + log.Infof("PeerGroup %s is updated", pg.Config.PeerGroupName) + if _, err := apiServer.UpdatePeerGroup(context.Background(), &api.UpdatePeerGroupRequest{ + PeerGroup: api.NewPeerGroupFromConfigStruct(&pg), + }); err != nil { + log.Warn(err) + } } - } - for _, dn := range newConfig.DynamicNeighbors { - log.Infof("Dynamic Neighbor %s is added to PeerGroup %s", dn.Config.Prefix, dn.Config.PeerGroup) - if _, err := apiServer.AddDynamicNeighbor(context.Background(), &api.AddDynamicNeighborRequest{ - DynamicNeighbor: &api.DynamicNeighbor{ - Prefix: dn.Config.Prefix, - PeerGroup: dn.Config.PeerGroup, - }, - }); err != nil { - log.Warn(err) + for _, dn := range newConfig.DynamicNeighbors { + log.Infof("Dynamic Neighbor %s is added to PeerGroup %s", dn.Config.Prefix, dn.Config.PeerGroup) + if _, err := apiServer.AddDynamicNeighbor(context.Background(), &api.AddDynamicNeighborRequest{ + DynamicNeighbor: &api.DynamicNeighbor{ + Prefix: dn.Config.Prefix, + PeerGroup: dn.Config.PeerGroup, + }, + }); err != nil { + log.Warn(err) + } } - } - for _, p := range added { - log.Infof("Peer %v is added", p.State.NeighborAddress) - if _, err := apiServer.AddNeighbor(context.Background(), &api.AddNeighborRequest{ - Peer: api.NewPeerFromConfigStruct(&p), - }); err != nil { - log.Warn(err) + for _, p := range added { + log.Infof("Peer %v is added", p.State.NeighborAddress) + if _, err := apiServer.AddNeighbor(context.Background(), &api.AddNeighborRequest{ + Peer: api.NewPeerFromConfigStruct(&p), + }); err != nil { + log.Warn(err) + } } - } - for _, p := range deleted { - log.Infof("Peer %v is deleted", p.State.NeighborAddress) - if _, err := apiServer.DeleteNeighbor(context.Background(), &api.DeleteNeighborRequest{ - Peer: api.NewPeerFromConfigStruct(&p), - }); err != nil { - log.Warn(err) + for _, p := range deleted { + log.Infof("Peer %v is deleted", p.State.NeighborAddress) + if _, err := apiServer.DeleteNeighbor(context.Background(), &api.DeleteNeighborRequest{ + Peer: api.NewPeerFromConfigStruct(&p), + }); err != nil { + log.Warn(err) + } } - } - for _, p := range updated { - log.Infof("Peer %v is updated", p.State.NeighborAddress) - if u, err := apiServer.UpdateNeighbor(context.Background(), &api.UpdateNeighborRequest{ - Peer: api.NewPeerFromConfigStruct(&p), - }); err != nil { - log.Warn(err) - } else { - updatePolicy = updatePolicy || u.NeedsSoftResetIn + for _, p := range updated { + log.Infof("Peer %v is updated", p.State.NeighborAddress) + if u, err := apiServer.UpdateNeighbor(context.Background(), &api.UpdateNeighborRequest{ + Peer: api.NewPeerFromConfigStruct(&p), + }); err != nil { + log.Warn(err) + } else { + updatePolicy = updatePolicy || u.NeedsSoftResetIn + } } - } - if updatePolicy { - if _, err := apiServer.SoftResetNeighbor(context.Background(), &api.SoftResetNeighborRequest{ - Address: "", - Direction: api.SoftResetNeighborRequest_IN, - }); err != nil { - log.Warn(err) + if updatePolicy { + if _, err := apiServer.SoftResetNeighbor(context.Background(), &api.SoftResetNeighborRequest{ + Address: "", + Direction: api.SoftResetNeighborRequest_IN, + }); err != nil { + log.Warn(err) + } } } - case <-sigCh: - apiServer.Shutdown(context.Background(), &api.ShutdownRequest{}) } } + + loop() } diff --git a/server/server.go b/server/server.go index 31d2d42e..f642ece3 100644 --- a/server/server.go +++ b/server/server.go @@ -19,8 +19,8 @@ import ( "bytes" "fmt" "net" - "os" "strconv" + "sync" "time" "github.com/eapache/channels" @@ -111,7 +111,7 @@ type BgpServer struct { globalRib *table.TableManager rsRib *table.TableManager roaManager *roaManager - shutdown bool + shutdownWG *sync.WaitGroup watcherMap map[WatchEventType][]*Watcher zclient *zebraClient bmpManager *bmpClientManager @@ -1233,7 +1233,7 @@ func (server *BgpServer) handleFSMMessage(peer *Peer, e *FsmMsg) { } } } else { - if server.shutdown && nextState == bgp.BGP_FSM_IDLE { + if server.shutdownWG != nil && nextState == bgp.BGP_FSM_IDLE { die := true for _, p := range server.neighborMap { if p.fsm.state != bgp.BGP_FSM_IDLE { @@ -1242,7 +1242,7 @@ func (server *BgpServer) handleFSMMessage(peer *Peer, e *FsmMsg) { } } if die { - os.Exit(0) + server.shutdownWG.Done() } } peer.fsm.pConf.Timers.State.Downtime = time.Now().Unix() @@ -1415,18 +1415,26 @@ func (s *BgpServer) DeleteBmp(c *config.BmpServerConfig) error { func (s *BgpServer) Shutdown() { s.mgmtOperation(func() error { - s.shutdown = true - stateOp := AdminStateOperation{ADMIN_STATE_DOWN, nil} + s.shutdownWG = new(sync.WaitGroup) + s.shutdownWG.Add(1) + stateOp := AdminStateOperation{ + State: ADMIN_STATE_DOWN, + Communication: nil, + } for _, p := range s.neighborMap { p.fsm.adminStateCh <- stateOp } - // the main goroutine waits for peers' goroutines to stop but if no peer is configured, needs to die immediately. - if len(s.neighborMap) == 0 { - os.Exit(0) - } // TODO: call fsmincomingCh.Close() return nil }, false) + + // Waits for all goroutines per peer to stop. + // Note: This should not be wrapped with s.mgmtOperation() in order to + // avoid the deadlock in the main goroutine of BgpServer. + if s.shutdownWG != nil { + s.shutdownWG.Wait() + s.shutdownWG = nil + } } func (s *BgpServer) UpdatePolicy(policy config.RoutingPolicy) error { |