feat: Introduce the Features Factory #412

clamoriniere · 2021-11-30T14:47:20Z

What does this PR do?

This draft PR is here to illustrate how the Feature Factory can improve the generically of the DatadogAgent controller logic.

Motivation

Ease code evolution
Better separation of concern: each feature logic are implemented in a separated package that allows better code ownership detection
Ease unit-testing: since each feature should implement the same interface. it will ease the creation of a generic unit-test framework and allow to test each feature separately.

Additional Notes

Some code entry point:

the factory implementation: instantiate and configure Feature when needed.
the dependencies Store: use to keep track of every resource dependencies.

Then the new Reconcile function is define here: https://github.com/DataDog/datadog-operator/blob/clamoriniere/generic-reconciler/controllers/datadogagent/reconcilerv2.go

Describe your test plan

Write there any instructions and details you may have to test your PR.

codecov-commenter · 2022-01-04T20:16:46Z

Codecov Report

Merging #412 (4751c1b) into main (c5aa4c7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #412   +/-   ##
=======================================
  Coverage   60.60%   60.60%           
=======================================
  Files           3        3           
  Lines         132      132           
=======================================
  Hits           80       80           
  Misses         40       40           
  Partials       12       12

Flag	Coverage Δ
unittests	`60.60% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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 c5aa4c7...4751c1b. Read the comment docs.

celenechang · 2022-01-18T19:36:47Z

controllers/datadogagent/feature/types.go

+type Feature interface {
+	// Configure use to configure the internal of a Feature
+	// It should return `true` if the feature is enabled, else `false`.
+	Configure(dda *v2alpha1.DatadogAgent) bool


Is this function both altering the DatadogAgent spec and checking if the feature is enabled?

From the ksmcore example it seems only the latter, in which case maybe we can call it IsConfigured or IsEnabled

yes the idea it to extra the information needed from the DatadogAgent. like this we don't need to access again the resource after when processing the feature

Makefile

vboulineau · 2022-01-24T14:02:22Z

controllers/datadogagent/dependencies/store.go

+	// OperatorStoreLabelKey used to identified which resource is managed by the store.
+	OperatorStoreLabelKey = "operator.datadoghq.com/managed-by-store"
+	// OperatorGenHashAnnotationKey annotation key used on a Resource in order to compare 2 resources without know the spec struct.
+	OperatorGenHashAnnotationKey = "operator.datadoghq.com/gen-hash"


Not used, could be easily implemented in the store, no?

it already the case and used here:

https://github.com/DataDog/datadog-operator/pull/412/files#diff-48d116016241a7d62a8614e21fc9b23f37962ca9d1bb54fa2ee5abd447709ed3R74

https://github.com/DataDog/datadog-operator/pull/412/files#diff-48d116016241a7d62a8614e21fc9b23f37962ca9d1bb54fa2ee5abd447709ed3R146

I can move it to private

vboulineau · 2022-01-24T14:08:23Z

controllers/datadogagent/feature/dummy/feature.go

+	return false
+}
+
+func (f *dummyFeature) ConfigureV1(dda *v1alpha1.DatadogAgent) bool {


Should not be necessary with conversion in place

Agree, But for now. I use it to validate the PR in #430

we can easily remove it later

vboulineau · 2022-01-24T14:15:43Z

controllers/datadogagent/feature/dummy/feature.go

+// ManageDependencies allows a feature to manage its dependencies.
+// Feature's dependencies should be added in the store.
+func (f *dummyFeature) ManageDependencies(store feature.DependenciesStoreClient) error {
+	return nil


With the store API, it means each deps need to do:

Get -> Check if nil -> Not nil: Update -> Nil: Create

I think we should think about how to improve this experience before implementing features?

IMO dependencies are not so often shared between features.

I was thinking to create a function GetOrCreate that return the instance or create a new instance automatically.
then the Store have a fuction AddOrUpdate so it doesn't need to know if the resource already exist in the API server.

I'm open to other idea

I was thinking about Roles for instance (or ConfigMap if we decide to move all configs to datadog.yaml). Having a GetOrCreate may not be useful as you will still need to check if the object is newly created or not to fill some fieds.

I think we way want to have Managers on top of the store. Like the RBACManager, offering a high level API like AddPermissions(serviceAccount, roleName, permissions) and when Apply is called on the store, it lazily generates the necessary objects.

About the comment below, your intention looks like a PodTemplateManager to me, with its helper functions to manage each piece.

@vboulineau
I have added a new commit with a RBACManager b0722e2

vboulineau · 2022-01-24T14:47:58Z

controllers/datadogagent/feature/types.go

+}
+
+// PodFeatureConfiguration use to store a Pod Feature Configuration.
+type PodFeatureConfiguration struct {


I'm not sure to understand how this will be used, wasn't able to find any reference?

I was a tentative to implement your proposition about not exposing the PodTemplate to feature function but only a subset of what can be configured.

But I haven't found a good implementation. It made me think that it was more clear to share the PodTemplate. and provide helper fonction to add|update|remove envvar, volume, volumemount.

celenechang · 2022-04-29T02:11:26Z

controllers/datadogagent/controller.go

+	// Manage dependencies
+	// -----------------------
+	depsStore := dependencies.NewStore(&dependencies.StoreOptions{SupportCilium: r.options.SupportCilium})
+	rbacManager := feature.NewResourcesManagers(depsStore)


should this be generalized to resourceManager instead of rbacManager ?

clamoriniere added this to the v1.0.0 milestone Nov 30, 2021

clamoriniere added the enhancement New feature or request label Nov 30, 2021

clamoriniere requested a review from a team November 30, 2021 14:47

github-actions bot approved these changes Nov 30, 2021

View reviewed changes

clamoriniere force-pushed the clamoriniere/generic-reconciler branch from b1344e5 to da281a5 Compare November 30, 2021 14:49

clamoriniere force-pushed the clamoriniere/generic-reconciler branch from 524c60f to d2579c9 Compare January 4, 2022 21:45

clamoriniere mentioned this pull request Jan 4, 2022

move apis/v1alpha1 utils to apis/utils #424

Merged

clamoriniere force-pushed the clamoriniere/generic-reconciler branch 5 times, most recently from bb39d31 to fbc5410 Compare January 17, 2022 17:07

celenechang reviewed Jan 18, 2022

View reviewed changes

clamoriniere force-pushed the clamoriniere/generic-reconciler branch from e68a192 to bb4ee42 Compare January 22, 2022 21:07

clamoriniere marked this pull request as ready for review January 23, 2022 23:42

vboulineau reviewed Jan 24, 2022

View reviewed changes

clamoriniere force-pushed the clamoriniere/generic-reconciler branch from 3e6a95e to 42c1ff4 Compare January 25, 2022 21:17

clamoriniere mentioned this pull request Jan 25, 2022

migration KSMCore to feature pattern #430

Merged

clamoriniere force-pushed the clamoriniere/generic-reconciler branch from 42c1ff4 to 06ed743 Compare January 25, 2022 21:39

clamoriniere force-pushed the clamoriniere/generic-reconciler branch 3 times, most recently from 39f3a0e to 523c55f Compare February 21, 2022 10:49

vboulineau approved these changes Feb 21, 2022

View reviewed changes

clamoriniere force-pushed the clamoriniere/generic-reconciler branch 2 times, most recently from 89b4697 to 67e92b7 Compare February 21, 2022 12:59

clamoriniere force-pushed the clamoriniere/generic-reconciler branch 4 times, most recently from 9814ca2 to 4751c1b Compare April 5, 2022 20:35

clamoriniere force-pushed the clamoriniere/generic-reconciler branch 5 times, most recently from c4db9a3 to f0e8997 Compare April 6, 2022 10:53

celenechang reviewed Apr 29, 2022

View reviewed changes

clamoriniere added 2 commits April 29, 2022 11:20

revert change to have unit-test working

d5ca5e6

rebase apis common after rebase

c6adf6e

clamoriniere force-pushed the clamoriniere/generic-reconciler branch from f0e8997 to c6adf6e Compare April 29, 2022 09:30

clamoriniere requested a review from a team as a code owner April 29, 2022 09:30

clamoriniere merged commit 88fe849 into main Apr 29, 2022

clamoriniere deleted the clamoriniere/generic-reconciler branch April 29, 2022 09:54

celenechang pushed a commit that referenced this pull request May 18, 2022

feat: Introduce the Features Factory (#412)

b706662

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce the Features Factory #412

feat: Introduce the Features Factory #412

clamoriniere commented Nov 30, 2021

codecov-commenter commented Jan 4, 2022 •

edited

celenechang Jan 18, 2022

celenechang Jan 18, 2022

clamoriniere Jan 20, 2022

vboulineau Jan 24, 2022

clamoriniere Jan 25, 2022 •

edited

vboulineau Jan 24, 2022

clamoriniere Jan 25, 2022

vboulineau Jan 24, 2022

clamoriniere Jan 25, 2022 •

edited

vboulineau Jan 26, 2022

clamoriniere Jan 28, 2022

vboulineau Jan 24, 2022

clamoriniere Jan 25, 2022 •

edited

celenechang Apr 29, 2022

feat: Introduce the Features Factory #412

feat: Introduce the Features Factory #412

Conversation

clamoriniere commented Nov 30, 2021

What does this PR do?

Motivation

Additional Notes

Describe your test plan

codecov-commenter commented Jan 4, 2022 • edited

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

clamoriniere Jan 25, 2022 • edited

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

clamoriniere Jan 25, 2022 • edited

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

clamoriniere Jan 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 4, 2022 •

edited

clamoriniere Jan 25, 2022 •

edited

clamoriniere Jan 25, 2022 •

edited

clamoriniere Jan 25, 2022 •

edited