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

in_calyptia_fleet: windows support. #7929

Merged
merged 7 commits into from
Sep 21, 2023
Merged

in_calyptia_fleet: windows support. #7929

merged 7 commits into from
Sep 21, 2023

Conversation

pwhelan
Copy link
Contributor

@pwhelan pwhelan commented Sep 15, 2023

Summary

This PR implemenets hot reload in windows for the in_calyptia_fleet plugin. It includes the necessary changes for the plugin itself as well as rudimentary support for hot-reload under win32.

Obstacles

The calyptia fleet plugin reloads configuration taken from calyptia, which requires hot-reload in windows. This requires two major changes:

Solutions

TLS

The first concern was fixed in monkey and is still awaiting merging upstream to fluent-bit.

SIGHUP

I used GenerateConsoleCtrlEvent to simulate the behavior of the SIGHUP signal in windows and implemented a simple mechanism to signal the main thread from the GenerateConsoleCtrlEvent handler. This is so the main thread can execute flb_reload.

The implementation I went with uses a simple int to mark that the signal has been raised. The main loop then checks it, marks it as zero and then executes flb_reload. This is, admittedly, functional but not elegant.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

src/fluent-bit.c Outdated Show resolved Hide resolved
src/fluent-bit.c Show resolved Hide resolved
@pwhelan pwhelan temporarily deployed to pr September 15, 2023 15:51 — with GitHub Actions Inactive
@pwhelan pwhelan temporarily deployed to pr September 15, 2023 15:51 — with GitHub Actions Inactive
@pwhelan pwhelan force-pushed the pwhelan-fleet-windows branch from b2234f8 to 23942ed Compare September 15, 2023 16:14
@pwhelan pwhelan temporarily deployed to pr September 15, 2023 16:15 — with GitHub Actions Inactive
@pwhelan pwhelan temporarily deployed to pr September 15, 2023 16:15 — with GitHub Actions Inactive
@pwhelan pwhelan temporarily deployed to pr September 15, 2023 17:09 — with GitHub Actions Inactive
@pwhelan pwhelan temporarily deployed to pr September 15, 2023 17:09 — with GitHub Actions Inactive
@pwhelan pwhelan temporarily deployed to pr September 15, 2023 17:09 — with GitHub Actions Inactive
@pwhelan pwhelan temporarily deployed to pr September 15, 2023 17:35 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens added the ok-package-test Run PR packaging tests label Sep 15, 2023
@niedbalski niedbalski requested a review from edsiper September 18, 2023 09:54
@pwhelan
Copy link
Contributor Author

pwhelan commented Sep 18, 2023

There is still a blocker concerning the compile error on 32 bit windows which I have yet to resolve.

@patrick-stephens
Copy link
Contributor

There is still a blocker concerning the compile error on 32 bit windows which I have yet to resolve.

This one? https://ci.appveyor.com/project/fluent/fluent-bit-2e87g/builds/48047875/job/ubcx7x6cxx26ftkj
That looks like the usual flaky test...

[ FAILED ]
  log.c:16: Check (*interval >= timeout - 1) && *interval <= timeout... failed
    interval error. got=3 expect=4-5
  log.c:92: Check ret == 0... failed
    update_and_check_interval for TEST_RECORD_01 failed. i=9
  log.c:16: Check (*interval >= timeout - 1) && *interval <= timeout... failed
    interval error. got=3 expect=4-5
  log.c:98: Check ret == 0... failed
    update_and_check_interval for TEST_RECORD_02 failed. i=9
Test cache_one_slot...                          
[ OK ]
FAILED: 1 of 2 unit tests has failed.

@pwhelan
Copy link
Contributor Author

pwhelan commented Sep 18, 2023

It was failing on compile before. Seems like my last commit fixed it.

@edsiper
Copy link
Member

edsiper commented Sep 18, 2023

@pwhelan I merged #7925 which generated some conflicts here, please fix the conflicts.

@pwhelan
Copy link
Contributor Author

pwhelan commented Sep 20, 2023

I have resolved the conflicts and added a wrapper for readlink for win32. (I opened the PR #7951 to merge the fix).

The commit 59270d8 from monkey still needs to be integrated before windows reload can work. Without the calyptia fleet plugin will crash during co-routine setup.

@pwhelan pwhelan force-pushed the pwhelan-fleet-windows branch from 02d3a2c to 141c3c2 Compare September 20, 2023 14:50
@pwhelan pwhelan temporarily deployed to pr September 20, 2023 14:51 — with GitHub Actions Inactive
@pwhelan pwhelan temporarily deployed to pr September 20, 2023 14:51 — with GitHub Actions Inactive
@pwhelan pwhelan temporarily deployed to pr September 20, 2023 14:51 — with GitHub Actions Inactive
@pwhelan pwhelan mentioned this pull request Sep 20, 2023
7 tasks
@pwhelan pwhelan temporarily deployed to pr September 20, 2023 15:20 — with GitHub Actions Inactive
@edsiper edsiper merged commit 0f1f3e6 into master Sep 21, 2023
71 of 74 checks passed
@edsiper edsiper deleted the pwhelan-fleet-windows branch September 21, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-required ok-package-test Run PR packaging tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants