Remove temporary cached files #2625

Fedosin · 2019-11-05T18:38:40Z

If the installer crashes (OOM, checksum validations, etc.) while downloading an OS image, temporary files remain in the system, clogging it.

This commit makes sure that temporary files are deleted before starting a new download.

Fixes: #1668

Fedosin · 2019-11-05T19:53:04Z

/test e2e-openstack

Fedosin · 2019-11-05T19:53:12Z

/test e2e-libvirt

abhinavdahiya · 2019-11-05T20:56:28Z

pkg/tfvars/internal/cache/cache.go

@@ -96,6 +96,21 @@ func cacheFile(reader io.Reader, filePath string, sha256Checksum string) (err er
 		}
 	}()

+	// Make sure that the temp file does not exist after completion of the function
+	defer func() {


so if we have the lock on the file above <file>.lock and we have a tmp for that, should we just overwrite it or block it away?

or we could maybe create a tmp file, not anchored on the filename at all.

cc @wking because he did the initial implementation.

I like the idea of giving temporary files random names!

...should we just overwrite it or block it away?

I'm having trouble parsing this. Can you elaborate?

or we could maybe create a tmp file, not anchored on the filename at all.

Anchored on the filename is because we want to be in the same directory to setup for an atomic rename.

...should we just overwrite it or block it away?

I'm having trouble parsing this. Can you elaborate?

since we know that we own the lock on the end file, we can either overwrite the file.tmp or remove the file.tmp if it exists and create a new one.

or we could maybe create a tmp file, not anchored on the filename at all.

Anchored on the filename is because we want to be in the same directory to setup for an atomic rename.

hhh, so rename is only atomic in same directory.. need to read a bit on it

Oh, I think I understand "should we just overwrite it", and I think "yes, if we have the flock, we should be clobbering any pre-existing .tmp file". That's somewhat orthogonal to this PR's current attempt to remove any dangling .tmp file on exit. We could do both, although clobbering pre-existing .tmp files is the only thing that will help in the "earlier installer was killed without having time to clean up" case.

technically the goal of the PR should be to fix #1668

and I think in that case, we should probably just go with, overwrite the tmp file instead of trying to clean it up..
@Fedosin wdyt?

@abhinavdahiya funny you said that, because one of the requirements for the checksum validation work is "Corrupted data should remain on disk for further investigation of the cause of failure", so I think you're right here.

wking · 2019-11-05T21:11:53Z

pkg/tfvars/internal/cache/cache.go

@@ -96,6 +96,21 @@ func cacheFile(reader io.Reader, filePath string, sha256Checksum string) (err er
 		}
 	}()

