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

test in_tail: use safer method to cleanup #3283

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

kenhys
Copy link
Contributor

@kenhys kenhys commented Mar 5, 2021

Which issue(s) this PR fixes:

N/A

What this PR does / why we need it:

This PR aims to fix in_tail test cases on Windows.

On Windows, when the file or directory is removed and created
frequently, there is a case that creating file or directory will fail.
This situation is caused by pending file or directory deletion which
is mentioned on win32 API document.

ref. https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#files

As a workaround, execute rename and remove method.

Docs Changes:

N/A

Release Note:

N/A

@kenhys kenhys force-pushed the fix-in-tail-windows branch 2 times, most recently from b55a1c4 to d893c33 Compare March 8, 2021 02:38
@cosmo0920
Copy link
Contributor

@kenhys How long does it take to be ready to review?

test/plugin/test_in_tail.rb Outdated Show resolved Hide resolved
@ashie
Copy link
Member

ashie commented Mar 8, 2021

Finally we got success status on Windows! I think it's worth to merge soon.
If you are considering add more fixes, you can open new pull request.

@kenhys
Copy link
Contributor Author

kenhys commented Mar 8, 2021

Finally we got success status on Windows! I think it's worth to merge soon.

No. The root of failure is not resolved...

@ashie
Copy link
Member

ashie commented Mar 8, 2021

No. The root of failure is not resolved...

Hmm, I see.
We shouldn't merge it to make it clear.
Is the root cause that file deletion is pending?
We should resolve the root cause of such pending, shouldn't we?
(Someone still holds the file handles?)

@kenhys
Copy link
Contributor Author

kenhys commented Mar 8, 2021

  1. TMP_DIR is empty, safer cleanup works as expected,
  2. TMP_DIR is not empty, safer cleanup logic does not work as expected.

1 is ok, as GitHub CI indicates as well
2 is not OK. this is the blocker.

It may be TMP_DIR is still owned by the driver, but it is a bit strange behavior

@kenhys kenhys force-pushed the fix-in-tail-windows branch 2 times, most recently from aca881f to 113c8e6 Compare March 8, 2021 08:21
@kenhys
Copy link
Contributor Author

kenhys commented Mar 8, 2021

I've changed to use if Fluent.windows? block to reduce in_tail testing time overhead.

@kenhys
Copy link
Contributor Author

kenhys commented Mar 9, 2021

I've changed to use if Fluent.windows? block to reduce in_tail testing time overhead.

Oops, It introduces unexpected CI degration, I'll fix it.

@kenhys kenhys force-pushed the fix-in-tail-windows branch 4 times, most recently from b8aac2c to 6d4d1f0 Compare March 9, 2021 03:06
@kenhys
Copy link
Contributor Author

kenhys commented Mar 9, 2021

waiting CI result.

@kenhys kenhys marked this pull request as ready for review March 9, 2021 04:30
@kenhys
Copy link
Contributor Author

kenhys commented Mar 9, 2021

Ruby 3.0(windows)

4 failure is related to test_server.rb

Ruby 2.5 (ubuntu-latest)

2021-03-09T03:24:32.4985691Z Error: test: a node supporting responses after stop(ForwardOutputTest): Fluent::Test::Driver::TestTimedOut: Test case timed out with hard limit.
2021-03-09T03:24:32.4989241Z /home/runner/work/fluentd/fluentd/lib/fluent/test/driver/base.rb:201:in `rescue in run_actual'
2021-03-09T03:24:32.4991540Z /home/runner/work/fluentd/fluentd/lib/fluent/test/driver/base.rb:196:in `run_actual'
2021-03-09T03:24:32.4993666Z /home/runner/work/fluentd/fluentd/lib/fluent/test/driver/base.rb:95:in `run'
2021-03-09T03:24:32.4995739Z /home/runner/work/fluentd/fluentd/lib/fluent/test/driver/base_owner.rb:130:in `run'
2021-03-09T03:24:32.5000793Z /home/runner/work/fluentd/fluentd/test/plugin/test_out_forward.rb:651:in `block in <class:ForwardOutputTest>'

This error should be fixed in another PR.

@kenhys kenhys requested review from cosmo0920 and ashie March 9, 2021 04:52
test/plugin/test_in_tail.rb Outdated Show resolved Hide resolved
@kenhys kenhys force-pushed the fix-in-tail-windows branch from 6d4d1f0 to 6606a33 Compare March 9, 2021 05:32
@kenhys
Copy link
Contributor Author

kenhys commented Mar 9, 2021

I've fixed and rebased against master.

Copy link
Member

@ashie ashie left a comment

Choose a reason for hiding this comment

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

@kenhys kenhys force-pushed the fix-in-tail-windows branch 2 times, most recently from 895d365 to 9daac78 Compare March 9, 2021 06:40
Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

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

LGTM.
I'd confirmed that this PR makes to be more stable CI on Windows.

This PR aims to fix in_tail test cases on Windows.

On Windows, when the file or directory is removed and created
frequently, there is a case that creating file or directory will fail.
This situation is caused by pending file or directory deletion which
is mentioned on win32 API document.

ref. https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#files

As a workaround, execute rename and remove method.

Signed-off-by: Kentaro Hayashi <[email protected]>
Signed-off-by: Kentaro Hayashi <[email protected]>
@kenhys kenhys force-pushed the fix-in-tail-windows branch from 9daac78 to c78301a Compare March 9, 2021 07:15
@kenhys
Copy link
Contributor Author

kenhys commented Mar 9, 2021

As #3289 was merged, then rebased again.

@cosmo0920
Copy link
Contributor

Ruby 2.5, 2.6, and 2.7 jobs on Windows get green! 💪

@ashie ashie merged commit fa6614a into fluent:master Mar 9, 2021
@kenhys kenhys deleted the fix-in-tail-windows branch March 9, 2021 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants