Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[file-sd-part-1] Added file sd to query #546

Merged
merged 1 commit into from
Oct 17, 2018
Merged

Conversation

ivan-valkov
Copy link

@ivan-valkov ivan-valkov commented Oct 1, 2018

ref: #492

Changes

Added file sd to query. You can specify files to watch via a command line flag. Then the querier will load stores' addresses from the files whenever the files are updated.

We are reusing Prometheus' file sd.

Verification

Tested in a following PR that adds the different SD options to the tests - (#554)

@ivan-valkov ivan-valkov force-pushed the file-sd-query branch 3 times, most recently from df34473 to 6c9aca9 Compare October 5, 2018 10:23
@ivan-valkov ivan-valkov self-assigned this Oct 5, 2018
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, thanks! So EXCITED

@@ -64,6 +66,9 @@ func registerQuery(m map[string]setupFunc, app *kingpin.Application, name string
stores := cmd.Flag("store", "Addresses of statically configured store API servers (repeatable).").
PlaceHolder("<store>").Strings()

filesToWatch := cmd.Flag("filesd", "Path to file that contain addresses of store API servers (repeatable).").
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note something about globbing option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

store-sd-files

@@ -241,6 +258,15 @@ func runQuery(

specs = append(specs, &gossipSpec{id: id, addr: ps.StoreAPIAddr, peer: peer})
}

addrFromFileSD.mtx.Lock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not doing just addrFromFileSD.Lock()

@@ -264,6 +290,48 @@ func runQuery(
stores.Close()
})
}
// Run File Service Discovery and update the store set when the files are modified
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed trailing period

@@ -264,6 +290,48 @@ func runQuery(
stores.Close()
})
}
// Run File Service Discovery and update the store set when the files are modified
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if fileSD != nil {

{
if fileSD != nil {
var fileSDUpdates chan *discovery.Discoverable
ctx, cancel := context.WithCancel(context.Background())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never share contexts between two run groups

continue
}
// TODO(ivan): resolve dns here maybe?
addrFromFileSD.update(update.Source, update.Services)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`s/addrFromFileSD/cache

}
}, func(error) {
cancel()
stores.Close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it was closed before?

continue
}
// TODO(ivan): resolve dns here maybe?
addrFromFileSD.update(update.Source, update.Services)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate for dups?

@ivan-valkov ivan-valkov changed the base branch from file-sd to master October 9, 2018 15:42
@ivan-valkov ivan-valkov changed the title Added file sd to query [file-sd-part-1] Added file sd to query Oct 10, 2018
Copy link
Contributor

@domgreen domgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 like the reuse of Prom code :)

@@ -271,6 +302,46 @@ func runQuery(
stores.Close()
})
}
// Run File Service Discovery and update the store set when the files are modified.
if fileSD != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit - did we want to put this in a block so we can keep ctx, cancel as we have done in other blocks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in block, thanks for if (: right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah its fine, i didnt think about the indenting

@@ -129,6 +129,10 @@ Flags:
info endpoint (repeated).
--store=<store> ... Addresses of statically configured store API
servers (repeatable).
--store-sd-file=<path> ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - would change this to match prom file_sd_config https://prometheus.io/docs/prometheus/latest/configuration/configuration/#file_sd_config so maybe store.file-sd-config?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could agree

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions

@@ -87,6 +95,15 @@ func registerQuery(m map[string]setupFunc, app *kingpin.Application, name string
lookupStores[s] = struct{}{}
}

var filesd *file.Discovery
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/filesd/fileSD/ maybe?

@@ -248,6 +271,14 @@ func runQuery(

specs = append(specs, &gossipSpec{id: id, addr: ps.StoreAPIAddr, peer: peer})
}

// Add store specs from file sd.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/sd/SD

specs = append(specs, query.NewGRPCStoreSpec(addr))
}

specs = removeDuplicates(specs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove or we want to fail maybe?

@@ -271,6 +302,46 @@ func runQuery(
stores.Close()
})
}
// Run File Service Discovery and update the store set when the files are modified.
if fileSD != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in block, thanks for if (: right?

if update == nil {
continue
}
// TODO(ivan): resolve dns here maybe?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this comment is not needed here? Because further PRs are rdy?

}
}, func(error) {
cancelUpdate()
close(fileSDUpdates)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm.. Can we ensure we close this ONLY in one place? I feel like both fileSD and this guy closes this, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File SD doesn't close it. It is responsibility of the one who passes the channel to file SD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so why this comment exists?

// Handle the case that a discoverer exits and closes the channel
					// before the context is done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, good question. I think this is not needed then. I will remove it. I double checked again but file sd doesn't close this channel.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify, the check when receiving from the channel is not needed.

@@ -339,6 +410,17 @@ func runQuery(
level.Info(logger).Log("msg", "starting query node")
return nil
}
func removeDuplicates(specs []query.StoreSpec) []query.StoreSpec {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline

log line is must have - if not error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really error out

defer f.Unlock()
for _, tg := range tgs {
// Some Discoverers send nil target group so need to check for it to avoid panics.
if tg != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter out wrong cases to avoid indents and have cleaner code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean something like this:

if tg == nil {
    continue
}
f.tgs[tg.Source] = tg

@@ -129,6 +129,10 @@ Flags:
info endpoint (repeated).
--store=<store> ... Addresses of statically configured store API
servers (repeatable).
--store-sd-file=<path> ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could agree

}

// NewCache returns a new empty Cache.
func NewCache() *Cache {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - a style preference I have would be to make it pkg/discovery/cache/cache.go which will make it more readable in store.go as it allows cache.New().

if len(*fileSDFiles) > 0 {
conf := &file.SDConfig{
Files: *fileSDFiles,
RefreshInterval: *fileSDInterval,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only thing i can think of here is what if someone does < 1s? or not a valid duration?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah, it is possible to shoot yourself in the foot with this. However, I would argue that if you are using this flag at all you should know what you are doing? What do you think? We can default to 5s if interval < 5s?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let leave it for the moment, we have a sensible default 👍

PlaceHolder("<path>").Strings()

fileSDInterval := modelDuration(cmd.Flag("store.file-sd-config.interval", "Refresh interval to re-read file SD files. (used as a fallback)").
Default("5s"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this should be "5m" 👍

Copy link
Contributor

@domgreen domgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this whole chain of PRs 👍

if len(*fileSDFiles) > 0 {
conf := &file.SDConfig{
Files: *fileSDFiles,
RefreshInterval: *fileSDInterval,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let leave it for the moment, we have a sensible default 👍

Addressed PR comments

More pr comments addressed

Added logs and metrics when removing dups

Removed redundant check

Added flag for file sd refresh interval

Changed default fileSD interval to 5m

make docs

[file-sd-part-2] Query e2e test uses different service discovery options (#554)

* Query e2e tests different sd options
@ivan-valkov ivan-valkov merged commit 518e851 into master Oct 17, 2018
@ivan-valkov ivan-valkov deleted the file-sd-query branch October 17, 2018 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants