Skip to content

Commit

Permalink
chore: rebase feature/shutdownscrape on upstream/main
Browse files Browse the repository at this point in the history
This change does the following:
 - Rebase the feature branch on top of main to get rid of some vulns and
   some dependency issues that popped up when upgrading the cloud run
   sidecar
 - Fix tests by adding more robust and clear instructions on when to
   notify downstream managers of changes to the discovered targets.

Signed-off-by: Ridwan Sharif <ridwanmsharif@google.com>
  • Loading branch information
ridwanmsharif committed Oct 8, 2024
1 parent 477bf6a commit c9a4070
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 7 deletions.
27 changes: 22 additions & 5 deletions discovery/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,14 @@ func (m *Manager) ApplyConfig(cfg map[string]Configs) error {
m.metrics.FailedConfigs.Set(float64(failedCount))

var (
keep bool
// The updater goroutine is responsible for gathering updates to the
// tsets and notifying the manager but in some cases, the reload will
// update the tsets as well (for example: providers change during a
// reload and old targets need to be cleared).
// shouldNotify tracks whether this call to ApplyConfig updates the
// targets and is used to determine whether a notification to the
// manager needs to be triggered.
shouldNotify bool
wg sync.WaitGroup
newProviders []*Provider
)
Expand All @@ -248,7 +255,12 @@ func (m *Manager) ApplyConfig(cfg map[string]Configs) error {

m.targetsMtx.Lock()
for s := range prov.subs {
keep = true
// Since we have existing subs, and have new subs as well - we are
// going to be making changes to the targets map (deleting obsolete
// subs' targets and setting new subs' targets). Therefore,
// regardless of the discoverer we should notify the downstream
// managers so they can update the full target state.
shouldNotify = true
refTargets = m.targets[poolKey{s, prov.name}]
// Remove obsolete subs' targets.
if _, ok := prov.newSubs[s]; !ok {
Expand Down Expand Up @@ -277,13 +289,18 @@ func (m *Manager) ApplyConfig(cfg map[string]Configs) error {
m.startProvider(m.ctx, prov)
}
}

// This also helps making the downstream managers drop stale targets as soon as possible.
// See https://github.com/prometheus/prometheus/pull/13147 for details.
if !reflect.DeepEqual(m.providers, newProviders) {
shouldNotify = true
}

// Currently downstream managers expect full target state upon config reload, so we must oblige.
// While startProvider does pull the trigger, it may take some time to do so, therefore
// we pull the trigger as soon as possible so that downstream managers can populate their state.
// See https://github.com/prometheus/prometheus/pull/8639 for details.
// This also helps making the downstream managers drop stale targets as soon as possible.
// See https://github.com/prometheus/prometheus/pull/13147 for details.
if keep || len(m.providers) > 0 {
if shouldNotify {
select {
case m.triggerSend <- struct{}{}:
default:
Expand Down
6 changes: 4 additions & 2 deletions scrape/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,9 @@ func TestManagerTargetsUpdates(t *testing.T) {

func TestManagerSkipInitialWait(t *testing.T) {
opts := Options{DiscoveryReloadOnStartup: true}
m := NewManager(&opts, nil, nil)
testRegistry := prometheus.NewRegistry()
m, err := NewManager(&opts, nil, nil, nil, testRegistry)
require.NoError(t, err)

ts := make(chan map[string][]*targetgroup.Group, 1)
go m.Run(ts)
Expand Down Expand Up @@ -1445,7 +1447,7 @@ scrape_configs:
t,
scrapeManager,
jobName,
false,
true,
nil,
)
}
Expand Down

0 comments on commit c9a4070

Please sign in to comment.