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

Canonical Config #131

Merged
merged 2 commits into from
Aug 31, 2021
Merged

Canonical Config #131

merged 2 commits into from
Aug 31, 2021

Conversation

bojand
Copy link
Member

@bojand bojand commented Aug 31, 2021

The current FluentBitConfig controller in the operator owns Kubernetes Secret objects, and watches for the custom types, Input, Output, Parser, and Filter. Within the Reconcile we gather existing objects, and then performs a merge and re-render all the configuration options. However several types (especially in filter plugins) have map[string]string as parameters. Additionally when we load the objects, I think the order is not guaranteed to be the same every time. Due to these conditions, as we increase the number of Input, Filter, and Output objects created that may use map parameters, the config rendering becomes increasingly likely to be non-deterministic. This results in essentially an infinite loop in the reconciler...

  1. An event causes Reconcile(), for example a new Output.
  2. We gather all the objects and render the config.
  3. Configuration Secret object is updated, which causes the Update Event to fire and notify our operator.
  4. Reconcile() is called again.
  5. We gather all the objects again and render the config.
    • This time around we should be generating the same config since nothing has truly changed, and effectively ending the reconcile loop.
    • However, since the configuration rendering is non-deterministic, the rendered configuration file is likely to be different than the original. Order of Add or Set params in Modify filter may be different for example. Or order of plugins may be different. When we save the configuration in the Secret object, the Secret is actually updated, with a new resource version, and the Update Event is fired, and we get notification to reconcile again...

The issue worsens with the more fluentbit config objects are present. The fewer objects, the likelihood is greater we end up with the same permutation and stop the loop. More objects, and likelihood decreases and we are constantly reconciling.

This results in two main problems:

  • Operator starts using a lot of CPU constantly
  • We're constantly updating the config file, which is causing fluent bit watcher to constantly restart fluent bit process.

The problem is easily reproducible by creating a lot of objects, especially Modify filter with Set and/or Add, and Output objects with Headers property.

This PR attempts to fix this issue and improve the operator by the following changes:

  • Move the KVs type into its own new params package within plugins package so that it can be used within plugins
  • Add InsertStringMap function to params package to be used to canonically insert a map
  • Sort items in all the plugin Load() functions
  • Update dependencies. This was mainly done so we can use more optimized controllerutil.CreateOrPatch() function. However it seems updating sigs.k8s.io/controller-runtime package caused update of everything.
  • Update the build to Go 1.16. It would not compile with the new updated dependency. I get the following error:
 > [builder 10/10] RUN CGO_ENABLED=0 GO111MODULE=on go build -a -o manager main.go:
#17 60.75 # sigs.k8s.io/controller-runtime/pkg/manager
#17 60.75 /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/manager/manager.go:53:2: duplicate method Start
#17 60.75 note: module requires Go 1.16
------
error: failed to solve: executor failed running [/bin/sh -c CGO_ENABLED=0 GO111MODULE=on go build -a -o manager main.go]: exit code: 2

An alternative, perhaps somewhat hacky, solution is to use an event filter in the controller, and in Update predicate catch that we are doing an update to the secret and return false indicating that this Update event should not be processed. This does not seem like a correct approach to me.

* place kvs into own package.
* add sorted insert.
* update deps

Signed-off-by: Bojan <[email protected]>
@bojand bojand changed the title Canonical config (#3) Canonical Config Aug 31, 2021
@bojand
Copy link
Member Author

bojand commented Aug 31, 2021

Redo of PR #130 to fix the signoff breakage.

go.mod Outdated
@@ -3,19 +3,17 @@ module kubesphere.io/fluentbit-operator
go 1.13
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also needs to be upgraded to 1.16.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update Go version in README.md and go.mod files

@benjaminhuo
Copy link
Member

@bojand Have to say this is a great enhancement to existing logic!

You mentioned you got compile errors after upgrading to go 1.16, is it solved now?

You also mentioned An alternative, perhaps somewhat hacky, solution is to use an event filter in the controller, and in Update predicate catch that we are doing an update to the secret and return false indicating that this Update event should not be processed. This does not seem like a correct approach to me., it seems that your PR doesn't use this alternative, right?

@bojand
Copy link
Member Author

bojand commented Aug 31, 2021

@benjaminhuo That's correct. This solution only performs deterministic consistent rendering.

The alternative would look something like this:

return ctrl.NewControllerManagedBy(mgr).
	For(&logging.FluentBitConfig{}).
	Owns(&corev1.Secret{}).
	Watches(&source.Kind{Type: &logging.Input{}}, &handler.EnqueueRequestForObject{}).
	Watches(&source.Kind{Type: &logging.Filter{}}, &handler.EnqueueRequestForObject{}).
	Watches(&source.Kind{Type: &logging.Output{}}, &handler.EnqueueRequestForObject{}).
	Watches(&source.Kind{Type: &logging.Parser{}}, &handler.EnqueueRequestForObject{}).
	WithEventFilter(predicate.Funcs{
		UpdateFunc: func(e event.UpdateEvent) bool {
			sOld := e.ObjectOld.(*corev1.Secret)
			sNew := e.ObjectNew.(*corev1.Secret)

			if sOld != nil && sNew != nil {
				return false
			}

			return true
		},
	}).
	Complete(r)

But I don't think this is what we want really...

@benjaminhuo
Copy link
Member

@benjaminhuo That's correct. This solution only performs deterministic consistent rendering.

The alternative would look something like this:

return ctrl.NewControllerManagedBy(mgr).
	For(&logging.FluentBitConfig{}).
	Owns(&corev1.Secret{}).
	Watches(&source.Kind{Type: &logging.Input{}}, &handler.EnqueueRequestForObject{}).
	Watches(&source.Kind{Type: &logging.Filter{}}, &handler.EnqueueRequestForObject{}).
	Watches(&source.Kind{Type: &logging.Output{}}, &handler.EnqueueRequestForObject{}).
	Watches(&source.Kind{Type: &logging.Parser{}}, &handler.EnqueueRequestForObject{}).
	WithEventFilter(predicate.Funcs{
		UpdateFunc: func(e event.UpdateEvent) bool {
			sOld := e.ObjectOld.(*corev1.Secret)
			sNew := e.ObjectNew.(*corev1.Secret)

			if sOld != nil && sNew != nil {
				return false
			}

			return true
		},
	}).
	Complete(r)

But I don't think this is what we want really...

Agree, Thank You!

@benjaminhuo benjaminhuo merged commit 9ba046a into fluent:master Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants