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.

Sign up for GitHub

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

Jump to bottom

feat: DatadogAgent reconcileV2 skeleton #491

Merged
merged 5 commits into from
May 18, 2022
Merged

feat: DatadogAgent reconcileV2 skeleton #491

merged 5 commits into from
May 18, 2022

Conversation

clamoriniere
Copy link
Collaborator

What does this PR do?

  • Define new reconcileV2() function that will be use with v2alpha1.DatadogAgent
  • Add a new flag to the operator to switch to reconcileV2
  • Create mostly the workflow for the Cluster-Agent component in reconcileV2.
  • Create the skeleton of several function. they can be implemented in other PR.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Describe your test plan

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

github-actions[bot]
github-actions bot requested changes May 12, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

@clamoriniere clamoriniere added this to the v1.0.0 milestone May 12, 2022
@clamoriniere clamoriniere marked this pull request as ready for review May 12, 2022 13:06
@clamoriniere clamoriniere requested review from a team as code owners May 12, 2022 13:06
@clamoriniere clamoriniere added enhancement New feature or request refactoring labels May 12, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 12, 2022 •
edited
Loading

Codecov Report

Merging #491 (f1b216a) into main (c32af59) will decrease coverage by 1.17%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
- Coverage   59.68%   58.51%   -1.18%     
==========================================
  Files           3        3              
  Lines         129      135       +6     
==========================================
+ Hits           77       79       +2     
- Misses         40       43       +3     
- Partials       12       13       +1
Flag Coverage Δ
unittests 58.51% <22.22%> (-1.18%) ⬇️

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

Impacted Files Coverage Δ
controllers/datadogagent_controller.go 55.07% <12.50%> (-2.75%) ⬇️
controllers/setup.go 65.30% <100.00%> (+0.72%) ⬆️

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 c32af59...f1b216a. Read the comment docs.

vboulineau
vboulineau reviewed May 12, 2022
View reviewed changes
controllers/datadogagent/controller.go
var resp reconcile.Result
var err error

if r.options.V2Enabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we plan to keep it, this could be automated by checking the storageVersion of the CRD

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this hack, to allow me to link the new code pass, but keep the current reconcile function active to have all tests pass.

controllers/datadogagent/controller_reconcile_dca.go Outdated Show resolved Hide resolved
controllers/datadogagent/controller_reconcile_v2.go Outdated Show resolved Hide resolved
controllers/datadogagent/controller_reconcile_v2.go Show resolved Hide resolved
@clamoriniere clamoriniere force-pushed the clamoriniere/reconcilerv2 branch 2 times, most recently from eaa7abd to 7e5e5b5 Compare May 16, 2022 15:43
celenechang
celenechang reviewed May 17, 2022
View reviewed changes
controllers/datadogagent/feature/enabledefault/feature.go
Agent: feature.RequiredComponent{
IsRequired: &trueValue,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

so this "feature" ensures that the agent and cluster agent are enabled if nothing else is set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes,
I will add in another PR the creation of the default associate resources: serviceAccount, condigmap, RBAC...

celenechang
celenechang reviewed May 17, 2022
View reviewed changes
Copy link
Contributor

@celenechang celenechang left a comment

Choose a reason for hiding this comment

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

overall looks great, thanks. do you mind adding the copyright to new files? (ok with me if it's easier in a separate PR)

celenechang
celenechang reviewed May 17, 2022
View reviewed changes
controllers/datadogagent/feature/kubernetesstatecore/feature.go
f.serviceAccountName = v2alpha1.GetClusterAgentServiceAccount(dda)
if dda.Spec.Features.ClusterChecks != nil && apiutils.BoolValue(dda.Spec.Features.ClusterChecks.Enabled) {
f.clusterChecksEnabled = true
output.ClusterCheckRunner.IsRequired = &f.clusterChecksEnabled
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be set to true under if apiutils.BoolValue(dda.Spec.Features.ClusterChecks.UseClusterChecksRunners) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right fixing now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@celenechang
I made the fix in #496

celenechang
celenechang approved these changes May 17, 2022
View reviewed changes
Copy link
Contributor

@celenechang celenechang left a comment

Choose a reason for hiding this comment

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

added a question, but it can be addressed in a subsequent PR

@celenechang celenechang merged commit cda69ab into main May 18, 2022
@celenechang celenechang deleted the clamoriniere/reconcilerv2 branch May 18, 2022 19:21
@davidor davidor mentioned this pull request May 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vboulineau vboulineau vboulineau left review comments

@celenechang celenechang celenechang approved these changes

@github-actions GitHub Actions github-actions approved these changes

Assignees
No one assigned
Labels
component/controller enhancement New feature or request refactoring
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@clamoriniere @codecov-commenter @celenechang @vboulineau