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.

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

Do not memoize system-probe sock path error #32728

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

vitkyrka
Copy link
Contributor

@vitkyrka vitkyrka commented Jan 7, 2025

What does this PR do?

Do not memoize system-probe sock path error

The system-probe socket may not exist temporarily due to expected races
during startup. This error should not be memoized.

This restores the behaviour to how it was before #31150.

Motivation

Incident

Describe how you validated your changes

Tested manually by starting the agent first and then system-probe later. The service discovery check initially
fails but then succeeds on later runs after system-probe has been started. Without this PR it always fails even after system-probe starts.

Possible Drawbacks / Trade-offs

Additional Notes

The system-probe socket may not exist temporarily due to expected races
during startup.  This error should not be memoized.

This restores the behaviour to how it was before #31150.
@vitkyrka vitkyrka added changelog/no-changelog team/usm The USM team qa/done QA done before merge and regressions are covered by tests labels Jan 7, 2025
@github-actions github-actions bot added the short review PR is simple enough to be reviewed quickly label Jan 7, 2025
@vitkyrka vitkyrka marked this pull request as ready for review January 7, 2025 14:12
@vitkyrka vitkyrka requested review from a team as code owners January 7, 2025 14:12
@vitkyrka vitkyrka requested review from mbakht and removed request for a team January 7, 2025 14:12
sysProbeUtil := newSystemProbe(path)
err = sysProbeUtil.initRetry.SetupRetrier(&retry.Config{ //nolint:errcheck
err := sysProbeUtil.initRetry.SetupRetrier(&retry.Config{ //nolint:errcheck
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 get an error (a temporary error) here, we will still memoize it
I don't think the memoize is a good idea here

Copy link
Contributor

Choose a reason for hiding this comment

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

The error returning from this function is just for configuration issues of the retrier https://github.com/DataDog/datadog-agent/blob/7.61.x/pkg/util/retry/retrier.go#L30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And before the PR this was done in a sync.Once and the error was ignored: https://github.com/DataDog/datadog-agent/blob/7.60.1/pkg/process/net/common.go#L78-L88

@github-actions github-actions bot added medium review PR review might take time and removed short review PR is simple enough to be reviewed quickly labels Jan 7, 2025
@agent-platform-auto-pr
Copy link
Contributor

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=52329758 --os-family=ubuntu

Note: This applies to commit 77f1d59

@vitkyrka
Copy link
Contributor Author

vitkyrka commented Jan 7, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Jan 7, 2025

Devflow running: /merge

View all feedbacks in Devflow UI.


2025-01-07 17:07:04 UTC ℹ️ MergeQueue: waiting for PR to be ready

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.


2025-01-07 17:24:59 UTC ℹ️ MergeQueue: This merge request was already merged

This pull request was merged directly.

@FlorentClarret
Copy link
Member

/trigger-ci --variable RUN_ALL_BUILDS=true --variable RUN_KITCHEN_TESTS=true --variable RUN_E2E_TESTS=on --variable RUN_UNIT_TESTS=on --variable RUN_KMT_TESTS=on

@FlorentClarret FlorentClarret merged commit d9bd73e into 7.61.x Jan 7, 2025
246 of 248 checks passed
@FlorentClarret FlorentClarret deleted the vincent.whitchurch/no-memo-sock branch January 7, 2025 17:24
@github-actions github-actions bot added this to the 7.61.0 milestone Jan 7, 2025
@dd-devflow
Copy link

dd-devflow bot commented Jan 7, 2025

Devflow running: /trigger-ci --variable RUN_ALL_BUILDS=true --varia...

View all feedbacks in Devflow UI.


2025-01-07 17:25:38 UTC ℹ️ Gitlab pipeline started

Started pipeline #52357681

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog medium review PR review might take time qa/done QA done before merge and regressions are covered by tests team/usm The USM team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants