spacewander commented on code in PR #7749: URL: https://github.com/apache/apisix/pull/7749#discussion_r953429892
########## t/plugin/log-rotate3.t: ########## @@ -0,0 +1,143 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +use t::APISIX 'no_plan'; + +repeat_each(1); +no_long_string(); +no_shuffle(); +no_root_location(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!defined $block->yaml_config) { + my $yaml_config = <<_EOC_; +apisix: + node_listen: 1984 + admin_key: ~ +plugins: + - log-rotate +plugin_attr: + log-rotate: + interval: 86400 + max_size: 9 + max_kept: 3 + enable_compression: false +_EOC_ + + $block->set_value("yaml_config", $yaml_config); + } + + if ((!defined $block->error_log) && (!defined $block->no_error_log)) { + $block->set_value("no_error_log", "[error]"); + } + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } + +}); + +run_tests; + +__DATA__ + +=== TEST 1: log rotate by max_size +--- config + location /t { + content_by_lua_block { + ngx.log(ngx.ERR, "start xxxxxx") + ngx.sleep(2) + local has_split_access_file = false + local has_split_error_file = false + local lfs = require("lfs") + for file_name in lfs.dir(ngx.config.prefix() .. "/logs/") do + if string.match(file_name, "__access.log$") then + has_split_access_file = true + end + + if string.match(file_name, "__error.log$") then + has_split_error_file = true + end + end + + if not has_split_access_file and has_split_error_file then + ngx.status = 200 + else + ngx.status = 500 + end + } + } +--- error_code eval Review Comment: As we already check the default error_code 200 by default, we don't need this. ########## apisix/plugins/log-rotate.lua: ########## @@ -219,18 +209,66 @@ local function init_default_logs(logs_info, log_type) end +local function file_size(file) + local attr = lfs.attributes(file) + if attr then + return attr.size + end + return 0 +end + + +local function rotate_file(files, now_time, max_kept) + for _, file in ipairs(files) do + local now_date = os_date("%Y-%m-%d_%H-%M-%S", now_time) + local new_file = rename_file(default_logs[file], now_date) + if not new_file then + return + end + + local pid = process.get_master_pid() + + core.log.warn("send USR1 signal to master process [", pid, "] for reopening log file") + + if (pid) then + local ok, err = signal.kill(pid, signal.signum("USR1") or SIGUSR1) + if not ok then + core.log.error("failed to send USR1 signal for reopening log file: ", err) + end + end + + if enable_compression then + compression_file(new_file) + end + + -- clean the oldest file + local log_list, log_dir = scan_log_folder(file) + for i = max_kept + 1, #log_list do + local path = log_dir .. log_list[i] + local ok, err = os_remove(path) + if err then + core.log.error("remove old log file: ", path, " log: ", err, " res:", ok) + end + end + end +end + + local function rotate() local interval = INTERVAL local max_kept = MAX_KEPT + local max_size = -1 Review Comment: Could you use a module-level constant like the others? ########## apisix/plugins/log-rotate.lua: ########## @@ -49,6 +50,7 @@ local default_logs local enable_compression = false local DEFAULT_ACCESS_LOG_FILENAME = "access.log" local DEFAULT_ERROR_LOG_FILENAME = "error.log" +local SIGUSR1 = 10 Review Comment: Why hardcode the signum? The number is platform-dependent. ########## apisix/plugins/log-rotate.lua: ########## @@ -248,53 +286,24 @@ local function rotate() return end - if now_time < rotate_time then - -- did not reach the rotate time - core.log.info("rotate time: ", rotate_time, " now time: ", now_time) - return - end + if now_time >= rotate_time then + local files = {DEFAULT_ACCESS_LOG_FILENAME, DEFAULT_ERROR_LOG_FILENAME} + rotate_file(files, now_time, max_kept) - local now_date = os_date("%Y-%m-%d_%H-%M-%S", now_time) - local access_new_file = rename_file(default_logs[DEFAULT_ACCESS_LOG_FILENAME], now_date) - local error_new_file = rename_file(default_logs[DEFAULT_ERROR_LOG_FILENAME], now_date) - if not access_new_file and not error_new_file then -- reset rotate time rotate_time = rotate_time + interval - return - end - - core.log.warn("send USR1 signal to master process [", - process.get_master_pid(), "] for reopening log file") - local ok, err = signal.kill(process.get_master_pid(), signal.signum("USR1")) - if not ok then - core.log.error("failed to send USR1 signal for reopening log file: ", err) - end - - if enable_compression then - compression_file(access_new_file) - compression_file(error_new_file) - end - - -- clean the oldest file - local log_list, log_dir = scan_log_folder() - for i = max_kept + 1, #log_list.error do - local path = log_dir .. log_list.error[i] - ok, err = os_remove(path) - if err then - core.log.error("remove old error file: ", path, " err: ", err, " res:", ok) - end - end - - for i = max_kept + 1, #log_list.access do - local path = log_dir .. log_list.access[i] - ok, err = os_remove(path) - if err then - core.log.error("remove old error file: ", path, " err: ", err, " res:", ok) + else + if max_size > 0 then Review Comment: We can use elseif -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
