OCPBUGS-48163: Atomic nodename file #4504
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pmtk: This pull request references Jira Issue OCPBUGS-48163, 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
openshift-ci
bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@pmtk: This pull request references Jira Issue OCPBUGS-48163, which is valid. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
Satisfy linter (complexity)
|
|
||
| // GenerateUniqueTempPath returns a filepath from given path with extra suffix | ||
| // which doesn't exist. | ||
| func GenerateUniqueTempPath(path string) (string, error) { |
There was a problem hiding this comment.
Can't we use https://pkg.go.dev/os#CreateTemp instead of this code?
There was a problem hiding this comment.
We could in .nodename creation, but not in the atomic_dir_copy.go. This function gives us a path that we can use either as a file or a dir. CreateTemp will return a File, which can only be a file.
There was a problem hiding this comment.
The current function seems to be flawed because it only checks for a file existence, but does not create any. So, if the code is running in parallel, there is a chance that we'll be getting the same path back.
There was a problem hiding this comment.
We don't execute it in parallel (and we don't call it with the same path - it's always different). Also, if we use this function to get a temporary unique path for a directory, how does creating file helps us?
BTW. This isn't new function - it was moved from state.go and it's proven to be working for some time already
There was a problem hiding this comment.
I think we can leave this code as-is if it's called under certain conditions, but as a generic implementation, this function is flawed.
It does not create objects on the file system, but uses object existence as a test for a validity of the generated name.
This is a race condition when the function is called from different threads for the same path - random number generation might return the same output and the function would succeed because file object is not on the file system yet.
There was a problem hiding this comment.
I created https://issues.redhat.com/browse/USHIFT-5373 to follow-up on this separately.
Let's not hold this review on this topic.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ggiguash, pmtk 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 |
|
@pmtk: 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-sigs/prow repository. I understand the commands that are listed here. |
|
@pmtk: Jira Issue OCPBUGS-48163: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-48163 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 openshift-eng/jira-lifecycle-plugin repository. |
No description provided.