From 791b2fd503f3a93e49af2848b9ab213c738ce2fe Mon Sep 17 00:00:00 2001 From: Yicheng Qin Date: Wed, 5 Nov 2014 12:14:42 -0800 Subject: [PATCH] *: handle panic and fatal more consistently 1. etcd fatals if there is critical error in the system and operator should do something for it 2. etcd panics if there happens something unexpected, and it should be reported to us to debug. --- etcdserver/cluster.go | 14 +++++----- etcdserver/etcdhttp/httptypes/errors.go | 3 ++- etcdserver/member.go | 2 +- etcdserver/server.go | 36 ++++++++++++------------- pkg/flags/urls.go | 2 +- pkg/pbutil/pbutil.go | 4 +-- snap/snapshotter.go | 7 ++--- wal/util.go | 4 +-- 8 files changed, 35 insertions(+), 37 deletions(-) diff --git a/etcdserver/cluster.go b/etcdserver/cluster.go index 68d82295d..2d76ac3c2 100644 --- a/etcdserver/cluster.go +++ b/etcdserver/cluster.go @@ -164,7 +164,7 @@ func (c *Cluster) MemberByName(name string) *Member { for _, m := range c.members { if m.Name == name { if memb != nil { - panic("two members with the given name exist in the cluster") + log.Panicf("two members with the given name %s exist", name) } memb = m } @@ -270,19 +270,19 @@ func (c *Cluster) SetStore(st store.Store) { c.store = st } func (c *Cluster) AddMember(m *Member) { b, err := json.Marshal(m.RaftAttributes) if err != nil { - log.Panicf("marshal error: %v", err) + log.Panicf("marshal raftAttributes should never fail: %v", err) } p := path.Join(memberStoreKey(m.ID), raftAttributesSuffix) if _, err := c.store.Create(p, false, string(b), false, store.Permanent); err != nil { - log.Panicf("add raftAttributes should never fail: %v", err) + log.Panicf("create raftAttributes should never fail: %v", err) } b, err = json.Marshal(m.Attributes) if err != nil { - log.Panicf("marshal error: %v", err) + log.Panicf("marshal attributes should never fail: %v", err) } p = path.Join(memberStoreKey(m.ID), attributesSuffix) if _, err := c.store.Create(p, false, string(b), false, store.Permanent); err != nil { - log.Panicf("add attributes should never fail: %v", err) + log.Panicf("create attributes should never fail: %v", err) } c.members[m.ID] = m } @@ -291,11 +291,11 @@ func (c *Cluster) AddMember(m *Member) { // The given id MUST exist, or the function panics. func (c *Cluster) RemoveMember(id types.ID) { if _, err := c.store.Delete(memberStoreKey(id), true, true); err != nil { - log.Panicf("delete peer should never fail: %v", err) + log.Panicf("delete member should never fail: %v", err) } delete(c.members, id) if _, err := c.store.Create(removedMemberStoreKey(id), false, "", false, store.Permanent); err != nil { - log.Panicf("creating RemovedMember should never fail: %v", err) + log.Panicf("create removedMember should never fail: %v", err) } c.removed[id] = true } diff --git a/etcdserver/etcdhttp/httptypes/errors.go b/etcdserver/etcdhttp/httptypes/errors.go index 548cf8ffd..67ebd3da3 100644 --- a/etcdserver/etcdhttp/httptypes/errors.go +++ b/etcdserver/etcdhttp/httptypes/errors.go @@ -18,6 +18,7 @@ package httptypes import ( "encoding/json" + "log" "net/http" ) @@ -37,7 +38,7 @@ func (e HTTPError) WriteTo(w http.ResponseWriter) { w.WriteHeader(e.Code) b, err := json.Marshal(e) if err != nil { - panic("unexpected json marshal error") + log.Panicf("marshal HTTPError should never fail: %v", err) } w.Write(b) } diff --git a/etcdserver/member.go b/etcdserver/member.go index ebe48e846..cce6a5a58 100644 --- a/etcdserver/member.go +++ b/etcdserver/member.go @@ -75,7 +75,7 @@ func NewMember(name string, peerURLs types.URLs, clusterName string, now *time.T // It will panic if there is no PeerURLs available in Member. func (m *Member) PickPeerURL() string { if len(m.PeerURLs) == 0 { - panic("member should always have some peer url") + log.Panicf("member should always have some peer url") } return m.PeerURLs[rand.Intn(len(m.PeerURLs))] } diff --git a/etcdserver/server.go b/etcdserver/server.go index f526a2f5a..40f82ed26 100644 --- a/etcdserver/server.go +++ b/etcdserver/server.go @@ -189,7 +189,7 @@ func NewServer(cfg *ServerConfig) (*EtcdServer, error) { case !haveWAL && cfg.ClusterState == ClusterStateValueExisting: cl, err := GetClusterFromPeers(cfg.Cluster.PeerURLs()) if err != nil { - log.Fatal(err) + return nil, fmt.Errorf("cannot fetch cluster info from peer urls: %v", err) } if err := cfg.Cluster.ValidateAndAssignIDs(cl.Members()); err != nil { return nil, fmt.Errorf("error validating IDs from cluster %s: %v", cl, err) @@ -316,10 +316,10 @@ func (s *EtcdServer) run() { } if err := s.storage.Save(rd.HardState, rd.Entries); err != nil { - log.Panicf("etcdserver: save state and entries error: %v", err) + log.Fatalf("etcdserver: save state and entries error: %v", err) } if err := s.storage.SaveSnap(rd.Snapshot); err != nil { - log.Panicf("etcdserver: create snapshot error: %v", err) + log.Fatalf("etcdserver: create snapshot error: %v", err) } s.send(rd.Messages) @@ -338,7 +338,7 @@ func (s *EtcdServer) run() { // recover from snapshot if it is more updated than current applied if rd.Snapshot.Index > appliedi { if err := s.store.Recovery(rd.Snapshot.Data); err != nil { - panic("TODO: this is bad, what do we do about it?") + log.Panicf("recovery store error: %v", err) } appliedi = rd.Snapshot.Index } @@ -371,7 +371,7 @@ func (s *EtcdServer) Stop() { // an error. func (s *EtcdServer) Do(ctx context.Context, r pb.Request) (Response, error) { if r.ID == 0 { - panic("r.ID cannot be 0") + log.Panicf("request ID should never be 0") } if r.Method == "GET" && r.Quorum { r.Method = "QGET" @@ -484,7 +484,7 @@ func (s *EtcdServer) configure(ctx context.Context, cc raftpb.ConfChange) error return err } if x != nil { - log.Panicf("unexpected return type") + log.Panicf("return type should always be error") } return nil case <-ctx.Done(): @@ -571,7 +571,7 @@ func (s *EtcdServer) apply(es []raftpb.Entry, nodes []uint64) uint64 { pbutil.MustUnmarshal(&cc, e.Data) s.w.Trigger(cc.ID, s.applyConfChange(cc, nodes)) default: - panic("unexpected entry type") + log.Panicf("entry type should be either EntryNormal or EntryConfChange") } atomic.StoreUint64(&s.raftIndex, e.Index) atomic.StoreUint64(&s.raftTerm, e.Term) @@ -605,10 +605,10 @@ func (s *EtcdServer) applyRequest(r pb.Request) Response { id := mustParseMemberIDFromKey(path.Dir(r.Path)) m := s.Cluster.Member(id) if m == nil { - log.Fatalf("fetch member %s should never fail", id) + log.Panicf("fetch member %s should never fail", id) } if err := json.Unmarshal([]byte(r.Val), &m.Attributes); err != nil { - log.Fatalf("unmarshal %s should never fail: %v", r.Val, err) + log.Panicf("unmarshal %s should never fail: %v", r.Val, err) } } return f(s.store.Set(r.Path, r.Dir, r.Val, expr)) @@ -642,10 +642,10 @@ func (s *EtcdServer) applyConfChange(cc raftpb.ConfChange, nodes []uint64) error case raftpb.ConfChangeAddNode: m := new(Member) if err := json.Unmarshal(cc.Context, m); err != nil { - panic("unexpected unmarshal error") + log.Panicf("unmarshal member should never fail: %v", err) } if cc.NodeID != uint64(m.ID) { - panic("unexpected nodeID mismatch") + log.Panicf("nodeID should always be equal to member ID") } s.Cluster.AddMember(m) log.Printf("etcdserver: added node %s to cluster", types.ID(cc.NodeID)) @@ -671,7 +671,7 @@ func (s *EtcdServer) checkConfChange(cc raftpb.ConfChange, nodes []uint64) error return ErrIDNotFound } default: - panic("unexpected ConfChange type") + log.Panicf("ConfChange type should be either AddNode or RemoveNode") } return nil } @@ -682,11 +682,11 @@ func (s *EtcdServer) snapshot(snapi uint64, snapnodes []uint64) { // TODO: current store will never fail to do a snapshot // what should we do if the store might fail? if err != nil { - panic("TODO: this is bad, what do we do about it?") + log.Panicf("store save should never fail: %v", err) } s.node.Compact(snapi, snapnodes, d) if err := s.storage.Cut(); err != nil { - log.Panicf("etcdserver: rotate wal file error: %v", err) + log.Panicf("rotate wal file should never fail: %v", err) } } @@ -729,13 +729,13 @@ func startNode(cfg *ServerConfig, ids []types.ID) (id types.ID, n raft.Node, w * }, ) if w, err = wal.Create(cfg.WALDir(), metadata); err != nil { - log.Fatal(err) + log.Fatalf("etcdserver: create wal error: %v", err) } peers := make([]raft.Peer, len(ids)) for i, id := range ids { ctx, err := json.Marshal((*cfg.Cluster).Member(id)) if err != nil { - log.Fatal(err) + log.Panicf("marshal member should never fail: %v", err) } peers[i] = raft.Peer{ID: uint64(id), Context: ctx} } @@ -749,11 +749,11 @@ func restartNode(cfg *ServerConfig, index uint64, snapshot *raftpb.Snapshot) (id var err error // restart a node from previous wal if w, err = wal.OpenAtIndex(cfg.WALDir(), index); err != nil { - log.Fatal(err) + log.Fatalf("etcdserver: open wal error: %v", err) } wmetadata, st, ents, err := w.ReadAll() if err != nil { - log.Fatal(err) + log.Fatalf("etcdserver: read wal error: %v", err) } var metadata pb.Metadata diff --git a/pkg/flags/urls.go b/pkg/flags/urls.go index 2777a807a..f4baa7ee8 100644 --- a/pkg/flags/urls.go +++ b/pkg/flags/urls.go @@ -49,7 +49,7 @@ func (us *URLsValue) String() string { func NewURLsValue(init string) *URLsValue { v := &URLsValue{} if err := v.Set(init); err != nil { - log.Panicf("error setting URLsValue: %v", err) + log.Panicf("new URLsValue should never fail: %v", err) } return v } diff --git a/pkg/pbutil/pbutil.go b/pkg/pbutil/pbutil.go index 04fd25966..a2669dbc2 100644 --- a/pkg/pbutil/pbutil.go +++ b/pkg/pbutil/pbutil.go @@ -29,13 +29,13 @@ type Unmarshaler interface { func MustMarshal(m Marshaler) []byte { d, err := m.Marshal() if err != nil { - log.Panicf("pbutil: %v", err) + log.Panicf("marshal protobuf type should never fail: %v", err) } return d } func MustUnmarshal(um Unmarshaler, data []byte) { if err := um.Unmarshal(data); err != nil { - log.Panicf("pbutil: %v", err) + log.Panicf("unmarshal protobuf type should never fail: %v", err) } } diff --git a/snap/snapshotter.go b/snap/snapshotter.go index 3675b1045..1cdd6e7e5 100644 --- a/snap/snapshotter.go +++ b/snap/snapshotter.go @@ -27,6 +27,7 @@ import ( "sort" "strings" + "github.com/coreos/etcd/pkg/pbutil" "github.com/coreos/etcd/raft" "github.com/coreos/etcd/raft/raftpb" "github.com/coreos/etcd/snap/snappb" @@ -61,11 +62,7 @@ func (s *Snapshotter) SaveSnap(snapshot raftpb.Snapshot) error { func (s *Snapshotter) save(snapshot *raftpb.Snapshot) error { fname := fmt.Sprintf("%016x-%016x%s", snapshot.Term, snapshot.Index, snapSuffix) - b, err := snapshot.Marshal() - if err != nil { - panic(err) - } - + b := pbutil.MustMarshal(snapshot) crc := crc32.Update(0, crcTable, b) snap := snappb.Snapshot{Crc: crc, Data: b} d, err := snap.Marshal() diff --git a/wal/util.go b/wal/util.go index 21cd38d84..684df5358 100644 --- a/wal/util.go +++ b/wal/util.go @@ -38,7 +38,7 @@ func searchIndex(names []string, index uint64) (int, bool) { name := names[i] _, curIndex, err := parseWalName(name) if err != nil { - panic("parse correct name error") + log.Panicf("parse correct name should never fail: %v", err) } if index >= curIndex { return i, true @@ -54,7 +54,7 @@ func isValidSeq(names []string) bool { for _, name := range names { curSeq, _, err := parseWalName(name) if err != nil { - panic("parse correct name error") + log.Panicf("parse correct name should never fail: %v", err) } if lastSeq != 0 && lastSeq != curSeq-1 { return false