Skip to content

test in_tail: use safer method to cleanup#3283

Merged
ashie merged 2 commits into
fluent:masterfrom
kenhys:fix-in-tail-windows
Mar 9, 2021
Merged

test in_tail: use safer method to cleanup#3283
ashie merged 2 commits into
fluent:masterfrom
kenhys:fix-in-tail-windows

Conversation

@kenhys

@kenhys kenhys commented Mar 5, 2021

Copy link
Copy Markdown
Contributor

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

cosmo0920 commented Mar 8, 2021

Copy link
Copy Markdown
Contributor

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

ashie
ashie requested changes Mar 8, 2021
View reviewed changes
Comment thread test/plugin/test_in_tail.rb Outdated
@ashie

ashie commented Mar 8, 2021

Copy link
Copy Markdown
Member

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

kenhys commented Mar 8, 2021

Copy link
Copy Markdown
Contributor Author

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

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

@ashie

ashie commented Mar 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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
kenhys commented Mar 8, 2021
View reviewed changes
Comment thread test/plugin/test_in_tail.rb
@kenhys

kenhys commented Mar 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author
  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

kenhys commented Mar 8, 2021

Copy link
Copy Markdown
Contributor Author

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

@kenhys

kenhys commented Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

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

kenhys commented Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

waiting CI result.

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

kenhys commented Mar 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 ashie and cosmo0920 March 9, 2021 04:52
ashie
ashie requested changes Mar 9, 2021
View reviewed changes
Comment thread test/plugin/test_in_tail.rb Outdated
@kenhys kenhys force-pushed the fix-in-tail-windows branch from 6d4d1f0 to 6606a33 Compare March 9, 2021 05:32
@kenhys

kenhys commented Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

I've fixed and rebased against master.

ashie
ashie requested changes Mar 9, 2021
View reviewed changes

@ashie ashie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please follow DCO https://github.com/fluent/fluentd/pull/3283/checks?check_run_id=2063272446

@kenhys kenhys force-pushed the fix-in-tail-windows branch 2 times, most recently from 895d365 to 9daac78 Compare March 9, 2021 06:40
cosmo0920
cosmo0920 approved these changes Mar 9, 2021
View reviewed changes

@cosmo0920 cosmo0920 left a comment

Copy link
Copy Markdown
Contributor

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.

@kenhys
test in_tail: use safer method to cleanup
392b6d4 
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 <kenhys@gmail.com>
@kenhys
test: add missing tmpdir
c78301a 
Signed-off-by: Kentaro Hayashi <kenhys@gmail.com>
@kenhys kenhys force-pushed the fix-in-tail-windows branch from 9daac78 to c78301a Compare March 9, 2021 07:15
@kenhys

kenhys commented Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

As #3289 was merged, then rebased again.

@cosmo0920

cosmo0920 commented Mar 9, 2021

Copy link
Copy Markdown
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
@ashie ashie mentioned this pull request Mar 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashie ashie ashie approved these changes

@cosmo0920 cosmo0920 cosmo0920 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kenhys @cosmo0920 @ashie