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

Pass context with timeout to FQDN lookup #37756

Merged
merged 7 commits into from
Feb 2, 2024

Conversation

ycombinator
Copy link
Contributor

Proposed commit message

This PR improves the FQDN lookup function calls by passing a context with a 1 minute timeout, thus preventing the calls from blocking indefinitely due to DNS issues and such.

Depends on: elastic/go-sysinfo#199 and a new release of that library.

@ycombinator ycombinator requested a review from a team as a code owner January 25, 2024 19:52
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 25, 2024
Copy link
Contributor

mergify bot commented Jan 25, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @ycombinator? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@ycombinator ycombinator marked this pull request as draft January 25, 2024 19:56
@ycombinator ycombinator added bug Team:Elastic-Agent Label for the Agent team backport-v8.12.0 Automated backport with mergify labels Jan 25, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 25, 2024
@elasticmachine
Copy link
Collaborator

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Duration: 45 min 16 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

mergify bot commented Jan 25, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fqdn-ctx upstream/fqdn-ctx
git merge upstream/main
git push upstream fqdn-ctx

@elasticmachine
Copy link
Collaborator

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Duration: 132 min 33 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 133 min 38 sec

Pipeline error 1

This error is likely related to the pipeline itself. Click here
and then you will see the error (either incorrect syntax or an invalid configuration).

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@ycombinator ycombinator removed the Team:Elastic-Agent Label for the Agent team label Feb 1, 2024
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Feb 1, 2024
@ycombinator ycombinator marked this pull request as ready for review February 1, 2024 02:12
@ycombinator ycombinator requested a review from a team as a code owner February 1, 2024 02:12
@ycombinator ycombinator added the Team:Elastic-Agent Label for the Agent team label Feb 1, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Feb 1, 2024
@elasticmachine
Copy link
Collaborator

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Duration: 12 min 33 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Duration: 32 min 26 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

fqdnLookupCtx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
defer cancel()

fqdn, err := h.FQDNWithContext(fqdnLookupCtx)
Copy link
Member

Choose a reason for hiding this comment

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

If this times out it falls back to the non-FQDN, does that ever get updated? Or is the Beat stuck with the non-FQDN.

If the first rule of network communication is "never make a request without a timeout", the second one is "never rely on a single network request to succeed".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't get updated. I agree that it would be nice to keep retrying but I also think that's out of scope for this PR here; this one is just about implementing the first rule. I'll file a separate issue to implement the second one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 163 min 26 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@@ -96,7 +97,7 @@ func New(cfg *config.C) (beat.Processor, error) {
}

// create a unique ID for this instance of the processor
cbIDStr := ""
var cbIDStr string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is unrelated to this PR; it is included in this PR to make the linter happy. Without it, the linter was failing with this error:

assigned to cbIDStr, but reassigned without using the value (wastedassign)

@elasticmachine
Copy link
Collaborator

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2024-02-02T13:06:35.282+0000

  • Duration: 127 min 59 sec

Test stats 🧪

Test Results
Failed 0
Passed 23348
Skipped 1554
Total 24902

Steps errors 3

Expand to view the steps failures

Google Storage Download
  • Took 0 min 5 sec . View more details here
  • Description: [2024-02-02T13:27:29.142Z] [Google Cloud Storage Plugin] Found 1 files to download from pattern: gs:
Checks if running on a Unix-like node
  • Took 0 min 0 sec . View more details here
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: untar: step failed with error null

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 166 min 48 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2024-02-02T16:53:46.619+0000

  • Duration: 92 min 6 sec

Test stats 🧪

Test Results
Failed 0
Passed 28346
Skipped 2014
Total 30360

Steps errors 2

Expand to view the steps failures

metricbeat-goIntegTest - mage goIntegTest
  • Took 34 min 2 sec . View more details here
  • Description: mage goIntegTest
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: Error 'org.jenkinsci.plugins.workflow.steps.FlowInterruptedException'

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

💔 Build Failed

Failed CI Steps

History

cc @ycombinator

@ycombinator ycombinator enabled auto-merge (squash) February 2, 2024 19:19
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 165 min 45 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@ycombinator ycombinator merged commit e4b448f into elastic:main Feb 2, 2024
127 of 128 checks passed
mergify bot pushed a commit that referenced this pull request Feb 2, 2024
* Pass context with timeout to FQDN lookup

* Add temporary gomod replace

* Add CHANGELOG entry

* Update dependency

* Update method calls

* Add nolint for rand deprecation warning

* Make linter happy

(cherry picked from commit e4b448f)

# Conflicts:
#	NOTICE.txt
#	go.mod
#	go.sum
@ycombinator ycombinator deleted the fqdn-ctx branch February 2, 2024 21:06
ycombinator added a commit that referenced this pull request Feb 3, 2024
)

* Pass context with timeout to FQDN lookup (#37756)

* Pass context with timeout to FQDN lookup

* Add temporary gomod replace

* Add CHANGELOG entry

* Update dependency

* Update method calls

* Add nolint for rand deprecation warning

* Make linter happy

(cherry picked from commit e4b448f)

# Conflicts:
#	NOTICE.txt
#	go.mod
#	go.sum

* Fix conflicts

---------

Co-authored-by: Shaunak Kashyap <[email protected]>
Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024
* Pass context with timeout to FQDN lookup

* Add temporary gomod replace

* Add CHANGELOG entry

* Update dependency

* Update method calls

* Add nolint for rand deprecation warning

* Make linter happy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.12.0 Automated backport with mergify bug Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants