From 03adbe57e45fe675e6760527adc5b6b69a639107 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 14 Jun 2016 11:35:21 +0200 Subject: [PATCH 1/3] discovery/marathon: Fix race conditions in test The concurrency applied before is in most cases not even needed. With a cap=1 channel, most tests are much cleaner. TestMarathonSDRunAndStop was trickier. It could even have blocked before. This also includes a general refactoring of the whole file. --- retrieval/discovery/marathon/marathon_test.go | 261 +++++++++--------- 1 file changed, 137 insertions(+), 124 deletions(-) diff --git a/retrieval/discovery/marathon/marathon_test.go b/retrieval/discovery/marathon/marathon_test.go index 6400baded..6ee398881 100644 --- a/retrieval/discovery/marathon/marathon_test.go +++ b/retrieval/discovery/marathon/marathon_test.go @@ -24,108 +24,114 @@ import ( "github.com/prometheus/prometheus/config" ) -var marathonValidLabel = map[string]string{"prometheus": "yes"} +var ( + marathonValidLabel = map[string]string{"prometheus": "yes"} + testServers = []string{"http://localhost:8080"} +) -func newTestDiscovery(client AppListClient) (chan []*config.TargetGroup, *Discovery) { - ch := make(chan []*config.TargetGroup) - md := &Discovery{ - Servers: []string{"http://localhost:8080"}, +func testUpdateServices(client AppListClient, ch chan []*config.TargetGroup) error { + md := Discovery{ + Servers: testServers, Client: client, } - return ch, md + return md.updateServices(context.Background(), ch) } func TestMarathonSDHandleError(t *testing.T) { - var errTesting = errors.New("testing failure") - ch, md := newTestDiscovery(func(url string) (*AppList, error) { - return nil, errTesting - }) - go func() { - select { - case tg := <-ch: - t.Fatalf("Got group: %s", tg) - default: - } - }() - err := md.updateServices(context.Background(), ch) - if err != errTesting { + var ( + errTesting = errors.New("testing failure") + ch = make(chan []*config.TargetGroup, 1) + client = func(url string) (*AppList, error) { return nil, errTesting } + ) + if err := testUpdateServices(client, ch); err != errTesting { t.Fatalf("Expected error: %s", err) } + select { + case tg := <-ch: + t.Fatalf("Got group: %s", tg) + default: + } } func TestMarathonSDEmptyList(t *testing.T) { - ch, md := newTestDiscovery(func(url string) (*AppList, error) { - return &AppList{}, nil - }) - go func() { - select { - case tg := <-ch: - if len(tg) > 0 { - t.Fatalf("Got group: %v", tg) - } - default: - } - }() - err := md.updateServices(context.Background(), ch) - if err != nil { + var ( + ch = make(chan []*config.TargetGroup, 1) + client = func(url string) (*AppList, error) { return &AppList{}, nil } + ) + if err := testUpdateServices(client, ch); err != nil { t.Fatalf("Got error: %s", err) } + select { + case tg := <-ch: + if len(tg) > 0 { + t.Fatalf("Got group: %v", tg) + } + default: + } } func marathonTestAppList(labels map[string]string, runningTasks int) *AppList { - task := Task{ - ID: "test-task-1", - Host: "mesos-slave1", - Ports: []uint32{31000}, - } - docker := DockerContainer{Image: "repo/image:tag"} - container := Container{Docker: docker} - app := App{ - ID: "test-service", - Tasks: []Task{task}, - RunningTasks: runningTasks, - Labels: labels, - Container: container, - } + var ( + task = Task{ + ID: "test-task-1", + Host: "mesos-slave1", + Ports: []uint32{31000}, + } + docker = DockerContainer{Image: "repo/image:tag"} + container = Container{Docker: docker} + app = App{ + ID: "test-service", + Tasks: []Task{task}, + RunningTasks: runningTasks, + Labels: labels, + Container: container, + } + ) return &AppList{ Apps: []App{app}, } } func TestMarathonSDSendGroup(t *testing.T) { - ch, md := newTestDiscovery(func(url string) (*AppList, error) { - return marathonTestAppList(marathonValidLabel, 1), nil - }) - go func() { - select { - case tgs := <-ch: - tg := tgs[0] - - if tg.Source != "test-service" { - t.Fatalf("Wrong target group name: %s", tg.Source) - } - if len(tg.Targets) != 1 { - t.Fatalf("Wrong number of targets: %v", tg.Targets) - } - tgt := tg.Targets[0] - if tgt[model.AddressLabel] != "mesos-slave1:31000" { - t.Fatalf("Wrong target address: %s", tgt[model.AddressLabel]) - } - default: - t.Fatal("Did not get a target group.") + var ( + ch = make(chan []*config.TargetGroup, 1) + client = func(url string) (*AppList, error) { + return marathonTestAppList(marathonValidLabel, 1), nil } - }() - err := md.updateServices(context.Background(), ch) - if err != nil { + ) + if err := testUpdateServices(client, ch); err != nil { t.Fatalf("Got error: %s", err) } + select { + case tgs := <-ch: + tg := tgs[0] + + if tg.Source != "test-service" { + t.Fatalf("Wrong target group name: %s", tg.Source) + } + if len(tg.Targets) != 1 { + t.Fatalf("Wrong number of targets: %v", tg.Targets) + } + tgt := tg.Targets[0] + if tgt[model.AddressLabel] != "mesos-slave1:31000" { + t.Fatalf("Wrong target address: %s", tgt[model.AddressLabel]) + } + default: + t.Fatal("Did not get a target group.") + } } func TestMarathonSDRemoveApp(t *testing.T) { - ch, md := newTestDiscovery(func(url string) (*AppList, error) { - return marathonTestAppList(marathonValidLabel, 1), nil - }) - + var ( + ch = make(chan []*config.TargetGroup) + client = func(url string) (*AppList, error) { + return marathonTestAppList(marathonValidLabel, 1), nil + } + md = Discovery{ + Servers: testServers, + Client: client, + } + ) go func() { up1 := (<-ch)[0] up2 := (<-ch)[0] @@ -151,73 +157,80 @@ func TestMarathonSDRemoveApp(t *testing.T) { } func TestMarathonSDRunAndStop(t *testing.T) { - ch, md := newTestDiscovery(func(url string) (*AppList, error) { - return marathonTestAppList(marathonValidLabel, 1), nil - }) - md.RefreshInterval = time.Millisecond * 10 + var ( + ch = make(chan []*config.TargetGroup) + client = func(url string) (*AppList, error) { + return marathonTestAppList(marathonValidLabel, 1), nil + } + md = Discovery{ + Servers: testServers, + Client: client, + RefreshInterval: time.Millisecond * 10, + } + ) ctx, cancel := context.WithCancel(context.Background()) go func() { - select { - case <-ch: - cancel() - case <-time.After(md.RefreshInterval * 3): - cancel() - t.Fatalf("Update took too long.") + for { + select { + case _, ok := <-ch: + if !ok { + return + } + cancel() + case <-time.After(md.RefreshInterval * 3): + cancel() + t.Fatalf("Update took too long.") + } } }() md.Run(ctx, ch) - - select { - case <-ch: - default: - t.Fatalf("Channel not closed.") - } } func marathonTestZeroTaskPortAppList(labels map[string]string, runningTasks int) *AppList { - task := Task{ - ID: "test-task-2", - Host: "mesos-slave-2", - Ports: []uint32{}, - } - docker := DockerContainer{Image: "repo/image:tag"} - container := Container{Docker: docker} - app := App{ - ID: "test-service-zero-ports", - Tasks: []Task{task}, - RunningTasks: runningTasks, - Labels: labels, - Container: container, - } + var ( + task = Task{ + ID: "test-task-2", + Host: "mesos-slave-2", + Ports: []uint32{}, + } + docker = DockerContainer{Image: "repo/image:tag"} + container = Container{Docker: docker} + app = App{ + ID: "test-service-zero-ports", + Tasks: []Task{task}, + RunningTasks: runningTasks, + Labels: labels, + Container: container, + } + ) return &AppList{ Apps: []App{app}, } } func TestMarathonZeroTaskPorts(t *testing.T) { - ch, md := newTestDiscovery(func(url string) (*AppList, error) { - return marathonTestZeroTaskPortAppList(marathonValidLabel, 1), nil - }) - - go func() { - select { - case tgs := <-ch: - tg := tgs[0] - - if tg.Source != "test-service-zero-ports" { - t.Fatalf("Wrong target group name: %s", tg.Source) - } - if len(tg.Targets) != 0 { - t.Fatalf("Wrong number of targets: %v", tg.Targets) - } - default: - t.Fatal("Did not get a target group.") + var ( + ch = make(chan []*config.TargetGroup, 1) + client = func(url string) (*AppList, error) { + return marathonTestZeroTaskPortAppList(marathonValidLabel, 1), nil } - }() - err := md.updateServices(context.Background(), ch) - if err != nil { + ) + if err := testUpdateServices(client, ch); err != nil { t.Fatalf("Got error: %s", err) } + select { + case tgs := <-ch: + tg := tgs[0] + + if tg.Source != "test-service-zero-ports" { + t.Fatalf("Wrong target group name: %s", tg.Source) + } + if len(tg.Targets) != 0 { + t.Fatalf("Wrong number of targets: %v", tg.Targets) + } + default: + t.Fatal("Did not get a target group.") + } } From 4c864c8a88fb5b6190a4b2818ce1ddec2873c8c7 Mon Sep 17 00:00:00 2001 From: Fabian Reinartz Date: Tue, 14 Jun 2016 14:04:22 +0200 Subject: [PATCH 2/3] retrieval: don't sync to uninitialized scrape pool This change does just signal a scrape target update to the scraping loop once an initial target set is fetched. Before, the scrape pool was directly synced, causing a race against an uninitialized scrape pool. Fixes #1703 --- retrieval/targetmanager.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/retrieval/targetmanager.go b/retrieval/targetmanager.go index 2a25c48f2..cae910efe 100644 --- a/retrieval/targetmanager.go +++ b/retrieval/targetmanager.go @@ -331,7 +331,12 @@ func (ts *targetSet) runProviders(ctx context.Context, providers map[string]Targ // We wait for a full initial set of target groups before releasing the mutex // to ensure the initial sync is complete and there are no races with subsequent updates. wg.Wait() - ts.sync() + // Just signal that there are initial sets to sync now. Actual syncing must only + // happen in the runScraping loop. + select { + case ts.syncCh <- struct{}{}: + default: + } } // update handles a target group update from a target provider identified by the name. From a5b35498a870917d83293f565e05f8bf0f891adc Mon Sep 17 00:00:00 2001 From: Fabian Reinartz Date: Tue, 14 Jun 2016 14:19:31 +0200 Subject: [PATCH 3/3] *: bump version to 0.19.3 --- CHANGELOG.md | 5 +++++ VERSION | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2edb1a77..fdbbb541b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +## 0.19.3 / 2016-06-14 + +* [BUGFIX] Handle Marathon apps with zero ports +* [BUGFIX] Fix startup panic in retrieval layer + ## 0.19.2 / 2016-05-29 * [BUGFIX] Correctly handle `GROUP_LEFT` and `GROUP_RIGHT` without labels in diff --git a/VERSION b/VERSION index 61e6e92d9..b72b05ede 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.19.2 +0.19.3