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

[node-agent] Delete leftover *.tmp files from failed copy attempts #9630

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

oliver-goetz
Copy link
Member

How to categorize this PR?

/area robustness
/kind enhancement

What this PR does / why we need it:
This is a follow up to PR to #9609.
If a previous file copy attempt failed gardener-node-agent now deletes leftover *.tmp files instead of returning an error.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
/cc @rfranzke @MichaelEischer

I did not introduce a special handling for symlinks at the the destination.
copyAndSync copies into new files only.
In Copy and Move the final step is always fs.Rename(xyz, destination). If there is a symlink it will be overwritten with a file. GNA does not create symlinks. Thus, if there is one it was created by someone else. We don't check for files created by someone else either, so we can do the same for symlinks.

Release note:

If a previous file copy attempt failed `gardener-node-agent` now deletes leftover `*.tmp` files instead of returning an error. 

@gardener-prow gardener-prow bot requested a review from rfranzke April 19, 2024 16:44
Copy link
Contributor

gardener-prow bot commented Apr 19, 2024

@oliver-goetz: GitHub didn't allow me to request PR reviews from the following users: MichaelEischer.

Note that only gardener members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

How to categorize this PR?

/area robustness
/kind enhancement

What this PR does / why we need it:
This is a follow up to PR to #9609.
If a previous file copy attempt failed gardener-node-agent now deletes leftover *.tmp files instead of returning an error.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
/cc @rfranzke @MichaelEischer

I did not introduce a special handling for symlinks at the the destination.
copyAndSync copies into new files only.
In Copy and Move the final step is always fs.Rename(xyz, destination). If there is a symlink it will be overwritten with a file. GNA does not create symlinks. Thus, if there is one it was created by someone else. We don't check for files created by someone else either, so we can do the same for symlinks.

Release note:

If a previous file copy attempt failed `gardener-node-agent` now deletes leftover `*.tmp` files instead of returning an error. 

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.

@gardener-prow gardener-prow bot added area/robustness Robustness, reliability, resilience related kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 19, 2024
@@ -141,6 +141,14 @@ func Move(fs afero.Afero, source, destination string) error {
func moveCrossDevice(fs afero.Afero, source, destination string) error {
tmpFilePath := destination + ".tmp"

if tmpFile, err := fs.Stat(tmpFilePath); err == nil && tmpFile.Mode().IsRegular() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question - is it ok if we only do this removal in the moveCrossDevice function as copyAndSync is also called from Copy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy uses a temporary directory as destination (ref) which is deleted afterwards.
However, it is now removed with RemoveAll. Otherwise, it cannot be deleted if it is not empty e.g. when the file cannot be moved its the real destination.

@MichaelEischer
Copy link
Contributor

/lgtm

Thanks for the cleanup.

Copy link
Contributor

gardener-prow bot commented Apr 24, 2024

@MichaelEischer: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Thanks for the cleanup.

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.

Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2024
Copy link
Contributor

gardener-prow bot commented Apr 25, 2024

LGTM label has been added.

Git tree hash: a104eb4013391395c469aeb6cd8445a6c9bbdae2

@plkokanov
Copy link
Contributor

/approve

Copy link
Contributor

gardener-prow bot commented Apr 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: plkokanov

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2024
@gardener-prow gardener-prow bot merged commit f3e32fd into gardener:master Apr 25, 2024
17 checks passed
@oliver-goetz oliver-goetz deleted the enh/nodeagent-files branch May 21, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/robustness Robustness, reliability, resilience related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants