From b6851a54212c771bb82c5f3502f2d312584703ed Mon Sep 17 00:00:00 2001 From: Fabian Reinartz Date: Mon, 21 Nov 2016 11:09:49 +0100 Subject: [PATCH] silences: fix concurrent cache writes (#561) This fixes #559 by removing concurrent map writes to the matcher cache. The cache was guarded by the Silence's main lock, which only used a read-lock on queries. The cache's get methods lazily loads data into the cache and thus causing concurrent writes. We just change the main lock to always write-lock, as we don't expect high lock contention at this point and would have it in a dedicated cache lock anyway. --- config/notifiers.go | 2 +- silence/silence.go | 25 +++++---- template/internal/deftmpl/bindata.go | 2 +- ui/bindata.go | 76 ++++++++++++++-------------- 4 files changed, 54 insertions(+), 51 deletions(-) diff --git a/config/notifiers.go b/config/notifiers.go index ffea3d19..06131ea4 100644 --- a/config/notifiers.go +++ b/config/notifiers.go @@ -32,7 +32,7 @@ var ( NotifierConfig: NotifierConfig{ VSendResolved: false, }, - HTML: `{{ template "email.default.html" . }}`, + HTML: `{{ template "email.default.html" . }}`, } // DefaultEmailSubject defines the default Subject header of an Email. diff --git a/silence/silence.go b/silence/silence.go index 2a1fdec5..b1e21c1b 100644 --- a/silence/silence.go +++ b/silence/silence.go @@ -96,16 +96,16 @@ type Silences struct { now func() time.Time retention time.Duration - mc matcherCache - gossip mesh.Gossip // gossip channel for sharing silences // We store silences in a map of IDs for now. Currently, the memory // state is equivalent to the mesh.GossipData representation. // In the future we'll want support for efficient queries by time // range and affected labels. - mtx sync.RWMutex + // Mutex also guards the matcherCache, which always need write lock access. + mtx sync.Mutex st gossipData + mc matcherCache } type metrics struct { @@ -632,7 +632,9 @@ func (s *Silences) query(q *query, now *timestamp.Timestamp) ([]*pb.Silence, err // the trivial solution for now. var res []*pb.Silence - s.mtx.RLock() + s.mtx.Lock() + defer s.mtx.Unlock() + if q.ids != nil { for _, id := range q.ids { if s, ok := s.st[string(id)]; ok { @@ -662,7 +664,6 @@ func (s *Silences) query(q *query, now *timestamp.Timestamp) ([]*pb.Silence, err resf = append(resf, sil) } } - s.mtx.RUnlock() return resf, nil } @@ -672,6 +673,9 @@ func (s *Silences) query(q *query, now *timestamp.Timestamp) ([]*pb.Silence, err func (s *Silences) loadSnapshot(r io.Reader) error { st := gossipData{} + s.mtx.Lock() + defer s.mtx.Unlock() + for { var sil pb.MeshSilence if _, err := pbutil.ReadDelimited(r, &sil); err != nil { @@ -686,9 +690,8 @@ func (s *Silences) loadSnapshot(r io.Reader) error { return err } } - s.mtx.Lock() + s.st = st - s.mtx.Unlock() return nil } @@ -699,8 +702,8 @@ func (s *Silences) Snapshot(w io.Writer) (int, error) { start := time.Now() defer func() { s.metrics.snapshotDuration.Observe(time.Since(start).Seconds()) }() - s.mtx.RLock() - defer s.mtx.RUnlock() + s.mtx.Lock() + defer s.mtx.Unlock() var n int for _, s := range s.st { @@ -719,8 +722,8 @@ type gossiper struct { // Gossip implements the mesh.Gossiper interface. func (g gossiper) Gossip() mesh.GossipData { - g.mtx.RLock() - defer g.mtx.RUnlock() + g.mtx.Lock() + defer g.mtx.Unlock() return g.st.clone() } diff --git a/template/internal/deftmpl/bindata.go b/template/internal/deftmpl/bindata.go index 15448989..8464c1cd 100644 --- a/template/internal/deftmpl/bindata.go +++ b/template/internal/deftmpl/bindata.go @@ -182,6 +182,7 @@ type bintree struct { Func func() (*asset, error) Children map[string]*bintree } + var _bintree = &bintree{nil, map[string]*bintree{ "template": &bintree{nil, map[string]*bintree{ "default.tmpl": &bintree{templateDefaultTmpl, map[string]*bintree{}}, @@ -234,4 +235,3 @@ func _filePath(dir, name string) string { cannonicalName := strings.Replace(name, "\\", "/", -1) return filepath.Join(append([]string{dir}, strings.Split(cannonicalName, "/")...)...) } - diff --git a/ui/bindata.go b/ui/bindata.go index e186b45f..2f494364 100644 --- a/ui/bindata.go +++ b/ui/bindata.go @@ -602,29 +602,29 @@ func AssetNames() []string { // _bindata is a table, holding each asset generator, mapped to its name. var _bindata = map[string]func() (*asset, error){ - "ui/app/css/main.css": uiAppCssMainCss, - "ui/app/index.html": uiAppIndexHtml, - "ui/app/js/app.js": uiAppJsAppJs, - "ui/app/partials/alert.html": uiAppPartialsAlertHtml, - "ui/app/partials/alerts.html": uiAppPartialsAlertsHtml, - "ui/app/partials/route.html": uiAppPartialsRouteHtml, + "ui/app/css/main.css": uiAppCssMainCss, + "ui/app/index.html": uiAppIndexHtml, + "ui/app/js/app.js": uiAppJsAppJs, + "ui/app/partials/alert.html": uiAppPartialsAlertHtml, + "ui/app/partials/alerts.html": uiAppPartialsAlertsHtml, + "ui/app/partials/route.html": uiAppPartialsRouteHtml, "ui/app/partials/silence-form.html": uiAppPartialsSilenceFormHtml, - "ui/app/partials/silence.html": uiAppPartialsSilenceHtml, - "ui/app/partials/silences.html": uiAppPartialsSilencesHtml, - "ui/app/partials/status.html": uiAppPartialsStatusHtml, - "ui/bindata.go": uiBindataGo, - "ui/lib/angular-moment.min.js": uiLibAngularMomentMinJs, - "ui/lib/angular-resource.min.js": uiLibAngularResourceMinJs, - "ui/lib/angular-route.min.js": uiLibAngularRouteMinJs, - "ui/lib/angular-sanitize.min.js": uiLibAngularSanitizeMinJs, - "ui/lib/angular.min.js": uiLibAngularMinJs, - "ui/lib/d3.v3.min.js": uiLibD3V3MinJs, - "ui/lib/jquery.min.js": uiLibJqueryMinJs, - "ui/lib/js-yaml.min.js": uiLibJsYamlMinJs, - "ui/lib/kube.min.css": uiLibKubeMinCss, - "ui/lib/moment.min.js": uiLibMomentMinJs, - "ui/lib/routing-tree.js": uiLibRoutingTreeJs, - "ui/web.go": uiWebGo, + "ui/app/partials/silence.html": uiAppPartialsSilenceHtml, + "ui/app/partials/silences.html": uiAppPartialsSilencesHtml, + "ui/app/partials/status.html": uiAppPartialsStatusHtml, + "ui/bindata.go": uiBindataGo, + "ui/lib/angular-moment.min.js": uiLibAngularMomentMinJs, + "ui/lib/angular-resource.min.js": uiLibAngularResourceMinJs, + "ui/lib/angular-route.min.js": uiLibAngularRouteMinJs, + "ui/lib/angular-sanitize.min.js": uiLibAngularSanitizeMinJs, + "ui/lib/angular.min.js": uiLibAngularMinJs, + "ui/lib/d3.v3.min.js": uiLibD3V3MinJs, + "ui/lib/jquery.min.js": uiLibJqueryMinJs, + "ui/lib/js-yaml.min.js": uiLibJsYamlMinJs, + "ui/lib/kube.min.css": uiLibKubeMinCss, + "ui/lib/moment.min.js": uiLibMomentMinJs, + "ui/lib/routing-tree.js": uiLibRoutingTreeJs, + "ui/web.go": uiWebGo, } // AssetDir returns the file names below a certain @@ -666,6 +666,7 @@ type bintree struct { Func func() (*asset, error) Children map[string]*bintree } + var _bintree = &bintree{nil, map[string]*bintree{ "ui": &bintree{nil, map[string]*bintree{ "app": &bintree{nil, map[string]*bintree{ @@ -677,28 +678,28 @@ var _bintree = &bintree{nil, map[string]*bintree{ "app.js": &bintree{uiAppJsAppJs, map[string]*bintree{}}, }}, "partials": &bintree{nil, map[string]*bintree{ - "alert.html": &bintree{uiAppPartialsAlertHtml, map[string]*bintree{}}, - "alerts.html": &bintree{uiAppPartialsAlertsHtml, map[string]*bintree{}}, - "route.html": &bintree{uiAppPartialsRouteHtml, map[string]*bintree{}}, + "alert.html": &bintree{uiAppPartialsAlertHtml, map[string]*bintree{}}, + "alerts.html": &bintree{uiAppPartialsAlertsHtml, map[string]*bintree{}}, + "route.html": &bintree{uiAppPartialsRouteHtml, map[string]*bintree{}}, "silence-form.html": &bintree{uiAppPartialsSilenceFormHtml, map[string]*bintree{}}, - "silence.html": &bintree{uiAppPartialsSilenceHtml, map[string]*bintree{}}, - "silences.html": &bintree{uiAppPartialsSilencesHtml, map[string]*bintree{}}, - "status.html": &bintree{uiAppPartialsStatusHtml, map[string]*bintree{}}, + "silence.html": &bintree{uiAppPartialsSilenceHtml, map[string]*bintree{}}, + "silences.html": &bintree{uiAppPartialsSilencesHtml, map[string]*bintree{}}, + "status.html": &bintree{uiAppPartialsStatusHtml, map[string]*bintree{}}, }}, }}, "bindata.go": &bintree{uiBindataGo, map[string]*bintree{}}, "lib": &bintree{nil, map[string]*bintree{ - "angular-moment.min.js": &bintree{uiLibAngularMomentMinJs, map[string]*bintree{}}, + "angular-moment.min.js": &bintree{uiLibAngularMomentMinJs, map[string]*bintree{}}, "angular-resource.min.js": &bintree{uiLibAngularResourceMinJs, map[string]*bintree{}}, - "angular-route.min.js": &bintree{uiLibAngularRouteMinJs, map[string]*bintree{}}, + "angular-route.min.js": &bintree{uiLibAngularRouteMinJs, map[string]*bintree{}}, "angular-sanitize.min.js": &bintree{uiLibAngularSanitizeMinJs, map[string]*bintree{}}, - "angular.min.js": &bintree{uiLibAngularMinJs, map[string]*bintree{}}, - "d3.v3.min.js": &bintree{uiLibD3V3MinJs, map[string]*bintree{}}, - "jquery.min.js": &bintree{uiLibJqueryMinJs, map[string]*bintree{}}, - "js-yaml.min.js": &bintree{uiLibJsYamlMinJs, map[string]*bintree{}}, - "kube.min.css": &bintree{uiLibKubeMinCss, map[string]*bintree{}}, - "moment.min.js": &bintree{uiLibMomentMinJs, map[string]*bintree{}}, - "routing-tree.js": &bintree{uiLibRoutingTreeJs, map[string]*bintree{}}, + "angular.min.js": &bintree{uiLibAngularMinJs, map[string]*bintree{}}, + "d3.v3.min.js": &bintree{uiLibD3V3MinJs, map[string]*bintree{}}, + "jquery.min.js": &bintree{uiLibJqueryMinJs, map[string]*bintree{}}, + "js-yaml.min.js": &bintree{uiLibJsYamlMinJs, map[string]*bintree{}}, + "kube.min.css": &bintree{uiLibKubeMinCss, map[string]*bintree{}}, + "moment.min.js": &bintree{uiLibMomentMinJs, map[string]*bintree{}}, + "routing-tree.js": &bintree{uiLibRoutingTreeJs, map[string]*bintree{}}, }}, "web.go": &bintree{uiWebGo, map[string]*bintree{}}, }}, @@ -750,4 +751,3 @@ func _filePath(dir, name string) string { cannonicalName := strings.Replace(name, "\\", "/", -1) return filepath.Join(append([]string{dir}, strings.Split(cannonicalName, "/")...)...) } -