Add an ability to load an arbitrary list of Cortex modules #3275

blind-oracle · 2020-10-03T11:17:41Z

Signed-off-by: Igor Novgorodov [email protected]

What this PR does:
Adds an ability to load a comma-separated list of modules instead of a single module or 'all'.
This should be backwards-compatible with the current behaviour.

The moduleManager is extended a bit to keep track of already loaded modules.
I don't really like having All as a module, but I'll leave it for now as-is to keep changes compact.
And single-binary detection depends on this in a couple of places.
Ideally we should just alias the word all to a predefined list of modules and load them like they were specified on the command line or in config file.

Which issue(s) this PR fixes:
Fixes #3272

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Igor Novgorodov <[email protected]>

pracucci

Thanks for working on this! I left few comments. @pstibrany what do you think?

pracucci · 2020-10-06T16:04:21Z

CHANGELOG.md

+* [FEATURE] Implement an ability to load multiple Cortex modules using a comma-separated list. #3272
+  - Should be backwards compatible with the current behavior.
+  - Target list like 'all,compactor' can be used to load the single-binary modules plus some other modules.


I suggest a little rephrasing to simplify it:

Suggested change

* [FEATURE] Implement an ability to load multiple Cortex modules using a comma-separated list. #3272

- Should be backwards compatible with the current behavior.

- Target list like 'all,compactor' can be used to load the single-binary modules plus some other modules.

* [ENHANCEMENT] Allow to specify multiple comma-separated Cortex services to `-target` CLI option (or its respective YAML config option). For example, `-target=all,compactor` can be used to start Cortex single-binary with compactor as well. #3272

We consider these improvements "enhancement" and not new core features. Please move the changelog entry to the right place (entries are grouped by type).

Sure, forgot that they're grouped.

pracucci · 2020-10-06T16:14:23Z

pkg/cortex/modules.go

 	// whenever it's not running as an internal dependency (ie. querier or
 	// ruler's dependency)
-	canJoinDistributorsRing := (t.Cfg.Target == All || t.Cfg.Target == Distributor)
+	canJoinDistributorsRing := t.Cfg.Modules[Distributor] || t.Cfg.Modules[All]


You should do a similar change in initAlertManager() too.

pracucci · 2020-10-06T16:16:18Z

pkg/cortex/cortex.go

-	PrintConfig bool   `yaml:"-"`
-	HTTPPrefix  string `yaml:"http_prefix"`
+	Target      string          `yaml:"target"`
+	Modules     map[string]bool `yaml:"-"`


I think keeping both Target string and the new Modules map[string]bool may lead to bugs in the future. We may forget to check Modules instead of Target. A better option may be define a CLI flag custom type in pkg/util/flagext which is a []string but implements flag.Value and yaml.Marshaller/Unmarshaller to marshal/unmarshal a comma separated string.

We wouldn't have a map, but checking if a module is within the []string would be as easy as calling util.StringsContain().

What do you think?

There's already flagext.StringSlice, should I just use it?

flagext.StringSlice doesn't support comma-separated argument, only argument with multiple flags. I'm afraid that would be incompatible change in YAML config.

pracucci · 2020-10-06T16:29:05Z

pkg/util/modules/modules.go

 	modules map[string]*module
+
+	// Modules that are already initialized
+	modulesLoaded map[string]bool


I would rename "loaded" into "initialised", which is the naming we use here.

pracucci · 2020-10-06T16:29:18Z

pkg/util/modules/modules.go

 }

