OCPBUGS-16735: Truncate existing files when writing from inspect #1520
Conversation
oc adm inspect generated files sometime have the leading "---" and some time do not. This depends on the order of objects collected. This by itself is not an issue. However this becomes an issue when combined with multiple invocations of oc adm inspect and collecting data to the same directory like must-gather does. If an object is collected multiple times then the second time oc might overwrite the original file improperly and leave 4 bytes of the original content behind. This is happening when not writing the "---\n" in the second invocation as this makes the content 4B shorter and the original tailing 4B are left in the file intact. This garbage confuses YAML parsers. Signed-off-by: Martin Sivak <msivak@redhat.com>
openshift-ci-robot
added
jira/severity-important
|
@MarSik: This pull request references Jira Issue OCPBUGS-16735, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@MarSik: This pull request references Jira Issue OCPBUGS-16735, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
| @@ -51,7 +51,7 @@ func (r *resourceWriterReadCloser) Close() error { | |||
| type simpleFileWriter struct{} | |||
|
|
|||
| func (f *simpleFileWriter) Write(filepath string, src fileWriterSource) error { | |||
| dest, err := os.OpenFile(filepath, os.O_RDWR|os.O_CREATE, 0755) | |||
| dest, err := os.OpenFile(filepath, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0755) | |||
There was a problem hiding this comment.
In terms of yaml files generated by oc adm inspect command, this is indeed a problem and I'm supportive of this change.
On the other hand, this writer is also used for saving the collected logs of pods, operators, etc. and I think, we should ensure that this change will not cause truncating the logs.
There was a problem hiding this comment.
Well this always starts writing from the beginning, so it will either overwrite from the beginning (causing the same bug) or drop the "old" data. The only way to behave differently would be to pass O_APPEND.
Or is there something I am missing?
There was a problem hiding this comment.
I agree with you @MarSik. Since we are not passing O_APPEND, we are already overwriting on the old data and it would be good to truncate it.
Thanks for fixing this.
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, MarSik 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
bot
added
the
approved
|
e2e-aws-ovn looks good to me, the bug is not present in the generated must-gather and logs are there. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest-required |
|
@MarSik: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@MarSik: Jira Issue OCPBUGS-16735: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-16735 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. |
|
/cherry-pick release-4.13 |
|
@sferich888: new pull request created: #1553 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. |
oc adm inspect generated files sometime have the leading "---" and some time do not. This depends on the order of objects collected. This by itself is not an issue.
However this becomes an issue when combined with multiple invocations of oc adm inspect and collecting data to the same directory like must-gather does.
If an object is collected multiple times then the second time oc might overwrite the original file improperly and leave 4 bytes of the original content behind.
This is happening when not writing the "---\n" in the second invocation as this makes the content 4B shorter and the original tailing 4B are left in the file intact.
This garbage confuses YAML parsers.