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

[PROCS-3915] Add agent version check to Orchestrator Explorer feature to determine if process agent is required #1125

Merged
merged 7 commits into from
Mar 26, 2024
Merged

[PROCS-3915] Add agent version check to Orchestrator Explorer feature to determine if process agent is required #1125

merged 7 commits into from
Mar 26, 2024

Conversation

daniel-taf
Copy link
Contributor

@daniel-taf daniel-taf commented Mar 18, 2024 •
edited
Loading

What does this PR do?

This PR adds an agent version check to the Orchestrator Explorer feature to determine if the process agent container should be required. This container is no longer required for this feature as of 7.51.0.

Motivation

This behavior blocked our work to run in the process checks in the core agent. This was due to the orchestrator explorer feature always requiring the process agent container when now runs no checks. Thus, When the process checks run in the core agent, there are no checks running and the process agent exits resulting in a crash loop.

Additional Notes

Anything else we should know when reviewing?

Minimum Agent Versions

Are there minimum versions of the Datadog Agent and/or Cluster Agent required?

  • Agent: v7.51.0 (The minimum version where the Orchestrator explorer doesn't have checks in the process agent)
  • Cluster Agent: N/A

Describe your test plan

Agent Version < 7.51.0

spec:
  override:
    nodeAgent:
      image:
        name: agent
        tag: 7.50.0
  features:
    orchestratorExplorer:
      enabled: true
    # All processes features were disabled as they require the process agent container
    processDiscovery:
      enabled: false
    liveContainerCollection:
      enabled: false
    liveProcessCollection:
      enabled: false

Start the operator and configure the agent to: use a version less than 7.51.0, enable the orchestratorExplorer, and disable all Processes features. Verify the process agent container is included in the agent pod.

Agent Version >= 7.51.0

spec:
  override:
    nodeAgent:
      image:
        name: agent
        tag: 7.51.0
  features:
    orchestratorExplorer:
      enabled: true
    # All processes features were disabled as they require the process agent container
    processDiscovery:
      enabled: false
    liveContainerCollection:
      enabled: false
    liveProcessCollection:
      enabled: false

Start the operator and configure the agent to: use a version greather than or equal to 7.51.0, enable the orchestratorExplorer, and disable all Processes features. Verify the process agent container is no longer included in the agent pod.

Checklist

  • PR has at least one valid label: bug, enhancement, refactoring, documentation, tooling, and/or dependencies
  • PR has a milestone or the qa/skip-qa label

github-actions[bot]
github-actions bot requested changes Mar 18, 2024
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, dependencies

@daniel-taf daniel-taf added this to the v1.4.0 milestone Mar 18, 2024
@daniel-taf daniel-taf added the enhancement New feature or request label Mar 18, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.00%. Comparing base (98276c5) to head (3072e4e).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1125      +/-   ##
==========================================
+ Coverage   58.98%   59.00%   +0.01%     
==========================================
  Files         174      174              
  Lines       21386    21396      +10     
==========================================
+ Hits        12615    12625      +10     
  Misses       8004     8004              
  Partials      767      767
Flag Coverage Δ
unittests 59.00% <100.00%> (+0.01%) ⬆️

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

Files Coverage Δ
...tadogagent/feature/orchestratorexplorer/feature.go 83.53% <100.00%> (+1.06%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98276c5...3072e4e. Read the comment docs.

@daniel-taf daniel-taf changed the title Add agent version check to orchestrator explorer Add agent version check to orchestrator explorer to determine if process agent is required Mar 18, 2024
daniel-taf
daniel-taf commented Mar 20, 2024
View reviewed changes
controllers/datadogagent/feature/orchestratorexplorer/feature.go
@@ -116,6 +129,7 @@ func (f *orchestratorExplorerFeature) Configure(dda *v2alpha1.DatadogAgent) (req
func (f *orchestratorExplorerFeature) ConfigureV1(dda *v1alpha1.DatadogAgent) (reqComp feature.RequiredComponents) {
f.owner = dda
orchestratorExplorer := dda.Spec.Features.OrchestratorExplorer
f.processAgentRequired = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I understand V1 is being deprecated. however this was done to maintain current behavior.

@daniel-taf daniel-taf changed the title Add agent version check to orchestrator explorer to determine if process agent is required [PROCS-3710] Add agent version check to orchestrator explorer to determine if process agent is required Mar 20, 2024
@daniel-taf daniel-taf changed the title [PROCS-3710] Add agent version check to orchestrator explorer to determine if process agent is required [PROCS-3915] Add agent version check to orchestrator explorer to determine if process agent is required Mar 20, 2024
@daniel-taf daniel-taf marked this pull request as ready for review March 20, 2024 14:19
@daniel-taf daniel-taf requested review from a team as code owners March 20, 2024 14:19
@daniel-taf daniel-taf changed the title [PROCS-3915] Add agent version check to orchestrator explorer to determine if process agent is required [PROCS-3915] Add agent version check to Orchestrator Explorer feature to determine if process agent is required Mar 20, 2024
JLineaweaver
JLineaweaver approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@JLineaweaver JLineaweaver left a comment

Choose a reason for hiding this comment

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

LGTM

controllers/datadogagent/feature/orchestratorexplorer/feature.go Outdated
@@ -71,12 +75,21 @@ func (f *orchestratorExplorerFeature) ID() feature.IDType {
func (f *orchestratorExplorerFeature) Configure(dda *v2alpha1.DatadogAgent) (reqComp feature.RequiredComponents) {
f.owner = dda
orchestratorExplorer := dda.Spec.Features.OrchestratorExplorer
nodeAgent, ok := dda.Spec.Override[v2alpha1.NodeAgentComponentName]
// Process Agent is not required on versions as of 7.51.0
f.processAgentRequired = ok && nodeAgent.Image != nil && !utils.IsAboveMinVersion(component.GetAgentVersionFromImage(*nodeAgent.Image), "7.51.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any risk of *nodeAgent.Image being nil?

Copy link
Contributor Author

@daniel-taf daniel-taf Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hopefully someone from container-ecosystems can confirm, but if nodeAgent.Image is not set then gcr.io/datadoghq/agent:latest is being used for the image by default. Thus, I have that if *nodeAgent.Image is nil, processAgentRequired evaluates to false (latest is 7.51.0 as of now).

Copy link
Contributor

Choose a reason for hiding this comment

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

for readability, do you mind structuring the conditional similar to how it's done here?

@celenechang celenechang modified the milestones: v1.4.0, v1.6.0 Mar 21, 2024
@daniel-taf daniel-taf mentioned this pull request Mar 21, 2024
Closed
2 tasks
celenechang
celenechang reviewed Mar 25, 2024
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.

just a couple small suggestions, thanks!

celenechang
celenechang reviewed Mar 26, 2024
View reviewed changes
controllers/datadogagent/feature/orchestratorexplorer/feature.go Outdated
Comment on lines 84 to 86
if nodeAgent, ok := dda.Spec.Override[v2alpha1.NodeAgentComponentName]; ok {
if f.processAgentRequired = nodeAgent.Image != nil &&
!utils.IsAboveMinVersion(component.GetAgentVersionFromImage(*nodeAgent.Image), "7.51.0"); f.processAgentRequired {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry i wasn't clear in my comment earlier - here's my suggested edit:

Suggested change
if nodeAgent, ok := dda.Spec.Override[v2alpha1.NodeAgentComponentName]; ok {
if f.processAgentRequired = nodeAgent.Image != nil &&
!utils.IsAboveMinVersion(component.GetAgentVersionFromImage(*nodeAgent.Image), "7.51.0"); f.processAgentRequired {
if nodeAgent, ok := dda.Spec.Override[v2alpha1.NodeAgentComponentName]; ok {
if nodeAgent.Image != nil && !utils.IsAboveMinVersion(component.GetAgentVersionFromImage(*nodeAgent.Image), "7.51.0") {
f.processAgentRequired = true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

celenechang
celenechang reviewed Mar 26, 2024
View reviewed changes
controllers/datadogagent/feature/orchestratorexplorer/feature.go Outdated
Comment on lines 84 to 86
if nodeAgent, ok := dda.Spec.Override[v2alpha1.NodeAgentComponentName]; ok {
if f.processAgentRequired = nodeAgent.Image != nil &&
!utils.IsAboveMinVersion(component.GetAgentVersionFromImage(*nodeAgent.Image), "7.51.0"); f.processAgentRequired {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we put the minimum agent version in a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@daniel-taf daniel-taf force-pushed the daniel.tafoya/orchestrator-process-agent-container-fix branch from 9cf434c to 3072e4e Compare March 26, 2024 14:51
@daniel-taf
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented Mar 26, 2024

🚂 MergeQueue

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

Use /merge -c to cancel this operation!

@daniel-taf
Copy link
Contributor Author

/remove

@dd-devflow
Copy link

dd-devflow bot commented Mar 26, 2024

🚂 Devflow: /remove

@dd-devflow
Copy link

dd-devflow bot commented Mar 26, 2024

⚠️ MergeQueue

This merge request was unqueued

If you need support, contact us on Slack #ci-interfaces!

@daniel-taf
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented Mar 26, 2024

🚂 MergeQueue

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

Use /merge -c to cancel this operation!

@daniel-taf
Copy link
Contributor Author

/remove

@dd-devflow
Copy link

dd-devflow bot commented Mar 26, 2024

🚂 Devflow: /remove

@dd-devflow
Copy link

dd-devflow bot commented Mar 26, 2024

⚠️ MergeQueue

This merge request was unqueued

If you need support, contact us on Slack #ci-interfaces!

@celenechang celenechang merged commit 7db14b9 into main Mar 26, 2024
33 of 34 checks passed
@celenechang celenechang deleted the daniel.tafoya/orchestrator-process-agent-container-fix branch March 26, 2024 18:22
mftoure pushed a commit that referenced this pull request Oct 3, 2024
@daniel-taf
[PROCS-3915] Add agent version check to Orchestrator Explorer feature…
6ff96ff 
… to determine if process agent is required (#1125)

* Add agent version check to orchestrator explorer

* Add unit tests for agent version checks

* Add unit tests for agent version checks

* fix v1 tests

* Fix generated files

* Update agent check logic

* min version var
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] approved these changes

@celenechang celenechang celenechang approved these changes

@JLineaweaver JLineaweaver JLineaweaver approved these changes

@robertjli robertjli robertjli approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request mergequeue-status: removed
Projects
None yet
Milestone
v1.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@daniel-taf @codecov-commenter @robertjli @JLineaweaver @celenechang