+// GetServicesMap returns services map
+func (m *Manager) GetServicesMap() map[string]services.Service {


We may call it GetInitialisedServices() and update the comment accordingly.

pracucci · 2020-10-06T16:32:26Z

pkg/util/modules/modules.go


 	for ix, n := range deps {
+		// Skip already loaded modules
+		if m.modulesLoaded[n] {


Couldn't we just check m.servicesMap?

That wouldn't work... not all modules have a service.

pstibrany

Thank you for working on this! I suggest we keep the changes to modules.Manager backwards compatible and not introduce new public API to it (it's always easy to add new methods, and difficult to remove them once they get used), since there are several other projects using this other than Cortex.

pstibrany · 2020-10-07T11:21:37Z

pkg/util/modules/modules.go

-func (m *Manager) InitModuleServices(target string) (map[string]services.Service, error) {
-	if _, ok := m.modules[target]; !ok {
-		return nil, fmt.Errorf("unrecognised module name: %s", target)
+func (m *Manager) InitModuleServices(name string) error {


Could we make it work with this signature instead?

Suggested change

func (m *Manager) InitModuleServices(name string) error {

func (m *Manager) InitModuleServices(names ...string) (map[string]services.Service, error)

There are multiple projects using this system, and this is backwards compatible, while your suggestion isn't. Furthermore it keeps the state of initialization outside of this manager.

Sure, didn't know that anything else other than Cortex used it.

pstibrany · 2020-10-07T11:24:22Z

pkg/util/modules/modules.go

 }

+// IsModuleRegistered returns true if given module was registered
+func (m *Manager) IsModuleRegistered(mod string) bool {


Let's not introduce new public methods to manager, that are not needed externally.

Signed-off-by: Igor Novgorodov <[email protected]>

blind-oracle · 2020-10-07T15:43:58Z

Thanks for the comments everyone.
I've tried to take all of them into account and improve accordingly:

I've created a new type flagext.StringSliceCSV which implements serialisation to/from comma-separated string + tests.
I also had to add a workaround for this type to the doc-generator as it considers it a "list of strings".
Reverted the API of module loader to be backwards-compatible without extra public methods
Workaround Jaeger name generation as before it was suffixed by Target.
Now it will be suffixed only if there's only one module listed in Target, otherwise it will be just cortex. This should be backwards-compatible also.
Fixed the CHANGELOG

pstibrany

Thank you for addressing my feedback! I have left more comments in the spirit of “minimize state”.

pkg/cortex/cortex.go

pkg/util/modules/modules.go

Signed-off-by: Igor Novgorodov <[email protected]>

pstibrany

Thank you a lot for your patience. This is on the right track, few more minor nits.

pstibrany · 2020-10-08T07:49:39Z

pkg/cortex/cortex.go

+	f.Var((*flagext.StringSliceCSV)(&c.Target), "target", "List of Cortex modules to load, comma separated. "+
+		"The alias 'all' can be used in the list to load a number of core modules and will enable single-binary mode. "+
+		"Use '-modules' command line flag to get a list of available modules, and to see which modules are included in 'all'.")
+


Suggested change

f.Var((*flagext.StringSliceCSV)(&c.Target), "target", "List of Cortex modules to load, comma separated. "+

"The alias 'all' can be used in the list to load a number of core modules and will enable single-binary mode. "+

"Use '-modules' command line flag to get a list of available modules, and to see which modules are included in 'all'.")

f.Var(&c.Target, "target", "Comma-separated list of Cortex modules to load. "+

"The alias 'all' can be used in the list to load a number of core modules and will enable single-binary mode. "+

"Use '-modules' command line flag to get a list of available modules, and to see which modules are included in 'all'.")

pstibrany · 2020-10-08T07:53:43Z

pkg/cortex/cortex.go

+	// Set the default module list to 'all'
+	// Make linter happy
+	c.Target.Set(All) //nolint:errcheck
+


Suggested change

// Set the default module list to 'all'

// Make linter happy

c.Target.Set(All) //nolint:errcheck

c.Target = []string{All}

pkg/cortex/cortex.go

pstibrany · 2020-10-08T08:04:40Z

pkg/util/modules/modules_test.go

-	svcs, err = mm.InitModuleServices("service_unknown")
-	assert.Nil(t, svcs)
-	assert.Error(t, err, fmt.Errorf("unrecognised module name: service_unknown"))
+	// Test loading of the module second time (should be noop)


It should not be noop -- it should in fact create new services. Can we extend the test to verify that services from first and second invocation of mm.InitModuleServices("serviceC") are different?

Yeah, the noop part is left from the previous stateful ModuleManager. Fixed

…, update docs Signed-off-by: Igor Novgorodov <[email protected]>

Signed-off-by: Igor Novgorodov <[email protected]>

blind-oracle · 2020-10-08T10:42:33Z

Integration test failed, but looking at the logs it looks like some other transient issue.

pstibrany

I have left some non-blocking nitpicky comments (mostly to minimize the diff), but LGTM. Thank you!

pkg/cortex/cortex.go

pkg/cortex/cortex_test.go

pkg/util/modules/modules.go

pkg/cortex/cortex_test.go

Signed-off-by: Igor Novgorodov <[email protected]>

blind-oracle · 2020-10-08T16:00:24Z

Done mentioned fixes

pstibrany

Thank you very much!

Signed-off-by: Igor Novgorodov <[email protected]>

blind-oracle · 2020-10-08T17:45:52Z

There's a conflict with newer PR #3287, I've merged current master and fixed.

blind-oracle · 2020-10-08T18:12:03Z

Looks like tests are unstable.
I also occasionally get failed etcd-related tests when running go test ./... locally.

But now it's something else:

--- FAIL: TestShuffleShardWithCaching (5.03s)
    ring_test.go:790: lifecycler 0 failed: failed to pick tokens in the KV store, ring: test: failed to CAS test
    ring_test.go:834: 6 != 5
FAIL

pstibrany · 2020-10-08T18:23:19Z

Looks like tests are unstable.
I also occasionally get failed etcd-related tests when running go test ./... locally.

Yes, this test is flaky. We need to fix it.

CHANGELOG.md

Signed-off-by: Marco Pracucci <[email protected]>

pkg/cortex/cortex_test.go

Signed-off-by: Marco Pracucci <[email protected]>

pracucci

You did great! LGTM! Will merge as soon as the CI passes.

Signed-off-by: Marco Pracucci <[email protected]>

Add support for by explictely specifying a target in the servcies config. cortexproject/cortex#3275 Signed-off-by: Julien Pivotto <[email protected]>

* Update cortex and add support for multiple modules Add support for by explictely specifying a target in the servcies config. cortexproject/cortex#3275 Signed-off-by: Julien Pivotto <[email protected]>

Add an ability to load an arbitrary list of Cortex modules

6c2e3c3

Signed-off-by: Igor Novgorodov <[email protected]>

pull-request-size bot added the size/L label Oct 3, 2020

blind-oracle mentioned this pull request Oct 3, 2020

Add an ability to run mixed number of modules in one instance #3272

Closed

Fix module loading logic

e3b67b2

Signed-off-by: Igor Novgorodov <[email protected]>

pracucci reviewed Oct 6, 2020

View reviewed changes

pstibrany reviewed Oct 7, 2020

View reviewed changes

blind-oracle added 3 commits October 7, 2020 16:57

Improve multiple module loading

3fb005c

Signed-off-by: Igor Novgorodov <[email protected]>

Add forgotten files

1228912

Signed-off-by: Igor Novgorodov <[email protected]>

Fix linter errors

b83369b

Signed-off-by: Igor Novgorodov <[email protected]>

pstibrany reviewed Oct 7, 2020

View reviewed changes

pkg/cortex/cortex.go Show resolved Hide resolved

pkg/util/modules/modules.go Outdated Show resolved Hide resolved

Reduce state in ModuleManager

53190a1

Signed-off-by: Igor Novgorodov <[email protected]>

pstibrany reviewed Oct 8, 2020

View reviewed changes

blind-oracle added 2 commits October 8, 2020 11:57

Fix pkg/cortex & pkg/util/modules tests, make initModules() stateless…

5d98caa

…, update docs Signed-off-by: Igor Novgorodov <[email protected]>

Fix missed mess with &c.Target pointer cast

a3613ca

Signed-off-by: Igor Novgorodov <[email protected]>

pstibrany approved these changes Oct 8, 2020

View reviewed changes

pkg/cortex/cortex.go Outdated Show resolved Hide resolved

pkg/cortex/cortex_test.go Outdated Show resolved Hide resolved

pkg/util/modules/modules.go Outdated Show resolved Hide resolved

pkg/cortex/cortex_test.go Outdated Show resolved Hide resolved

Cosmetic changes to minimize diff

4bb0dad

Signed-off-by: Igor Novgorodov <[email protected]>

pstibrany approved these changes Oct 8, 2020

View reviewed changes

blind-oracle added 2 commits October 8, 2020 19:42

Merge branch 'master' into module-separate

6d035cd

Fix problems with PR cortexproject#3287

dfc92da

Signed-off-by: Igor Novgorodov <[email protected]>

pracucci reviewed Oct 9, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

80282b7

Signed-off-by: Marco Pracucci <[email protected]>

pracucci reviewed Oct 9, 2020

View reviewed changes

pkg/cortex/cortex_test.go Outdated Show resolved Hide resolved

Update pkg/cortex/cortex_test.go

0875ecc

Signed-off-by: Marco Pracucci <[email protected]>

pracucci approved these changes Oct 9, 2020

View reviewed changes

Fixed linter

e64d873

Signed-off-by: Marco Pracucci <[email protected]>

pracucci merged commit 1734c75 into cortexproject:master Oct 9, 2020

blind-oracle deleted the module-separate branch October 9, 2020 12:18

roidelapluie mentioned this pull request Nov 17, 2020

Update cortex and add support for multiple modules cloudalchemy/ansible-cortex#8

Merged

	func (m *Manager) InitModuleServices(name string) error {
	func (m *Manager) InitModuleServices(names ...string) (map[string]services.Service, error)

Add an ability to load an arbitrary list of Cortex modules #3275

Add an ability to load an arbitrary list of Cortex modules #3275

Uh oh!

Conversation

blind-oracle commented Oct 3, 2020

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blind-oracle commented Oct 7, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blind-oracle commented Oct 8, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blind-oracle commented Oct 8, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

blind-oracle commented Oct 8, 2020

Uh oh!

blind-oracle commented Oct 8, 2020

Uh oh!

pstibrany commented Oct 8, 2020

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment