Bug 1837564: pkg/terraform: add diagnostics errors for terraform apply operations #3535
Conversation
|
/test e2e-gcp |
|
/test all |
abhinavdahiya
force-pushed
the
tf_diagnose
branch
2 times, most recently
from
a27c07b
to
4043e43
Compare
|
/approve |
|
/test e2e-azure |
|
/test e2e-gcp |
|
/lgtm |
There was a problem hiding this comment.
Looks good to me too. I would be interested to see some actual log errors.
@rna-afk PTAL re metric relevance
|
/test e2e-azure |
|
@patrickdillon here is an example of the error logged by the installer https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_installer/3535/pull-ci-openshift-installer-master-e2e-azure/541#1:build-log.txt%3A108 |
|
/retest |
| }, { | ||
| match: regexp.MustCompile(`Error: Error Creating/Updating Subnet .*: network.SubnetsClient#CreateOrUpdate: .* Code="AnotherOperationInProgress" Message="Another operation on this or dependent resource is in progress`), | ||
|
|
||
| reason: "AzureMultiOperationFailure", |
There was a problem hiding this comment.
Can we structure this so Azure conditions can be defined in a pkg/terraform/azure subpackage, so per-platform sub-teams can be assigned as owners for their platform's conditions? There might be some generic conditions that apply to all platforms, but I'd expect most of the time, you'd want a whole bunch of platform-specific conditions from the platform the installer is using, plus maybe a handful of generic conditions.
There was a problem hiding this comment.
for i'm keeping this list in same file and we can split in into platforms in future work.
pkg/diagnostics/error.go
Outdated
| func (e *Err) Error() string { | ||
| buf := &bytes.Buffer{} | ||
| if len(e.Source) > 0 { | ||
| fmt.Fprintf(buf, "error(%s) from %q", e.Reason, e.Source) |
There was a problem hiding this comment.
Do we need %q for Source, or can we use %s (here and in your later Error from %q)? Both of those strings are compiled into the installer right? I'd expect to use %q only if we were passing along some information slurped from the provider, where we weren't sure if there was going to be whitespace or other potentially confusing characters.
abhinavdahiya
changed the title
openshift-ci-robot
added
bugzilla/severity-medium
|
@abhinavdahiya: This pull request references Bugzilla bug 1837564, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The diagnostics error allows the providing important context to provide better error reporting for the the users. The error allows the installer assets etc. to provide structural information, - Source: the source of the error, the installer assets have errors from cloud providers or internal errors, the source allows providing hat context to better categorize these errors. - Reason: is a single word reason that corrrectly summarizes the type of error, allows the users to quickly understand the type of error. also should allow internal metrics tracking to tracks these error types.
The terraform errors are tracked in a buffer. This buffers is then used to match against various known conditions to understand the reasons for the errors. This now allows the terraform apply to return specific errors in these cases instead of previous `failed to apply Terraform` constant string message.
abhinavdahiya
force-pushed
the
tf_diagnose
branch
from
dabcc4d
to
699beca
Compare
|
/retest /test e2e-azure |
|
/retest |
|
/lgtm |
|
/approve |
1 similar comment
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, jhixson74 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@abhinavdahiya: All pull requests linked via external trackers have merged: openshift/installer#3535. Bugzilla bug 1837564 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The terraform errors are tracked in a buffer. This buffers is then used to match against various
known conditions to understand the reasons for the errors.
This now allows the terraform apply to return specific errors in these cases instead of previous
failed to apply Terraformconstantstring message.
/cc @openshift/openshift-team-installer