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

refactor: add log after apisix.yml config reload in standalone mode #10404

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

markokocic
Copy link
Contributor

Fixes #10402

Please review. Not sure why is the log message printed multiple times and how to avoid it.

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)

Fixes apache#10402

Please review. Not sure why is the log message printed multiple times and how to avoid it.
@markokocic markokocic changed the title Add log message after apisix.yml config reload fix: Add log message after apisix.yml config reload Oct 26, 2023
@shreemaan-abhishek shreemaan-abhishek changed the title fix: Add log message after apisix.yml config reload chore: add log message after apisix.yml config reload Oct 26, 2023
@monkeyDluffy6017
Copy link
Contributor

The error log will be flooded by you newly added message.

 ngx.timer.every(1, read_apisix_yaml)

@markokocic
Copy link
Contributor Author

Hi @monkeyDluffy6017
read_apisix_yaml is indeed called every second, but there's already a guard in place to skip execution if file is not changed:

    local last_change_time = attributes.change
    if apisix_yaml_ctime == last_change_time then
        return
    end

Log statement is added at the end of the method, after successful execution.

@monkeyDluffy6017
Copy link
Contributor

I don't think it's very necessary, we don't have this log when use etcd too, the number of logs is also very lagre even the log is printed when you send a new configuration

@markokocic
Copy link
Contributor Author

This additional log entry won't much increase the log size, since it's applied only when config file changes, which is not very frequent, depending on the usage scenario.

If we add it, the benefit would be that we can confirm when changes are applied successfully.

As for the log verbosity, I created a new ticket that is not related to this topic, but for log entry duplication in general. See #10414

@monkeyDluffy6017
Copy link
Contributor

Some of our users change their configurations very frequently, like thousands of times in 1s

@markokocic
Copy link
Contributor Author

Some of our users change their configurations very frequently, like thousands of times in 1s

In apisix.yml in standalone mode? I doubt how is that supposed to work, given that APISIX reads the config file only once per second.

@monkeyDluffy6017
Copy link
Contributor

What I mean is that even in standalone mode, configuration changes are frequent and should not be logged every time a configuration change is made without an error. This information is meaningless to most users. If you think this is necessary, you can keep this change yourself.

@monkeyDluffy6017
Copy link
Contributor

I will close this pr, feel free to reopen it

@polaru
Copy link

polaru commented Dec 4, 2023

I agree with @markokocic that when running apisix in standalone mode you don't get a lot of reloads; if ones needs that, they would use etcd or another config plane, not a static file.

@monkeyDluffy6017: running apisix in standalone mode in a kubernetes pod, getting apisix.yaml from a configMap, I would like to know when the pod has finished loading the configuration so I can start sending traffic through it. how do you suggest to accomplish this?

If you think this is necessary, you can keep this change yourself.
@monkeyDluffy6017 what do you mean by this? fork apisix?

@monkeyDluffy6017
Copy link
Contributor

if ones needs that, they would use etcd or another config plane, not a static file.

Sounds reasonable.

@AlinsRan
Copy link
Contributor

AlinsRan commented Dec 5, 2023

@monkeyDluffy6017: running apisix in standalone mode in a kubernetes pod, getting apisix.yaml from a configMap, I would like to know when the pod has finished loading the configuration so I can start sending traffic through it. how do you suggest to accomplish this?

I agree with your point that static files usually do not change frequently.
We need to confirm whether the apisix.yaml configuration has been updated through logs.

@monkeyDluffy6017 monkeyDluffy6017 changed the title chore: add log message after apisix.yml config reload refactor: add log after apisix.yml config reload in standalone mode Dec 5, 2023
@monkeyDluffy6017 monkeyDluffy6017 merged commit fe892b0 into apache:master Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants