Introducing defaulting logic #324

CharlyF · 2021-06-16T19:41:11Z

What does this PR do?

This PR introduces a new field in the status that surfaces the fields that have been defaulted as part of the defaulting logic of the DatadogAgent (DDA) object.
We do not update the spec of the DDA anymore, thus an update of the operator will directly benefit the deployment of the DDA.
Started using more pointers in order not to end up with empty structures in the object rendered.
Introduced the FeatureOverride method in order to take a second round of defaulting in case a certain feature is enabled (this is in order to accommodate our update work on simplifying Features)
Fixing the command used in the init-volume and the volumes mounted in case the Runtime security is enabled (or not).
Repackaged the defaults a bit.

Motivation

Get ready for GA.

Additional Notes

Defaulted objects are put in the defaultOverride structure, initialised ones are just left in the internal DDA (which represents the union of the defaultOverride and the spec).
One thing I did not fix is the new logic and structure of the Image needs to be updated in the patch section similarly to the Agent.Spec.Logs that moved to Features.Logs, because if the old image name is used we still default the tag value but with the current logic it will not be taken into account.
We use the method IsEqualStruct extensively, which I introduced in order to properly compare the large DDA object that contains nested json and pointers.
Main features and parts of the DDA have an Enable boolean because we want to support having a DDA without the DS for instance.
Added proper liveness/readiness probes. Fixed one for the DCA that was overridden if one used the External Metrics Server. We need to merge the liveness response upstream on one endpoint (in our backlog already)

Describe your test plan

Create DDAs with limited elements in the spec and verify that the DefaultOverride reflects the proper defaults.

github-actions

This pull request does not contain a valid label. Please add one of the following labels: bug, enhancement, documentation

codecov-commenter · 2021-06-16T19:48:06Z

Codecov Report

Merging #324 (f684135) into main (61f5b48) will increase coverage by 4.05%.
The diff coverage is 74.21%.

@@            Coverage Diff             @@
##             main     #324      +/-   ##
==========================================
+ Coverage   59.07%   63.12%   +4.05%     
==========================================
  Files          60       60              
  Lines        6370     6552     +182     
==========================================
+ Hits         3763     4136     +373     
+ Misses       2337     2119     -218     
- Partials      270      297      +27

Flag	Coverage Δ
unittests	`63.12% <74.21%> (+4.05%)`	⬆️

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

Impacted Files	Coverage Δ
api/v1alpha1/datadogagent_types.go	`100.00% <ø> (ø)`
api/v1alpha1/datadogagent_validation.go	`0.00% <0.00%> (ø)`
controllers/datadogagent/agent_rbac.go	`54.05% <0.00%> (ø)`
api/v1alpha1/utils.go	`36.84% <33.33%> (-3.16%)`	⬇️
controllers/datadogagent/secret_agent.go	`76.00% <33.33%> (-7.34%)`	⬇️
controllers/datadogagent/secret_clusteragent.go	`17.39% <40.00%> (-7.61%)`	⬇️
controllers/datadogagent/controller.go	`51.80% <41.17%> (+3.75%)`	⬆️
controllers/datadogagent/common_configmap.go	`60.71% <42.85%> (+0.97%)`	⬆️
cmd/kubectl-datadog/agent/upgrade/upgrade.go	`10.71% <50.00%> (ø)`
controllers/datadogagent/utils.go	`82.72% <59.34%> (-1.55%)`	⬇️
... and 14 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 61f5b48...f684135. Read the comment docs.

clamoriniere

Really like this implementation 👏
It is great improvement for the Operation UX!

controllers/datadogagent/controller.go

controllers/datadogagent/agent.go

controllers/datadogagent/clusteragent.go

cswatt

just small docs changes

docs/configuration.md

vboulineau

Great work, some comments. I think newer Operator with older CRDs will panic (the opposite is also probably true), so we should state that somewhere in the README.

api/v1alpha1/datadogagent_default.go

vboulineau · 2021-06-21T09:08:28Z

api/v1alpha1/utils.go

