Add CONFIG SET and GET loglevel feature in Sentinel #11214

hwware · 2022-08-30T18:59:09Z

Till now Sentinel allowed modifying the log level in the config file, but not at runtime.
this makes it possible to tune the log level at runtime

enjoy-binbin

how about using config get, like config get loglevel?

btw, i am curious why the client needs this information and is there any usecase?

oranagra · 2022-08-31T07:30:49Z

i'm also not certain why we want this info to be visible in the INFO command.
arguably, we can push the entire CONFIG into the INFO command, but that doesn't make any sense.
i think the only few things that make sense are configs that are useful for monitoring systems.
like maxmemory to compare to used_memory, and maxclients to compare to connected_clients.

hwware · 2022-08-31T14:12:18Z

@oranagra @enjoy-binbin The reason I add this feature is that in Sentinel Mode

Monitors have no way to know current loglevel and logfile location, config get loglevel does not work in sentinel mode and sentinel config get loglevel do not support this feature as well.
There is no way to change the loglevel by using config set loglevel XXX in sentinel mode..

Thanks.

oranagra · 2022-09-01T05:58:27Z

ok, so i think the problem is with sentinel, it does parse the redis conf file at startup, but offers no way to interact with the config at runtime, maybe that's what we should fix.
@moticless @yossigo WDYT?

moticless · 2022-09-01T07:06:45Z

@oranagra,
As sentinel is actually a modified redis-server, it hides many unused redis-server configuration.
Exposing config get * will expose all those unused parameters.
On the other hand it doesn't support natively sentinel configuration.
It requires to refactor config infrastructure. I doubt it worth the effort for sentinel alone.

IMHO, there is no need to expose this arguments. I do see that loglevel is not documented
at file sentinel.conf, maybe this is what we should only need to fix.

hwware · 2022-09-01T13:59:10Z

@moticless In sentinel, so far there is no way to check current the loglevel and change the loglevel, do you think it makes sense to provide the function similar to redis server: config get loglevel and config set loglvel XXX

moticless · 2022-09-01T14:22:35Z

@hwware, I think it can be nice to have such option. If we add setter to loglevel, then we also need getter. Anyway, we need to add loglevel section to sentinel.conf.

hwware · 2022-09-01T14:24:27Z

@hwware, I think it can be nice to have such option. If we add setter to loglevel, then we also need getter. Anyway, we need to add loglevel section to sentinel.conf.

ok, I will follow your idea to finish this function and let you know when I update the code in this PR, thanks,

oranagra

LGTM.
@moticless?

oranagra · 2022-10-22T18:24:13Z

@hwware please copy the loglevel documentation to sentinel.conf

hwware · 2022-10-24T18:44:11Z

@oranagra @moticless Sorry mislead you, some features are not finished yet. I change the title to WIP status

hwware · 2022-11-14T19:19:04Z

@moticless @oranagra Feature and test case ar done, please do a code review when you have time, Thanks a lot

moticless · 2022-11-14T20:38:07Z

src/sentinel.c

 /* =========================== SENTINEL command ============================= */

