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

Redo CLI conf get|set|ls #485

Closed
jesperpedersen opened this issue Nov 9, 2024 · 8 comments
Closed

Redo CLI conf get|set|ls #485

jesperpedersen opened this issue Nov 9, 2024 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@jesperpedersen
Copy link
Collaborator

No description provided.

@jesperpedersen jesperpedersen added the bug Something isn't working label Nov 9, 2024
@fluca1978
Copy link
Collaborator

Testing head f34e4f9:

% pgagroal-cli conf get max_connections
pgagroal-cli: Unknown subcommand 'get' for command 'conf'

and most notably:

% pgagroal-cli conf reload             
-> WARN  management.c:1162 pgagroal-cli: read_byte: (nil) 3 Invalid argument
pgagroal-cli: No connection to pgagroal on /tmp

and for the last command conf reload the pgagroal logs show:

-> DEBUG main.c:1444 Management 12: {
  "Header": {
    "ClientVersion": "2.0.0",
    "Command": 12,
    "Compression": 0,
    "Encryption": 0,
    "Output": 0,
    "Timestamp": "20241113104305"
  },
  "Request": {}
}
-> DEBUG main.c:1776 pgagroal: Management reload
pgagroal: main: Unknown key <ev_backend> with value <io_uring> in section [pgagroal] (line 80 of file </etc/pgagroal/pgagroal.conf>)
-> DEBUG configuration.c:2016 Reload: Success
-> DEBUG main.c:2569 Socket: 7
-> DEBUG main.c:2569 Socket: 8
-> DEBUG main.c:2571 Unix Domain Socket: 5
-> DEBUG main.c:2574 Metrics: 11
-> DEBUG main.c:2574 Metrics: 12
-> DEBUG main.c:2578 Remote management: 13
-> DEBUG main.c:2578 Remote management: 14

fluca1978 added a commit to fluca1978/pgagroal that referenced this issue Nov 13, 2024
In the case of the `conf reload` command there were a couple of
explicit `exit` calls that terminated immediatly the `main`. They have
been changed to the boolean return value.

See agroal#485
@fluca1978
Copy link
Collaborator

With regard to the conf reload, I found that here https://github.com/agroal/pgagroal/blob/master/src/main.c#L2586 there is an explicit exit(0) (and similarly exit(1) on the error path), that is also not consistent with the function defintion that is supposed to return a bool.

fluca1978 added a commit to fluca1978/pgagroal that referenced this issue Nov 13, 2024
In the case of the `conf reload` command there were a couple of
explicit `exit` calls that terminated immediatly the `main`. They have
been changed to the boolean return value.

Implemented the `conf get` command.

See agroal#485
@fluca1978
Copy link
Collaborator

@jesperpedersen PTAL commit ea07bd4 that implements conf get to see if I get it right.

Please note that there is some sort of user after free bug somewhere, but I've not digged it a lot. It could be that destroying the JSON object after writing the response suffice.
Take a look at the following two executions: the first is ok, the secondis in error but reports the same configuration value for the requested key.

% pgagroal-cli conf get log_level 
Header: 
  ClientVersion: 2.0.0
  Command: 2
  Compression: none
  Encryption: none
  Output: text
  Timestamp: 20241113123146
Outcome: 
  Status: true
  Time: 00:00:00
Request: 
  ConfigurationKey: log_level
Response: 
  ServerVersion: 2.0.0
  log_level: debug

% pgagroal-cli conf get log_     
Header: 
  ClientVersion: 2.0.0
  Command: 2
  Compression: none
  Encryption: none
  Output: text
  Timestamp: 20241113123152
Outcome: 
  Error: 1
  Status: false
Request: 
  ConfigurationKey: log_
Response: 
  ServerVersion: 2.0.0
  log_: debug

@jesperpedersen
Copy link
Collaborator Author

@fluca1978 All the CONF_XYZ work will be ported from pgmoneta/pgmoneta#409 once done.

@fluca1978
Copy link
Collaborator

@fluca1978 All the CONF_XYZ work will be ported from pgmoneta/pgmoneta#409 once done.

Ok, so this is a "phantom" issue.
Hope at least my initial work can be useful.

@jesperpedersen
Copy link
Collaborator Author

Yeah, so it doesn't fly out of my head and we have a feature regression

@jesperpedersen
Copy link
Collaborator Author

@ashu3103 You are taking this one, right ?

@ashu3103
Copy link
Collaborator

ashu3103 commented Dec 1, 2024

@ashu3103 You are taking this one, right ?

Yeah, I was actually occupied with some other stuff this week. Will take this at the earliest

ashu3103 added a commit to ashu3103/pgagroal that referenced this issue Dec 7, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this issue Dec 9, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this issue Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants