Implement composite watchable config map provider #4391

tigrannajaryan · 2021-11-10T15:52:33Z

The new compositeMapProvider implements a generic map provider
that can be composed of subproviders and also implements a
watchable interface that can be triggered by any subprovider.

The ExpandMapProvider and MergeMapProvider are rewritten using
the compositeMapProvider with custom compose function.

This is necessary to implement file watching in fileMapProvider.
I have that part ready as well, but it won't work until the
providers that contain it are watchable (which this change adds).

There is a fair but of concurrent code in this change, it would
be great to have another pair of eyes to check I don't have
races and other bugs here.

tigrannajaryan · 2021-11-10T16:12:52Z

I will check the test failure, it probably caught a race bug.

tigrannajaryan · 2021-11-11T01:24:13Z

There is indeed a race bug, which the test caught. Fixing it and will also rebase.

bogdandrutu · 2021-11-11T01:32:34Z

config/configmapprovider/composite.go

+	return cr.composedMap
+}
+
+func (cr *compositeRetrieved) WatchForUpdate() error {


I think a much simpler implementation also after I did #4370 which solved few bugs, is to change the Provider interface and instead pass a "chan" or even better a func pointer:

// Provider is an interface that helps providing configuration's parser. // Implementations may load the parser from a file, a database or any other source. type Provider interface { // Retrieve goes to the configuration source and retrieves the selected data which // contains the value to be injected in the configuration and the corresponding watcher that // will be used to monitor for updates of the retrieved value. Retrieve(ctx context.Context, watcher func(Event{})) (Retrieved, error) // Close signals that the configuration for which it was used to retrieve values is no longer in use // and the object should close and release any watchers that it may have created. // This method must be called when the service ends, either in case of success or error. Shutdown(ctx context.Context) error } // Retrieved holds the result of a call to the Retrieve method of a Session object. type Retrieved interface { // Get returns the Map. Get() *config.Map // Close signals that the configuration for which it was used to retrieve values is no longer in use // and the object should close and release any watchers that it may have created. // Implementations should not call the "watcher" func after this func returned. Close(ctx context.Context) error }

@bogdandrutu I posted a PR that does this, see #4403
I like it, I believe it simplifies watching for changes. I suggest that we close this PR and continue further work with the callback approach.

The new compositeMapProvider implements a generic map provider that can be composed of subproviders and also implements a watchable interface that can be triggered by any subprovider. The ExpandMapProvider and MergeMapProvider are rewritten using the compositeMapProvider with custom compose function. This is necessary to implement file watching in fileMapProvider. I have that part ready as well, but it won't work until the providers that contain it are watchable (which this change adds). There is a fair but of concurrent code in this change, it would be great to have another pair of eyes to check I don't have races and other bugs here.

codecov · 2021-11-11T01:42:24Z

Codecov Report

Merging #4391 (b15ee6f) into main (93c9cab) will increase coverage by 0.11%.
The diff coverage is 95.42%.

@@            Coverage Diff             @@
##             main    #4391      +/-   ##
==========================================
+ Coverage   90.31%   90.42%   +0.11%     
==========================================
  Files         177      178       +1     
  Lines       10338    10459     +121     
==========================================
+ Hits         9337     9458     +121     
- Misses        779      780       +1     
+ Partials      222      221       -1

Impacted Files	Coverage Δ
config/configmapprovider/composite.go	`95.17% <95.17%> (ø)`
config/configmapprovider/expand.go	`70.73% <100.00%> (-5.27%)`	⬇️
config/configmapprovider/merge.go	`100.00% <100.00%> (+15.78%)`	⬆️
service/config_watcher.go	`100.00% <0.00%> (+12.50%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93c9cab...b15ee6f. Read the comment docs.

bogdandrutu · 2021-11-11T01:44:45Z

config/configmapprovider/composite.go

+
+// configComposeFunc combines a current config "orig" with another config "add".
+// The resulting config is stored in "orig".
+type configComposeFunc func(orig *config.Map, add *config.Map) error


Confusing "orig" and "add" names.

This type needs to be Exported.

bogdandrutu · 2021-11-11T02:01:23Z

config/configmapprovider/provider.go

+	//
+	// If the update is successful then the returned error will be nil.
+	// If Close() was called while WatchForUpdate() is in progress then the returned
+	// error will be configsource.ErrSessionClosed.


separate PR to move this in this package, maybe an issue to track it?

bogdandrutu · 2021-11-11T02:06:36Z

config/configmapprovider/composite.go

+		// At least one watcher fired. Close all sub-Retrieved. We don't need
+		// to close the one that fired the signal (readyWatchable.watchable),
+		// so provide it as an exception to closeAllExcept().
+		if err := cr.closeAllExcept(cr.watchContext, readyWatchable.watchable); err != nil {
+			return err
+		}


Not sure why you don't close that as well. I am not sure that we want the limitation to be when WatchForUpdate returns to also close, maybe we want to WatchForUpdate again? If this is the case you need to document that when returns Close is not needed to be called.

bogdandrutu · 2021-11-11T02:08:48Z

config/configmapprovider/composite.go

+		if err == nil {
+			// All Watchables returned without error, which means they are ready for
+			// another Get() call. Call Get() for all subproviders again and combine
+			// the results.
+			configMap, err2 := cr.subRetrieved.getAllAndCompose(cr.composeFunc)
+			if err2 != nil {
+				return err2
+			}
+
+			// Remember the result.
+			cr.composedMap = configMap
+		}


I think this is an overoptimization. You just signal the caller that you got an update and let the collector redo the map etc. This will simplify the requirements of concurrent calls, and also the logic here.

bogdandrutu · 2021-11-11T02:12:25Z

config/configmapprovider/provider.go

+//	r := mapProvider.Retrieve()                        |
+//	r.Get()                                            |
+//	r.WatchForUpdate() blocks until there are updates  |
+//	...                                                |
+//	r.Get()                                            |
+//	r.WatchForUpdate()                                 | Close() can be called anytime after Retrieve() returns


This is not what we have in the collector right now. We call r.Close after everyr.WatchForUpdate() then mapProvider.Retrieve() again.

tigrannajaryan · 2021-11-12T00:05:04Z

Let's not merge this. I think it is preferable to go this way #4403 and after that re-implement the composite provider if needed.

github-actions · 2021-11-19T03:15:53Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-12-01T03:15:54Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

bogdandrutu · 2021-12-08T18:19:15Z

@tigrannajaryan I think this PR reached his goal and we learned/implemented from it. Do we still need anything from here?

tigrannajaryan · 2021-12-08T19:03:41Z

No, it is not needed anymore.

tigrannajaryan requested a review from a team as a code owner November 10, 2021 15:52

tigrannajaryan requested a review from Aneurysm9 November 10, 2021 15:52

tigrannajaryan force-pushed the composite-configprovider branch from 6bd7ea3 to 834f18e Compare November 10, 2021 16:07

bogdandrutu reviewed Nov 11, 2021

View reviewed changes

tigrannajaryan force-pushed the composite-configprovider branch from 834f18e to b15ee6f Compare November 11, 2021 01:36

bogdandrutu reviewed Nov 11, 2021

View reviewed changes

tigrannajaryan marked this pull request as draft November 12, 2021 00:05

github-actions bot added the Stale label Nov 19, 2021

bogdandrutu removed the Stale label Nov 23, 2021

github-actions bot added the Stale label Dec 1, 2021

bogdandrutu removed the Stale label Dec 1, 2021

tigrannajaryan closed this Dec 8, 2021

tigrannajaryan deleted the composite-configprovider branch December 8, 2021 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement composite watchable config map provider #4391

Implement composite watchable config map provider #4391

tigrannajaryan commented Nov 10, 2021

tigrannajaryan commented Nov 10, 2021

tigrannajaryan commented Nov 11, 2021 •

edited

Loading

bogdandrutu Nov 11, 2021

tigrannajaryan Nov 11, 2021

codecov bot commented Nov 11, 2021

bogdandrutu Nov 11, 2021

bogdandrutu Nov 11, 2021

bogdandrutu Nov 11, 2021

bogdandrutu Nov 11, 2021

bogdandrutu Nov 11, 2021

bogdandrutu Nov 11, 2021

tigrannajaryan commented Nov 12, 2021

github-actions bot commented Nov 19, 2021

github-actions bot commented Dec 1, 2021

bogdandrutu commented Dec 8, 2021

tigrannajaryan commented Dec 8, 2021

Implement composite watchable config map provider #4391

Implement composite watchable config map provider #4391

Conversation

tigrannajaryan commented Nov 10, 2021

tigrannajaryan commented Nov 10, 2021

tigrannajaryan commented Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 11, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented Nov 12, 2021

github-actions bot commented Nov 19, 2021

github-actions bot commented Dec 1, 2021

bogdandrutu commented Dec 8, 2021

tigrannajaryan commented Dec 8, 2021

tigrannajaryan commented Nov 11, 2021 •

edited

Loading