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

fix: reload once when log rotate #7869

Merged

Conversation

monkeyDluffy6017
Copy link
Contributor

@monkeyDluffy6017 monkeyDluffy6017 commented Sep 6, 2022

Description

  1. Waiting for nginx reopen files to avoid losing logs during compression
  2. Refactor code to ensure that the signal is sent once at a time

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@monkeyDluffy6017 monkeyDluffy6017 changed the title Reload once when log rotate fix: reload once when log rotate Sep 6, 2022
@monkeyDluffy6017 monkeyDluffy6017 marked this pull request as draft September 6, 2022 08:38
@monkeyDluffy6017 monkeyDluffy6017 marked this pull request as ready for review September 6, 2022 09:37
@tokers
Copy link
Contributor

tokers commented Sep 6, 2022

Is it feasible to add some test cases?

@monkeyDluffy6017
Copy link
Contributor Author

monkeyDluffy6017 commented Sep 6, 2022

Is it feasible to add some test cases?

We have log-rotate.t, log-rotate2.t, log-rotate3.t 3 test files to test its functionality.
But I have no idea how to add test cases about losing log file or sending several reopen signal.


local new_tab = require "table.new"
local ngx_sleep = require("apisix.core.utils").sleep
local isempty = require "table.isempty"
Copy link
Member

Choose a reason for hiding this comment

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

Better to introduce it via core.table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if enable_compression then
-- Waiting for nginx reopen files
-- to avoid losing logs during compression
ngx_sleep(wait_time)
Copy link
Member

Choose a reason for hiding this comment

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

Will the sleep block the timer and cause the next rotation time incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait_time is just 0.5s by default. rotation time is much lager than 0.5s, right?

Copy link
Member

Choose a reason for hiding this comment

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

@monkeyDluffy6017
Currently, there is no limitation for the user-specific wait_time. BTW, we need to doc it if this argument is exposed to the user.

Copy link
Member

Choose a reason for hiding this comment

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

If the wait time is only for hardcode, maybe we can add extra sleep in the test so we don't need to set a special value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tzssangglass
tzssangglass previously approved these changes Sep 7, 2022
@spacewander spacewander merged commit 8b5c9b0 into apache:master Sep 8, 2022
hongbinhsu pushed a commit to fitphp/apix that referenced this pull request Sep 10, 2022
* upstream/master: (214 commits)
  feat: set constants.apisix_lua_home for used by plugins (apache#7893)
  fix: response-rewrite plugin might cause Apache AIPSIX hanging (apache#7836)
  test: sleep 1 second in t/cli/test_upstream_mtls.sh (apache#7889)
  fix: reload once when log rotate (apache#7869)
  change: move etcd conf under deployment (apache#7860)
  fix: plugin metadata missing v3 adapter call (apache#7877)
  docs: add ClickHouse and Elasticsearch loggers. (apache#7848)
  docs(plugin): refactor limit-conn.md (apache#7857)
  feat: call `destroy` method when Nginx exits (apache#7866)
  feat: Add ability to inject headers via prefix to otel traces (apache#7822)
  docs(plugin): refactor proxy-cache.md (apache#7858)
  fix: don't enable passive healthcheck by default (apache#7850)
  feat: add openfunction plugin (apache#7634)
  fix(zipkin): send trace IDs with a reject sampling decision (apache#7833)
  fix: Change opentelemetry's span kind to server (apache#7830)
  docs(hmac-auth): additional details for generating signing_string (apache#7816)
  docs: correct the test-nginx description (apache#7818)
  docs: add docs of workflow plugin (apache#7813)
  docs(FAQ): add how to detect APISIX alive status (apache#7812)
  feat: add elasticsearch-logger (apache#7643)
  ...
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
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.

5 participants