-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Loki: Allow configuring query_store_max_look_back_period when running a filesystem store and boltdb-shipper #2073
Changes from 3 commits
b59b88f
56fe4e2
820f909
46b495f
af2ac6d
e9442ea
c15f5a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,7 +64,7 @@ type Config struct { | |
ingesterClientFactory func(cfg client.Config, addr string) (client.HealthAndIngesterClient, error) | ||
|
||
QueryStore bool `yaml:"-"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can maybe get rid of this because now we mostly rely on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems correct what @sandeepsukhani is saying ? |
||
QueryStoreMaxLookBackPeriod time.Duration `yaml:"-"` | ||
QueryStoreMaxLookBackPeriod time.Duration `yaml:"query_store_max_look_back_period"` | ||
} | ||
|
||
// RegisterFlags registers the flags. | ||
|
@@ -84,6 +84,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | |
f.Float64Var(&cfg.SyncMinUtilization, "ingester.sync-min-utilization", 0, "Minimum utilization of chunk when doing synchronization.") | ||
f.IntVar(&cfg.MaxReturnedErrors, "ingester.max-ignored-stream-errors", 10, "Maximum number of ignored stream errors to return. 0 to return all errors.") | ||
f.DurationVar(&cfg.MaxChunkAge, "ingester.max-chunk-age", time.Hour, "Maximum chunk age before flushing.") | ||
f.DurationVar(&cfg.QueryStoreMaxLookBackPeriod, "ingester.query-store-max-look-back-period", 0, "How far back should an ingester be allowed to query the store for data, for use only with boltdb-shipper index and filesystem object store. -1 for infinite.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the requirement should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If my above assumption is true, that the file names are unique, I specified With the exception of one other reason I could think of, it would be easier to take backups or even move the Loki to another host because the index files can be directly copied with the chunks directory while Loki is still running, I don't think this is true for |
||
} | ||
|
||
// Ingester builds chunks for incoming log streams. | ||
|
@@ -414,7 +415,7 @@ func buildStoreRequest(cfg Config, req *logproto.QueryRequest) *logproto.QueryRe | |
} | ||
start := req.Start | ||
end := req.End | ||
if cfg.QueryStoreMaxLookBackPeriod != 0 { | ||
if cfg.QueryStoreMaxLookBackPeriod > 0 { | ||
oldestStartTime := time.Now().Add(-cfg.QueryStoreMaxLookBackPeriod) | ||
if oldestStartTime.After(req.Start) { | ||
start = oldestStartTime | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package loki | ||
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"net/http" | ||
"os" | ||
|
@@ -198,10 +199,14 @@ func (t *Loki) initIngester() (_ services.Service, err error) { | |
t.cfg.Ingester.LifecyclerConfig.ListenPort = t.cfg.Server.GRPCListenPort | ||
|
||
// We want ingester to also query the store when using boltdb-shipper | ||
if activeIndexType(t.cfg.SchemaConfig) == local.BoltDBShipperType { | ||
pc := activePeriodConfig(t.cfg.SchemaConfig) | ||
if pc.IndexType == local.BoltDBShipperType { | ||
t.cfg.Ingester.QueryStore = true | ||
// When using shipper, limit max look back for query to MaxChunkAge + upload interval by shipper + 15 mins to query only data whose index is not pushed yet | ||
t.cfg.Ingester.QueryStoreMaxLookBackPeriod = t.cfg.Ingester.MaxChunkAge + local.ShipperFileUploadInterval + (15 * time.Minute) | ||
mlb, err := calculateMaxLookBack(pc, t.cfg.Ingester.QueryStoreMaxLookBackPeriod, t.cfg.Ingester.MaxChunkAge) | ||
if err != nil { | ||
return nil, err | ||
} | ||
t.cfg.Ingester.QueryStoreMaxLookBackPeriod = mlb | ||
} | ||
|
||
t.ingester, err = ingester.New(t.cfg.Ingester, t.cfg.IngesterClient, t.store, t.overrides) | ||
|
@@ -256,7 +261,7 @@ func (t *Loki) initTableManager() (services.Service, error) { | |
} | ||
|
||
func (t *Loki) initStore() (_ services.Service, err error) { | ||
if activeIndexType(t.cfg.SchemaConfig) == local.BoltDBShipperType { | ||
if activePeriodConfig(t.cfg.SchemaConfig).IndexType == local.BoltDBShipperType { | ||
t.cfg.StorageConfig.BoltDBShipperConfig.IngesterName = t.cfg.Ingester.LifecyclerConfig.ID | ||
switch t.cfg.Target { | ||
case Ingester: | ||
|
@@ -483,15 +488,35 @@ var modules = map[moduleName]module{ | |
}, | ||
} | ||
|
||
// activeIndexType type returns index type which would be applicable to logs that would be pushed starting now | ||
// activePeriodConfig type returns index type which would be applicable to logs that would be pushed starting now | ||
// Note: Another periodic config can be applicable in future which can change index type | ||
func activeIndexType(cfg chunk.SchemaConfig) string { | ||
func activePeriodConfig(cfg chunk.SchemaConfig) chunk.PeriodConfig { | ||
now := model.Now() | ||
i := sort.Search(len(cfg.Configs), func(i int) bool { | ||
return cfg.Configs[i].From.Time > now | ||
}) | ||
if i > 0 { | ||
i-- | ||
} | ||
return cfg.Configs[i].IndexType | ||
return cfg.Configs[i] | ||
} | ||
|
||
func calculateMaxLookBack(pc chunk.PeriodConfig, maxLookBackConfig, maxChunkAge time.Duration) (time.Duration, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That deserve a test and it seems simple. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol, yeah, i thought i put in the description or somewhere I wanted to test this but hadn't done it yet |
||
if pc.ObjectType != local.FilesystemObjectStoreType && maxLookBackConfig.Milliseconds() != 0 { | ||
return 0, errors.New("it is an error to specify a non zero `query_store_max_look_back_period` value when using any object store other than `filesystem`") | ||
} | ||
// When using shipper, limit max look back for query to MaxChunkAge + upload interval by shipper + 15 mins to query only data whose index is not pushed yet | ||
defaultMaxLookBack := maxChunkAge + local.ShipperFileUploadInterval + (15 * time.Minute) | ||
|
||
if maxLookBackConfig == 0 { | ||
// If the QueryStoreMaxLookBackPeriod is still it's default value of 0, set it to the default calculated value. | ||
return defaultMaxLookBack, nil | ||
} else if maxLookBackConfig > 0 && maxLookBackConfig < defaultMaxLookBack { | ||
// If the QueryStoreMaxLookBackPeriod is > 0 (-1 is allowed for infinite), make sure it's at least greater than the default or throw an error | ||
return 0, fmt.Errorf("the configured query_store_max_look_back_period of '%v' is less than the calculated default of '%v' "+ | ||
"which is calculated based on the max_chunk_age + 15 minute boltdb-shipper interval + 15 min additional buffer. Increase this value"+ | ||
"greater than the default or remove it from the configuration to use the default", maxLookBackConfig, defaultMaxLookBack) | ||
|
||
} | ||
return maxLookBackConfig, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the other instance would also have index files with same name so it would not be just copying of files. I guess we would have to provide a tool to merge boltdb files. We will have to make it clear in the docs until we have that tool.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each ingester should be writing files with a unique name such that you should be able to copy ones file to another without conflicts
However, I think I may have found a bug here, this is what I see for my current test ingesters, it looks like the lifecycler ID was empty when they created their files
These still wouldn't collide because the timestamps were different but they should also have the unique lifecycler ID in front
My suspicion here is they created the files before joining the ring? Can you take a look at this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at how this works, there is a real chicken and egg problem here when initializing the shipper and trying to get the ingester ID to use in the name file.
I'm not sure it's possible to do this, but maybe you have some ideas?
I'm wondering instead if we should try a different strategy and use the hostname or allow for the name to be configured in the yaml directly?