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

Bugs fixes #18086

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Bugs fixes #18086

wants to merge 2 commits into from

Conversation

TheRealMal
Copy link

This PR fixes 2 bugs:

Fixes #18085 Memory leak
Fixes #18084 Dereference of nil

@k8s-ci-robot
Copy link

Hi @TheRealMal. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@henrybear327
Copy link
Contributor

/ok-to-test

@@ -64,6 +64,7 @@ func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, d
if err != nil {
return "", fmt.Errorf("could not open %s (%v)", partpath, err)
}
defer f.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

it sounds like an easy fix, but how would the rename operation below work on an open file?

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL this actually works with the rename syscall on linux. It will fail on Windows with the classic "the process cannot access the file because it is being used by another process". Which is Support Tier 3, but the fix is simple: just wrap the file operations before the rename in a function and the defer within.

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by "rename operation"? I just moved f.Close() to defer, because file should be closed even if any error occured after it being opened. I can't see any logic changes.

Copy link
Contributor

@tjungblu tjungblu May 28, 2024

Choose a reason for hiding this comment

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

This operation:

if err = os.Rename(partpath, dbPath); err != nil {
return resp.Version, fmt.Errorf("could not rename %s to %s (%v)", partpath, dbPath, err)
}

will fail on Windows, because defer closes the file after the function exits and you can't rename a still opened file.

MRE if you want to try this out: https://gist.github.com/tjungblu/7d4ff9bc88afe86004bf292fd9032966

Copy link
Author

Choose a reason for hiding this comment

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

Well, you are right

Copy link
Contributor

Choose a reason for hiding this comment

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

turned out to be slightly more verbose than I thought, but that could work:

	err = func() (err error) {
		var f *os.File
		f, err = os.OpenFile(partpath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileutil.PrivateFileMode)
		if err != nil {
			return fmt.Errorf("could not open %s (%v)", partpath, err)
		}
		defer func() {
			err = f.Close()
		}()
		
		lg.Info("created temporary db file", zap.String("path", partpath))

		var rd io.ReadCloser
		rd, err = cli.Snapshot(ctx)
		if err != nil {
			return
		}
		lg.Info("fetching snapshot", zap.String("endpoint", cfg.Endpoints[0]))
		var size int64
		size, err = io.Copy(f, rd)
		if err != nil {
			return
		}
		if !hasChecksum(size) {
			return fmt.Errorf("sha256 checksum not found [bytes: %d]", size)
		}
		if err = fileutil.Fsync(f); err != nil {
			return
		}

		lg.Info("fetched snapshot",
			zap.String("endpoint", cfg.Endpoints[0]),
			zap.String("size", humanize.Bytes(uint64(size))),
			zap.String("took", humanize.Time(now)),
		)

		return
	}()

	if err != nil {
		return err
	}

Copy link
Author

@TheRealMal TheRealMal May 28, 2024

Choose a reason for hiding this comment

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

But err = f.Close() will rewrite any error which comes after defer func() {err = f.Close()}() inside that block, won't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Joining the errors should work :)

Ref: https://pkg.go.dev/errors#Join

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Hi @TheRealMal - Thanks for raising these fixes. It looks like your two commits are not signed. Can you please take a look and ensure all commits are signed so the developer certificate of origin check passes? i.e.

git rebase --signoff HEAD~2

@jmhbnz
Copy link
Member

jmhbnz commented Jul 4, 2024

Discussed during sig-etcd triage meeting - hey @TheRealMal do you have capacity to continue this pr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Memory leak Missing nil check
5 participants