-/* SENTINEL CONFIG SET <option> */
+const char* getLogLevel(){


Suggested change

const char* getLogLevel(){

const char* getLogLevel() {

Update it. Thanks

moticless · 2022-11-14T20:46:36Z

tests/sentinel/tests/01-conf-update.tcl

+
+test "Update log level" {
+    set current_loglevel [S 0 SENTINEL CONFIG GET loglevel]
+    assert {[lindex $$current_loglevel 1] == {notice}}


Suggested change

assert {[lindex $$current_loglevel 1] == {notice}}

assert {[lindex $current_loglevel 1] == {notice}}

moticless · 2022-11-14T20:46:50Z

tests/sentinel/tests/01-conf-update.tcl

+    assert {[lindex $$current_loglevel 1] == {notice}}
+    S 0 SENTINEL CONFIG SET loglevel warning
+    set updated_loglevel [S 0 SENTINEL CONFIG GET loglevel]
+    assert {[lindex $$updated_loglevel 1] == {warning}}


Suggested change

assert {[lindex $$updated_loglevel 1] == {warning}}

assert {[lindex $updated_loglevel 1] == {warning}}

Update it. Thanks. But in sentinel test case, double $$ can pass. Weird.

oranagra

@redis/core-team technically a major decision.
loglevel was supported in sentinel.conf, but not at runtime CONFIG SET and GET

Apparently we set `loglevel debug` for tls in spawn_instance. At the same time, in order to better distinguish the tests, change the name of `test-centos7-tls` to `test-centos7-tls-module`, change the name of `test-centos7-tls-no-tls` to `test-centos7-tls-module-no-tls-module`. Note that in `test-centos7-tls-module`, we did not pass `--tls-module` in sentinel test because it is not supported, see 4faddf1, added in redis#9320. So only test-ubuntu-tls fails in daily CI. The test was introduced in redis#11214.

enjoy-binbin · 2022-11-21T03:36:38Z

tests/sentinel/tests/01-conf-update.tcl

+
+test "Update log level" {
+    set current_loglevel [S 0 SENTINEL CONFIG GET loglevel]
+    assert {[lindex $current_loglevel 1] == {notice}}


this failed in test-ubuntu-tls: https://github.com/redis/redis/actions/runs/3510377800/jobs/5880112424

because we set loglevel to debug for tls, see #11528

Apparently we used to set `loglevel debug` for tls in spawn_instance. I.e. cluster and sentinel tests used to run with debug logging, only when tls mode was enabled. this was probably a leftover from when creating the tls mode tests. it cause a new test created for #11214 to fail in tls mode. At the same time, in order to better distinguish the tests, change the name of `test-centos7-tls` to `test-centos7-tls-module`, change the name of `test-centos7-tls-no-tls` to `test-centos7-tls-module-no-tls`. Note that in `test-centos7-tls-module`, we did not pass `--tls-module` in sentinel test because it is not supported, see 4faddf1, added in #9320. So only `test-ubuntu-tls` fails in daily CI. Co-authored-by: Oran Agra <oran@redislabs.com>

Till now Sentinel allowed modifying the log level in the config file, but not at runtime. this makes it possible to tune the log level at runtime

Apparently we used to set `loglevel debug` for tls in spawn_instance. I.e. cluster and sentinel tests used to run with debug logging, only when tls mode was enabled. this was probably a leftover from when creating the tls mode tests. it cause a new test created for redis#11214 to fail in tls mode. At the same time, in order to better distinguish the tests, change the name of `test-centos7-tls` to `test-centos7-tls-module`, change the name of `test-centos7-tls-no-tls` to `test-centos7-tls-module-no-tls`. Note that in `test-centos7-tls-module`, we did not pass `--tls-module` in sentinel test because it is not supported, see 4faddf1, added in redis#9320. So only `test-ubuntu-tls` fails in daily CI. Co-authored-by: Oran Agra <oran@redislabs.com>

We add a new loglevel 'nothing' to disable logging in redis#12133. This PR syncs that config change to sentinel. Because in redis#11214 we support modifying loglevel in runtime. Although I think sentinel doesn't need this nothing config, it's better to be consistent.

We add a new loglevel 'nothing' to disable logging in #12133. This PR syncs that config change to sentinel. Because in #11214 we support modifying loglevel in runtime. Although I think sentinel doesn't need this nothing config, it's better to be consistent.

Till now Sentinel allowed modifying the log level in the config file, but not at runtime. this makes it possible to tune the log level at runtime

Apparently we used to set `loglevel debug` for tls in spawn_instance. I.e. cluster and sentinel tests used to run with debug logging, only when tls mode was enabled. this was probably a leftover from when creating the tls mode tests. it cause a new test created for redis#11214 to fail in tls mode. At the same time, in order to better distinguish the tests, change the name of `test-centos7-tls` to `test-centos7-tls-module`, change the name of `test-centos7-tls-no-tls` to `test-centos7-tls-module-no-tls`. Note that in `test-centos7-tls-module`, we did not pass `--tls-module` in sentinel test because it is not supported, see 4faddf1, added in redis#9320. So only `test-ubuntu-tls` fails in daily CI. Co-authored-by: Oran Agra <oran@redislabs.com>

hwware requested a review from oranagra August 30, 2022 19:07

enjoy-binbin reviewed Aug 31, 2022

View reviewed changes

oranagra added the state:to-be-closed requesting the core team to close the issue label Aug 31, 2022

enjoy-binbin removed the state:to-be-closed requesting the core team to close the issue label Sep 2, 2022

hwware force-pushed the add-log-info branch from 2ba86ac to e155bd9 Compare September 21, 2022 19:38

hwware force-pushed the add-log-info branch from e155bd9 to 83fd8bc Compare October 20, 2022 17:57

oranagra approved these changes Oct 22, 2022

View reviewed changes

oranagra changed the title ~~Add log information in the info command~~ Add CONFIG SET loglevel to Sntinel Oct 22, 2022

hwware changed the title ~~Add CONFIG SET loglevel to Sntinel~~ Add CONFIG SET loglevel to Sentinel Oct 24, 2022

hwware changed the title ~~Add CONFIG SET loglevel to Sentinel~~ WIP---Add CONFIG SET loglevel to Sentinel Oct 24, 2022

hwware added 3 commits November 14, 2022 15:17

add some information for loglevel

a990a28

avoid memory leak

4e254c0

Add feature that sentinel can get loglevel

8697f19

hwware force-pushed the add-log-info branch from baefa40 to 8697f19 Compare November 14, 2022 15:19

add set loglevel feature and test case

f10c652

hwware changed the title ~~WIP---Add CONFIG SET loglevel to Sentinel~~ Add CONFIG SET and GET loglevel feature in Sentinel Nov 14, 2022

moticless reviewed Nov 14, 2022

View reviewed changes

Address Moti comments

152f85e

oranagra approved these changes Nov 16, 2022

View reviewed changes

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Nov 16, 2022

oranagra added the approval-needed Waiting for core team approval to be merged label Nov 16, 2022

madolson approved these changes Nov 16, 2022

View reviewed changes

moticless approved these changes Nov 16, 2022

View reviewed changes

yossigo approved these changes Nov 18, 2022

View reviewed changes

oranagra merged commit 2f41177 into redis:unstable Nov 20, 2022

enjoy-binbin mentioned this pull request Nov 21, 2022

Fix sentinel update loglevel tls test #11528

Merged

enjoy-binbin reviewed Nov 21, 2022

View reviewed changes

enjoy-binbin mentioned this pull request May 24, 2023

Sync the new loglevel nothing to sentinel #12223

Merged

	assert {[lindex $$current_loglevel 1] == {notice}}
	assert {[lindex $current_loglevel 1] == {notice}}

	assert {[lindex $$updated_loglevel 1] == {warning}}
	assert {[lindex $updated_loglevel 1] == {warning}}

Add CONFIG SET and GET loglevel feature in Sentinel #11214

Add CONFIG SET and GET loglevel feature in Sentinel #11214

Uh oh!

Conversation

hwware commented Aug 30, 2022 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enjoy-binbin left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra commented Aug 31, 2022

Uh oh!

hwware commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Sep 1, 2022

Uh oh!

moticless commented Sep 1, 2022

Uh oh!

hwware commented Sep 1, 2022

Uh oh!

moticless commented Sep 1, 2022

Uh oh!

hwware commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra commented Oct 22, 2022

Uh oh!

hwware commented Oct 24, 2022

Uh oh!

hwware commented Nov 14, 2022

Uh oh!

moticless Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

hwware Nov 15, 2022

Choose a reason for hiding this comment

Uh oh!

moticless Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

moticless Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

hwware Nov 15, 2022

Choose a reason for hiding this comment

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

enjoy-binbin Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hwware commented Aug 30, 2022 •

edited by oranagra

Loading

hwware commented Aug 31, 2022 •

edited

Loading

hwware commented Sep 1, 2022 •

edited

Loading