[features] Migrate LogCollection feature #484
Conversation
khewonc
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report
@@ Coverage Diff @@
## main #484 +/- ##
=======================================
Coverage 59.68% 59.68%
=======================================
Files 3 3
Lines 129 129
=======================================
Hits 77 77
Misses 40 40
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| if logCollection.ContainerCollectAll != nil && apiutils.BoolValue(logCollection.ContainerCollectAll) { | ||
| f.containerCollectAll = true | ||
| } | ||
| if apiutils.BoolValue(logCollection.ContainerCollectUsingFiles) { | ||
| f.containerCollectUsingFiles = true | ||
| } |
There was a problem hiding this comment.
| if logCollection.ContainerCollectAll != nil && apiutils.BoolValue(logCollection.ContainerCollectAll) { | |
| f.containerCollectAll = true | |
| } | |
| if apiutils.BoolValue(logCollection.ContainerCollectUsingFiles) { | |
| f.containerCollectUsingFiles = true | |
| } | |
| if logCollection.ContainerCollectAll != nil { | |
| // fallback to agent default if not set | |
| f.containerCollectAll = apiutils.BoolValue(logCollection.ContainerCollectAll) | |
| } | |
| f.containerCollectUsingFiles = apiutils.BoolValue(logCollection.ContainerCollectUsingFiles) |
does that work?
| func (f *logCollectionFeature) ConfigureV1(dda *v1alpha1.DatadogAgent) (reqComp feature.RequiredComponents) { | ||
| logCollection := dda.Spec.Features.LogCollection | ||
|
|
||
| if logCollection != nil && apiutils.BoolValue(logCollection.Enabled) { |
There was a problem hiding this comment.
| if logCollection != nil && apiutils.BoolValue(logCollection.Enabled) { | |
| if apiutils.BoolValue(logCollection.Enabled) { |
| // ManageNodeAgent allows a feature to configure the Node Agent's corev1.PodTemplateSpec | ||
| // It should do nothing if the feature doesn't need to configure it. | ||
| func (f *logCollectionFeature) ManageNodeAgent(managers feature.PodTemplateManagers) error { | ||
| if f.enable { |
There was a problem hiding this comment.
hmm good thought; i imagine we'll want to put this one level up, but it's not present yet (in the cluster agent reconciler)
There was a problem hiding this comment.
just to close the loop on this, we discussed separately but the features are filtered using the RequiredComponents struct a couple levels up. so this isn't needed and it may be possible to get rid of f.enable altogether
| func (f *logCollectionFeature) ManageNodeAgent(managers feature.PodTemplateManagers) error { | ||
| if f.enable { | ||
| // pointerdir volume mount | ||
| pointerVol, pointerVolMount := volume.GetVolumes(apicommon.PointerVolumeName, f.tempStoragePath, apicommon.PointerVolumePath, apicommon.PointerVolumeReadOnly) |
There was a problem hiding this comment.
i wonder whether it's worth having a readOnly const for each mount.. it may become cumbersome for something that isn't likely to change or particularly useful to track (open to debate). as an alternative, maybe we can use generic boolean constants such as
const trueValue = true
const falseValue = false
There was a problem hiding this comment.
Discussed separately. Decided on using true/false instead of adding more constants.
| // envvars | ||
| managers.EnvVar().AddEnvVarToContainer(apicommonv1.CoreAgentContainerName, &corev1.EnvVar{ | ||
| Name: apicommon.DDLogsEnabled, | ||
| Value: strconv.FormatBool(f.enable), |
There was a problem hiding this comment.
IMO since it's always true it's ok to be more explicit
* Migrate LogCollection feature
What does this PR do?
Migrate LogCollection feature
Add option to
GetVolumesto add non-readonly volumesMotivation
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.