Refactored limits to be able to define them dynamically when Cortex is vendored in another project

[sandeepsukhani]

This would be useful to define limits specific to Loki while still being able to use the same code for managing limits and per tenant overwrites.

Changes include:

1. Limits are now stored as key-value pairs, where the key is a string and values are interfaces for being able to support dynamic limits
2. Limits need to be registered before being able to use them
3. Sample and Labels validation methods now accept any type which implements required methods to get required limits for validation
4. Added Setter methods to change default limit values at runtime for testing purpose

Signed-off-by: Sandeep Sukhani sandeep.d.sukhani@gmail.com

[bboreham]

Can you say a bit more about what has been done in the 800 lines of changes?
It looks like some words were renamed - does this relate to making things dynamic?

[sandeepsukhani]

Exact usecase is being able to manage limits in Loki using same code as cortex. Loki uses some of the limits from Cortex and few additional ones specific to Loki.
pkg/util/limits/limits.go is to manage limit types and per tenant overwrites.
pkg/util/limits/validation.go provides methods to access specific limits.
With this change Loki would use code for managing limits from limits.go and provide its own methods for specific limits.

[bboreham]

For instance, it looks like you changed the package name "validation" to "limits".
Is that true? If so, what was the reason?
Please ensure that your commit message describes all the changes made.

[sandeepsukhani]

what was the reason?

Actually, I wrote a new package with the name "limits" because most of its use case was managing and accessing limits. I had copied parts of code from the previous package so it looks like just a rename.
If you feel that it made more sense with the previous name, then I can rename it to validation.

Please ensure that your commit message describes all the changes made.

Sure will do and will make sure I take care of them going forward. Sorry about that.

[bboreham]

This isn't about the name, it's about you explaining what you did so it can be reviewed.
As it stands it's an 800+ line change. Maybe if you didn't rename so much it would be a 100-line change and much easier to review.

[sandeepsukhani]

@bboreham renamed and refactored the code to make the changes minimal. Please let me know if it looks good or whether I can do any more improvements to it.
I have also updated commit message and description with the changes that I have done.

[gouthamve]

LGTM

[sandeepsukhani]

@bboreham @tomwilkie Can you please have a look at this and suggest any changes if required to get another approval?

[tomwilkie]

As discussed offline, not super happy with the amount of strings here. Quick sketch of an alternative:

package foo

type Limits interface {
    RegisterFlags()
    UnmarhsalYAML()
}

type Overrides struct {
    factory    func() Limits
    defaults   Limits
    overrides map[string]Limits
}

func (o Overrides) register() {]}
    o.defaults = o.factory()
    o.defaults.RegisterFlags()
}


func (o *Overrides) getFloat64(userID string, f func(*Limits) float64) float64 {
	o.overridesMtx.RLock()
	defer o.overridesMtx.RUnlock()
	override, ok := o.overrides[userID]
	if !ok {
		return f(&o.Defaults)
	}
	return f(override)
}

func loop() {
    // load the overrides file in the map above.
}

// Cortex 

type CortexLimits struct {
    IngestionRate          float64       `yaml:"ingestion_rate"`
    ...
}

type CortexOverrider struct {
    Overrides
}

func (o *CortexOverrider) IngestionRate(userID string) float64 {
	return o.getFloat64(userID, func(l *Limits) float64 {
        cl := l.(CortexLimits)
		return l.IngestionRate
	})
}

// Loki

type LokiLimits struct {
    ....
}

type LokiOverrider astruct {
    Overrides
}

[sandeepsukhani]

Without this values were getting reset to 0 while loading cortex config which had just per tenant overrides config path set.

[tomwilkie]

Why did this not happen before?

[tomwilkie]

@sandlis explained offline:

1. we init the flags, which sets up the default in a Limits struct in cortex pkg.
2. we then load YAML, which will overwrite them with 0s, as defaultLimits has not been inited.
3. we Parse the flags, which set only ones we've specifices
4. we then set this defaultLimits to the half-zero'd struct.

[gouthamve]

LGTM besides comment. Will take another pass though.

[rfratto]

LGTM

[tomwilkie]

I think this would be nicer as a config struct, not individual args.

[sandeepsukhani]

@tomwilkie Thanks for the review! Done the suggested changes.
Waiting for a fix for broken modules. Will rebase this when it is merged in master to run the checks.

[tomwilkie]

Can we make this take a "factory" for the Limits type? Then have the code to make a map vs the defaults would be in override.go.

I think this is possible with yaml.v3 and Node: https://godoc.org/gopkg.in/yaml.v3

[tomwilkie]

As discussed offline, Node.Decode doesn't honour KnownFields: go-yaml/yaml#460

[tomwilkie]

Yeah this is looking much better. Just a couple of nits, should be able to merge really soon now.