+	// Make sure that the temp file does not exist after completion of the function
+	defer func() {
+		_, err2 := os.Stat(tempPath)


Why bother with a racy Stat? Just try os.Remove(tempPath) and see if it works.

wking · 2019-11-05T21:13:47Z

pkg/tfvars/internal/cache/cache.go

+			// it manually.
+			err3 := os.Remove(tempPath)
+			if err3 != nil {
+				err = err3


I don't like clobbering err unless it's nil. I'm fine if you want to log an error you're about to discard though.

if err2 := os.Remove(tempPath); err2 != nil && !os.IsNotExist(err2) { if err == nil { err = err2 } else { logrus.Errorf("failed to clean up %s: %v", tempPath, err2) } }

or some such.

thanks! done as you said

Fedosin · 2019-11-05T23:45:09Z

Libvirt falls for a reason beyond the patch's control. Image download works well:

level=debug msg="Generating Terraform Variables..."
level=info msg="Fetching OS image: rhcos-43.81.201911011153.0-qemu.x86_64.qcow2"
level=debug msg="Unpacking OS image into "/home/packer/.cache/openshift-install/libvirt/image/0be301b959e6113d0dbd26a558a1c56d"..."
level=info msg="Consuming Master Machines from target directory"

Fedosin · 2019-11-06T00:11:33Z

/test e2e-libvirt

If the installer crashes (OOM, checksum validations, etc.) while downloading an OS image, temporary files remain in the system, clogging it. This commit makes sure that temporary files are deleted before starting a new download.

Fedosin · 2019-11-06T12:30:29Z

/close
Fixed by openshift/cluster-network-operator#365

openshift-ci-robot · 2019-11-06T12:30:31Z

@Fedosin: Closed this PR.

In response to this:

/close
Fixed by openshift/cluster-network-operator#365

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.

Fedosin · 2019-11-06T12:35:37Z

/reopen

openshift-ci-robot · 2019-11-06T12:35:38Z

@Fedosin: Reopened this PR.

In response to this:

/reopen

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.

openshift-ci-robot · 2019-11-06T14:18:37Z

@Fedosin: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`fff7484`	link	`/test e2e-aws-scaleup-rhel7`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

mandre · 2019-11-06T14:21:50Z

/lgtm

openshift-ci-robot · 2019-11-06T14:22:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fedosin, mandre, wking

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:

~~OWNERS~~ [wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 5, 2019

openshift-ci-robot requested review from jhixson74 and jstuever November 5, 2019 18:39

Fedosin force-pushed the delete_temp_files branch from c0e8e31 to 807b95c Compare November 5, 2019 20:19

abhinavdahiya reviewed Nov 5, 2019

View reviewed changes

wking reviewed Nov 5, 2019

View reviewed changes

Fedosin force-pushed the delete_temp_files branch from 807b95c to 69a7231 Compare November 5, 2019 21:46

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 5, 2019

Fedosin force-pushed the delete_temp_files branch from 69a7231 to 50832f6 Compare November 6, 2019 00:48

Remove temporary cached files

fff7484

If the installer crashes (OOM, checksum validations, etc.) while downloading an OS image, temporary files remain in the system, clogging it. This commit makes sure that temporary files are deleted before starting a new download.

Fedosin force-pushed the delete_temp_files branch from 50832f6 to fff7484 Compare November 6, 2019 00:52

wking approved these changes Nov 6, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2019

openshift-ci-robot closed this Nov 6, 2019

openshift-ci-robot reopened this Nov 6, 2019

openshift-ci-robot assigned mandre Nov 6, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2019

openshift-merge-robot merged commit 1f5526b into openshift:master Nov 6, 2019

Fedosin deleted the delete_temp_files branch March 16, 2020 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove temporary cached files #2625

Remove temporary cached files #2625

Fedosin commented Nov 5, 2019 •

edited

Loading

Fedosin commented Nov 5, 2019

Fedosin commented Nov 5, 2019

abhinavdahiya Nov 5, 2019

Fedosin Nov 5, 2019

wking Nov 5, 2019

abhinavdahiya Nov 5, 2019

wking Nov 5, 2019

abhinavdahiya Nov 5, 2019

Fedosin Nov 6, 2019

wking Nov 5, 2019

wking Nov 5, 2019

Fedosin Nov 5, 2019

Fedosin commented Nov 5, 2019

Fedosin commented Nov 6, 2019

Fedosin commented Nov 6, 2019

openshift-ci-robot commented Nov 6, 2019

Fedosin commented Nov 6, 2019

openshift-ci-robot commented Nov 6, 2019

openshift-ci-robot commented Nov 6, 2019

mandre commented Nov 6, 2019

openshift-ci-robot commented Nov 6, 2019

Remove temporary cached files #2625

Remove temporary cached files #2625

Conversation

Fedosin commented Nov 5, 2019 • edited Loading

Fedosin commented Nov 5, 2019

Fedosin commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fedosin commented Nov 5, 2019

Fedosin commented Nov 6, 2019

Fedosin commented Nov 6, 2019

openshift-ci-robot commented Nov 6, 2019

Fedosin commented Nov 6, 2019

openshift-ci-robot commented Nov 6, 2019

openshift-ci-robot commented Nov 6, 2019

mandre commented Nov 6, 2019

openshift-ci-robot commented Nov 6, 2019

Fedosin commented Nov 5, 2019 •

edited

Loading