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

pkg/endpoint: Simplify search for C header file during restore #11028

Merged

Conversation

pchaigno
Copy link
Member

Since we know the C header filename, we don't need to list the endpoint directory's content. We can simply check if the file exists.

Extracted from the host firewall branch.

@aanm Do you remember if there was a reason for listing the directory content instead of checking if the file exists? I feel like I'm missing something here. Commit 2365922 introduced this code.

@pchaigno pchaigno added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. labels Apr 17, 2020
@pchaigno pchaigno requested a review from aanm April 17, 2020 09:52
@aanm
Copy link
Member

aanm commented Apr 17, 2020

Since we know the C header filename, we don't need to list the endpoint directory's content. We can simply check if the file exists.

Extracted from the host firewall branch.

@aanm Do you remember if there was a reason for listing the directory content instead of checking if the file exists? I feel like I'm missing something here. Commit 2365922 introduced this code.

@pchaigno I could never get to the bottom of it but it is as it sounds in the comment. The first "read" would return os.FileNotFound, but not the second attempt. That's why I kept it just to be safe, it doesn't hurt try reading the same file twice if the file didn't exist in the first place.

@coveralls
Copy link

coveralls commented Apr 17, 2020

Coverage Status

Coverage increased (+0.005%) to 44.643% when pulling e58d8d8 on pr/pchaigno/simplify-header-search-during-restore into 531e61f on master.

@pchaigno pchaigno marked this pull request as ready for review April 17, 2020 12:10
@pchaigno pchaigno requested review from a team as code owners April 17, 2020 12:10
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

testing

@aanm aanm requested review from aanm and a team April 17, 2020 17:55
@pchaigno
Copy link
Member Author

@aanm Do you remember if there was a reason for listing the directory content instead of checking if the file exists? I feel like I'm missing something here. Commit 2365922 introduced this code.

@pchaigno I could never get to the bottom of it but it is as it sounds in the comment. The first "read" would return os.FileNotFound, but not the second attempt. That's why I kept it just to be safe, it doesn't hurt try reading the same file twice if the file didn't exist in the first place.

I think we're talking about two different things. I was wondering if there was a special reason for listing the content instead of checking if the file exists. The second listing of the directory was introduced later. Checking directly if the file exists should hopefully remove the need for that second listing.

pkg/endpoint/restore.go Outdated Show resolved Hide resolved
@pchaigno pchaigno force-pushed the pr/pchaigno/simplify-header-search-during-restore branch from 9dd06eb to 5760bb3 Compare April 21, 2020 14:29
@pchaigno pchaigno requested a review from a team as a code owner April 21, 2020 14:29
pkg/endpoint/restore.go Outdated Show resolved Hide resolved
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM, assuming @aanm's comment is addressed

Since we know the C header filename, we don't need to list the endpoint
directory's content. We can simply check if the file exists.

This commit retains the previous retry behavior: the endpoint header
file was sometimes not found before the second directory listing.
However, this commit introduces additional logging ('BUG' warning +
badLogMessages entry) to fail our E2E tests if the buggy behavior arises.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/simplify-header-search-during-restore branch from 5760bb3 to e58d8d8 Compare April 22, 2020 17:11
@pchaigno
Copy link
Member Author

test-me-please

@christarazi christarazi merged commit d81a5cd into master Apr 22, 2020
@christarazi christarazi deleted the pr/pchaigno/simplify-header-search-during-restore branch April 22, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants