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/daemon: write bytes, use buffered writer and compare bytes #411

Merged
merged 1 commit into from Feb 12, 2019

Conversation

Projects
None yet
6 participants
@runcom
Copy link
Member

runcom commented Feb 12, 2019

contents.Data is a byte slice and can be anything (i.e. a binary after
deconding). Use a bytes writer to prevent misbehaves in such scenarios.
Along with that, use a buffered writer around the file in case we're
dealing with big files. Also, compare byte slices as well instead of
using strings.

Signed-off-by: Antonio Murdaca <runcom@linux.com>
@cgwalters

This comment has been minimized.

Copy link
Contributor

cgwalters commented Feb 12, 2019

/lgtm
/approved

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Feb 12, 2019

Good catch! /lgtm

I think we need the same thing in checkFileContentsAndMode().

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 12, 2019

/hold

@runcom do you want to update ``checkFileContentsAndMode()` in this PR or keep them separate?

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 12, 2019

@runcom do you want to update ``checkFileContentsAndMode()` in this PR or keep them separate?

I'll push to this PR

@runcom runcom force-pushed the runcom:buffer-file-encoding branch from 3bd435e to b609251 Feb 12, 2019

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 12, 2019

pushed with changes to the other func, ptal

@openshift-ci-robot openshift-ci-robot added size/S and removed size/XS labels Feb 12, 2019

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 12, 2019

All unit tests passed, but reported failure 😕

/test unit

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 12, 2019

Ahh the failure was only noted in the full log:

ok  	github.com/openshift/machine-config-operator/lib/resourceread	0.047s
make: *** [test-unit] Error 1
2019/02/12 14:54:38 Container test in pod unit failed, exit code 2, reason Error
2019/02/12 14:54:39 Ran for 37s
error: could not run steps: test "unit" failed: the pod ci-op-iwn3fmvt/unit failed after 35s (failed containers: test): ContainerFailed one or more containers exited
Container test exited with code 2, reason Error
---
e
--- 
@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 12, 2019

yeah, there's an error I'm fixing

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Feb 12, 2019

Looks good to me. Though the commit message title could be tweaked now that it also fixes the reading side as well.

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 12, 2019

Looks good to me. Though the commit message title could be tweaked now that it also fixes the reading side as well.

yup, I'll change it once I fix the unit tests 👍

@runcom runcom force-pushed the runcom:buffer-file-encoding branch from b609251 to 5edb59c Feb 12, 2019

@runcom runcom changed the title pkg/daemon: write bytes and use buffered writer pkg/daemon: write bytes, use buffered writer and compare bytes Feb 12, 2019

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 12, 2019

updated and changed commit message/title as well

@@ -881,8 +882,8 @@ func checkFileContentsAndMode(filePath, expectedContent string, mode os.FileMode
glog.Errorf("could not read file: %q, error: %v", filePath, err)
return false
}
if strings.Compare(string(contents), expectedContent) != 0 {
glog.Errorf("content mismatch for file: %q; expected: %v; received: %v", filePath, expectedContent, string(contents))
if !bytes.Equal(contents, expectedContent) {

This comment has been minimized.

@runcom

runcom Feb 12, 2019

Author Member

this should now be much faster compared to string methods as it's directly implemented in assembly as well

if strings.Compare(string(contents), expectedContent) != 0 {
glog.Errorf("content mismatch for file: %q; expected: %v; received: %v", filePath, expectedContent, string(contents))
if !bytes.Equal(contents, expectedContent) {
glog.Errorf("content mismatch for file: %q; expected: %v; received: %v", filePath, expectedContent, contents)

This comment has been minimized.

@jlebon

jlebon Feb 12, 2019

Member

Hmm, this will print the contents as a byte array, right? We could not print the raw contents at all, though it's definitely helpful if it is indeed human-readable text (which it likely is in most cases). WDYT about casting to string here before printing and otherwise fallback on printing as a byte array?

I'm good with this as is too though and doing that as a follow up.

This comment has been minimized.

@runcom

runcom Feb 12, 2019

Author Member

the issue is casting to string a bytes slice always succeeds, not sure how to differentiate if we can't show something, I'm not really sure this info is helpful for big files as well 🤔

This comment has been minimized.

@jlebon

jlebon Feb 12, 2019

Member

How about we just tweak it to "content mismatch for file: %q" for now?

This comment has been minimized.

@runcom

runcom Feb 12, 2019

Author Member

sounds great 👍

This comment has been minimized.

@runcom

runcom Feb 12, 2019

Author Member

done

pkg/daemon: write bytes, use buffered writer and compare bytes
`contents.Data` is a byte slice and can be anything (i.e. a binary after
deconding). Use a bytes writer to prevent misbehaves in such scenarios.
Along with that, use a buffered writer around the file in case we're
dealing with big files. Also, compare byte slices as well instead of
using strings.

Signed-off-by: Antonio Murdaca <runcom@linux.com>

@runcom runcom force-pushed the runcom:buffer-file-encoding branch from 5edb59c to 610f216 Feb 12, 2019

@jlebon

This comment has been minimized.

Copy link
Member

jlebon commented Feb 12, 2019

/lgtm

@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Feb 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, cgwalters, jlebon, runcom

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 [ashcrow,cgwalters,jlebon,runcom]

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

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Feb 12, 2019

/hold cancel

@runcom

This comment has been minimized.

Copy link
Member Author

runcom commented Feb 12, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit f6d7208 into openshift:master Feb 12, 2019

6 checks passed

ci/prow/e2e-aws Job succeeded.
Details
ci/prow/e2e-aws-op Job succeeded.
Details
ci/prow/images Job succeeded.
Details
ci/prow/rhel-images Job succeeded.
Details
ci/prow/unit Job succeeded.
Details
tide In merge pool.
Details

@runcom runcom deleted the runcom:buffer-file-encoding branch Feb 12, 2019

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