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

GCP Server AutoDiscover: only log when the script fails #51117

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

marcoandredinis
Copy link
Contributor

When running Auto Discover for GCP VMs, teleport always logs the output in debug level.

Now, it only logs the stdout/stderr when the installation fails. It also adds some context on which VMs it failed and the log level is now Error.

Related: #45172

@marcoandredinis marcoandredinis added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Jan 16, 2025
@github-actions github-actions bot requested review from nklaassen and Tener January 16, 2025 15:53
@marcoandredinis marcoandredinis added no-changelog Indicates that a PR does not require a changelog entry and removed no-changelog Indicates that a PR does not require a changelog entry labels Jan 20, 2025
@marcoandredinis marcoandredinis force-pushed the marco/improve_discover_gcpvm_install_logs branch from a060d61 to decc9ee Compare January 23, 2025 17:45
lib/cloud/gcp/vm.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Tener Tener left a comment

Choose a reason for hiding this comment

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

This is logged in the loop over different IP addresses, so we may end up in a situation where subsequent attempt to a different IP address succeeds, but we logged an error-level information anyway. I think we should only log an error once we are sure we have ran out of IP addresses to try. So perhaps some kind of aggregate error?

When running Auto Discover for GCP VMs, teleport always logs the output
in debug level.

Now, it only logs the stdout/stderr when the installation fails.
It also adds some context on which VMs it failed and the log level is
now Error.
@marcoandredinis marcoandredinis force-pushed the marco/improve_discover_gcpvm_install_logs branch from decc9ee to 01fc2f4 Compare January 24, 2025 10:06
@marcoandredinis
Copy link
Contributor Author

This is logged in the loop over different IP addresses, so we may end up in a situation where subsequent attempt to a different IP address succeeds, but we logged an error-level information anyway. I think we should only log an error once we are sure we have ran out of IP addresses to try. So perhaps some kind of aggregate error?

I've change things slightly.
Now, we'll log only once in case of an error.
When the connection is successful with one of the IPs but the script fails, it will log the script stdout/stderr
When the connection is not successful, it will log all the errors returned (just like it was before, but now it uses Error instead of Debug).

@marcoandredinis marcoandredinis requested a review from Tener January 24, 2025 10:12
@marcoandredinis marcoandredinis added backport/branch/v15 backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry and removed no-changelog Indicates that a PR does not require a changelog entry labels Jan 24, 2025
@marcoandredinis marcoandredinis added this pull request to the merge queue Jan 24, 2025
Merged via the queue into master with commit d3042d5 Jan 24, 2025
43 of 44 checks passed
@marcoandredinis marcoandredinis deleted the marco/improve_discover_gcpvm_install_logs branch January 24, 2025 17:17
@public-teleport-github-review-bot

@marcoandredinis See the table below for backport results.

Branch Result
branch/v15 Failed
branch/v16 Failed
branch/v17 Failed

marcoandredinis added a commit that referenced this pull request Jan 24, 2025
* GCP Server AutoDiscover: only log when the script fails

When running Auto Discover for GCP VMs, teleport always logs the output
in debug level.

Now, it only logs the stdout/stderr when the installation fails.
It also adds some context on which VMs it failed and the log level is
now Error.

* Log once per RunCommand invocation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants