Skip to content

Comments

Fixed file hashing for LCOW.#37316

Closed
notanaverageman wants to merge 1 commit intomoby:masterfrom
notanaverageman:lcow_wrong_cache
Closed

Fixed file hashing for LCOW.#37316
notanaverageman wants to merge 1 commit intomoby:masterfrom
notanaverageman:lcow_wrong_cache

Conversation

@notanaverageman
Copy link
Contributor

@notanaverageman notanaverageman commented Jun 20, 2018

- What I did
While file hashes are being generated, they are searched on the host filesystem. For LCOW this always returns a file not found error and the file name is used instead of the SHA hash. This was causing wrong cache uses like #36793. I have fixed the hashing method for LCOW.

- How I did it
For LCOW, instead of trying to read the file from the host filesystem, use the container filesystem.

- How to verify it
#36793 has the necessary steps.

- Description for the changelog

Fixed file hash values for LCOW that caused wrong usage of layer caches.

- A picture of a cute animal (not mandatory but encouraged)

Fixed file hashing for LCOW.
ebe4ee7 
Signed-off-by: Yusuf Tarık Günaydın <[email protected]>
@thaJeztah
Copy link
Member

thaJeztah commented Jun 20, 2018

ping @jhowardmsft @johnstep PTAL

@cpuguy83
Copy link
Member

cpuguy83 commented Jun 20, 2018

Seems like this should be handled lower down in the abstraction, as is this is pretty sloppy.

@notanaverageman
Copy link
Contributor Author

notanaverageman commented Jun 27, 2018

I am not familiar with this repository to make the abstraction and also do not have time for it. I have written a comment on #36793. If someone tries to fix this, he can use the information there. So, closing this.

@lowenna
Copy link
Member

lowenna commented Jun 27, 2018

I think the right/cleaner fix is:

	fi, err := c.root.Lstat(fullPath)
	if err != nil {
		// Backwards compatibility: a missing file returns a path as hash.
		// This is reached in the case of a broken symlink.
		return relPath, nil
	}

which takes advantage of containerd\continuity\driver abstraction.

I'll test this and put a replacement PR in if so.

This was referenced Jun 27, 2018
Closed
Merged
@cpuguy83
Copy link
Member

cpuguy83 commented Jun 27, 2018

@yusuf-gunaydin understood, thanks for the contribution!

@thaJeztah thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

Assignees

No one assigned

Labels

area/builder Build area/lcow Issues and PR's related to the experimental LCOW feature kind/experimental platform/windows status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@notanaverageman @thaJeztah @cpuguy83 @lowenna @GordonTheTurtle