+
+// IsEqualStruct is a util fonction that returns whether 2 structures are the same
+// We compare the marchaled results to avoid traversing all fields and be agnostic of the struct.
+func IsEqualStruct(in interface{}, cmp interface{}) bool {


Looking at the PR, it looks like IsEqualStruct is only used to compare userInput and emptyStruct, like v1alpha1.IsEqualStruct(dd.Spec.Agent, v1alpha1.DatadogAgentSpecAgentSpec{}).
Doing JSON Marshalling is costly in terms of CPU and I wonder if we could do better here.

I agree, it's not ideal... Because of the intricacy of the whole structure (with pointers of other structures etc), I decided to go the marchalling route to make sure that I would compare apple to apple.
I did look into a few alternatives with build tags, interfaces etc but that was the best I could find.
I did isolate the function on purpose though, in case we figure out something better.

I would expect https://github.com/kubernetes/apimachinery/blob/master/pkg/api/equality/semantic.go to work in this scenario. We could extend the Semantic var with more types if not.

I looked into it a bit - It would probably work indeed. Though as it uses reflects under the hood I would be curious to benchmark the change. I will make a card so we can evaluate changing this bit of logic.

api/v1alpha1/datadogagent_types.go

vboulineau · 2021-06-21T09:40:40Z

controllers/datadogagent/agent_test.go

@@ -897,6 +893,26 @@ func defaultSystemProbeEnvVars() []corev1.EnvVar {
 			Name:  datadoghqv1alpha1.DDSystemProbeNPMEnabled,
 			Value: "false",
 		},
+		{
+			Name:  datadoghqv1alpha1.DDSystemProbeConntrackEnabled,


It means it's no more possible to configure Agent through datadog.yaml even if booleans are not set in DatadogAgent CRD.
It's not a problem specific to this field, it has been there for a while, but with this PR we should take the time to clarify what we want to support there.

Sorry, I don't get this one. Are you talking about the config file of the agent? Do you mean that we don't mirror values to this file?
Happy to chat about how you think we could approach this and use maybe this change to fix it.

Any ENV variable that is set by default means that having a configuration in datadog.yaml (and nothing set in DatadogAgent) is not going to work, the value from datadog.yaml being ignored.
As this PR handles defaulting, I think it's a good place to clarify what we want to do for these cases.

The goal of this change was to surface defaulted values in the status and stop mutating the spec of the DDA.
But I agree with you, this problem should be tackled as part of the boarder defaulting initiative.
For the sake of unblocking iterations towards the GA, I am creating a task in our backlog to look into that.
I still have to work on the Features, so all can be done in parallel once we merge this (which is a blocker for the rest).

controllers/datadogagent/clusteragent.go

controllers/datadogagent/common_configmap.go

CharlyF · 2021-06-24T04:52:38Z

@cswatt I actually tried to use your suggestions but it conflicts with the code gen tool that we use.
I am going to create a card so that we can see what we can do and I will share the patch there so your recs are not lost. Thanks again!

CharlyF · 2021-06-28T20:14:04Z

Tried to use the operator 0.6.0 with the new CRD and also used the new controller with the old CRD and it went well.
Are we good to go @vboulineau @clamoriniere ?

CharlyF requested review from a team as code owners June 16, 2021 19:41

github-actions bot requested changes Jun 16, 2021

View reviewed changes

CharlyF added the enhancement New feature or request label Jun 16, 2021

github-actions bot approved these changes Jun 16, 2021

View reviewed changes

ruthnaebeck added the editorial review Waiting on a more in-depth review label Jun 17, 2021

clamoriniere reviewed Jun 17, 2021

View reviewed changes

cswatt requested changes Jun 17, 2021

View reviewed changes

vboulineau reviewed Jun 21, 2021

View reviewed changes

CharlyF force-pushed the charlyf/defaulting_strategy branch from c087c69 to 62021cf Compare June 24, 2021 04:39

charlyF added 16 commits June 29, 2021 16:46

generating crds/apis

1161afc

updating plugins

45868a5

updating types

68a3f3e

updating default logic, repackage default vars

0788201

updating logic to validate DDA and patch features

9c5eb39

updating main controller loop

b6fcb99

updating logic to default the main agent

dc54e67

updating logic to default the cluster agent

ee862a5

updating logic to default the cluster check runner

699529f

updating logic to default features

586f946

adding the test suite

97a1698

updating licenses

62f532e

fixing hash update bug

8713394

updating PR per peer review

eda7230

adding generate for rebase

5654d77

Rebasing

f684135

CharlyF force-pushed the charlyf/defaulting_strategy branch from 7129693 to f684135 Compare June 29, 2021 21:07

vboulineau approved these changes Jun 30, 2021

View reviewed changes

CharlyF merged commit 2b31269 into main Jun 30, 2021

CharlyF deleted the charlyf/defaulting_strategy branch June 30, 2021 14:11

vboulineau mentioned this pull request Oct 14, 2021

Update some docstring fields in CRD #391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing defaulting logic #324

Introducing defaulting logic #324

CharlyF commented Jun 16, 2021

github-actions bot left a comment

codecov-commenter commented Jun 16, 2021 •

edited

clamoriniere left a comment

cswatt left a comment

vboulineau left a comment

vboulineau Jun 21, 2021

CharlyF Jun 24, 2021

vboulineau Jun 29, 2021

CharlyF Jun 29, 2021

vboulineau Jun 21, 2021

CharlyF Jun 24, 2021

vboulineau Jun 29, 2021

CharlyF Jun 29, 2021

CharlyF commented Jun 24, 2021

CharlyF commented Jun 28, 2021

Introducing defaulting logic #324

Introducing defaulting logic #324

Conversation

CharlyF commented Jun 16, 2021

What does this PR do?

Motivation

Additional Notes

Describe your test plan

github-actions bot left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 16, 2021 • edited

Codecov Report

clamoriniere left a comment

Choose a reason for hiding this comment

cswatt left a comment

Choose a reason for hiding this comment

vboulineau left a 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

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

CharlyF commented Jun 24, 2021

CharlyF commented Jun 28, 2021

codecov-commenter commented Jun 16, 2021 •

edited