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

Add application version within the management protocol #392

Closed
fluca1978 opened this issue Dec 9, 2023 · 11 comments
Closed

Add application version within the management protocol #392

fluca1978 opened this issue Dec 9, 2023 · 11 comments
Labels
feature New feature

Comments

@fluca1978
Copy link
Collaborator

While working on #391 I got the idea that, since pgagroal-cli and pragroal are communicating over a socket thru the management protocol, chances are that the two applications are not at the very same version.
In order to better diagnose problems and even receive a better and more constant behavior, we should transmit the version of pgagroal-cli and pgagroal thru the socket, so that both parties have information about who they are talking to.
Moreover, this could be a chance to even block request made by older versions of any of the two to the other part.

My idea is to add an header-like block to any request sent out from pgagroal-cli, having pgagroal to get it in the begin of the message (so, at a constant offset) and viceversa, having pgagroal to send out the same information as the first part of the communication protocol.

This should not require a lot of changes in the management protocol implementation.
However, I would postpone this to #391 because importing JSON output will speed up the debugging of this implementation.

@fluca1978 fluca1978 added the feature New feature label Dec 9, 2023
@jesperpedersen
Copy link
Collaborator

👍 to the idea.

We just have to figure out what we want in that header... could also be stuff like the entire size of the management invocation

@fluca1978
Copy link
Collaborator Author

👍 to the idea.

We just have to figure out what we want in that header... could also be stuff like the entire size of the management invocation

Well, since we are going to add stuff, I think there is room for placing a size of the request.
At this point, I would also put the client (e.g., pgagroal-cli ) that did the request.

@jesperpedersen
Copy link
Collaborator

jesperpedersen commented Dec 11, 2023

Yeah, the name of the client plus its version number would be a good idea.

The response should include the version of the pgagroal instance.

An idea could be to be to create a variable length header where each "section" is identified by a unique char - like ...cpgagroal-cli\0v1.6.0\0... with a response of ...Spgagroal\0V1.6.0\0...

@fluca1978
Copy link
Collaborator Author

An idea could be to be to create a variable length header where each "section" is identified by a unique char - like ...cpgagroal-cli\0v1.6.0\0... with a response of ...Spgagroal\0V1.6.0\0...

I think that changing write_header https://github.com/agroal/pgagroal/blob/master/src/libpgagroal/management.c#L1488 (and consequently, pgagroal_managament_read_header https://github.com/agroal/pgagroal/blob/master/src/libpgagroal/management.c#L66) to interact with the extended headers.
But in my opinion, sections should be identified by a fixed letter, for example F as from (the fact that is a server or a client depends on the side we read the data), V for version, etc.
Dealing with headers means that the headaers have to be accessible in some way to the "client" methods, like pgagroal_management_read_something, so I'm still thinking if instead of dealing with headers we should deal with custom payloads.

@jesperpedersen
Copy link
Collaborator

Sounds good - a prototype of it will likely give a clearer indication of what is the best solution

@fluca1978
Copy link
Collaborator Author

Sounds good - a prototype of it will likely give a clearer indication of what is the best solution

Ok, I think this will be done in the next year, and after #391 has completed.

@jesperpedersen
Copy link
Collaborator

👍

@HazemRawi
Copy link

is this a possible GSoC 2024 project?

@fluca1978
Copy link
Collaborator Author

is this a possible GSoC 2024 project?

I don't think so: this is a quite short (even if not so trivial) change to the communication protocol.
@jesperpedersen could add more on this, and has the very last word (obviously).

@fluca1978
Copy link
Collaborator Author

Sounds good - a prototype of it will likely give a clearer indication of what is the best solution

Ok, I think this will be done in the next year, and after #391 has completed.

Meanwhile, I was thinking that either this change to the communication protocol can either broke backward compatibility or not.
I think the message should include information about the client/server version (and other details) in the beginning of the message, i.e., before the actual payload. Doing so, however, would break backward compatibility, because a new pgagroal-cli cannot talk anymore with an old pgagroal and viceversa.
One possible solution could be to make a fail path, so that if the "new" information is not found, the protocol is assumed to be old and we proceed immediatly to the payload. But I don't like this idea, since it exposes the risk of misinterpreting a piece of payload as "new" information.
Probably this poses the question about putting a protocol version number into the messages, so that pgagroal and pgagroal-cli could possibly decide if they can handle the message correctly.
But I see backward compatibility breaks in here.

On the other hand, we could add the "new" information at the very end of the message, but this would prevent a possible compatibility check (e.g., pgagroal-cli 1.6 talking to pgagroal 2), so I don't like the idea neither.

@jesperpedersen
Copy link
Collaborator

Getting new ideas into the project should not block the progress.

If we decide to change the CLI and management protocol that is enough for a version 2.0 -- it doesn't have to be at a pool level.

And yes, changing the management protocol isn't "large" enough for a GSoC 2024 project, even for a short project.

EuGig added a commit to EuGig/pgagroal that referenced this issue Mar 18, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version into the header
over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_header_info' function in
management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Mar 25, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version into the header
over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_header_info' function in
management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Mar 27, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version into the header
over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_header_info' function in
management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 3, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version into the header
over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_header_info' function in
management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 3, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version into the header
over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_header_info' function in
management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 6, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_info' function in management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 6, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_info' function in management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 11, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_info' function in management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 12, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_info' function in management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 16, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_info' function in management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 22, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_info' function in management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 22, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_info' function in management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 24, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.

Introduces the 'write_info' function in management.c.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 25, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.
EuGig added a commit to EuGig/pgagroal that referenced this issue Apr 29, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.
EuGig added a commit to EuGig/pgagroal that referenced this issue May 5, 2024
In order to improve debugging, [agroal#392] suggested to write
the sender application name and its version in the
socket over which pgagroal-cli and pgagroal communicate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

No branches or pull requests

3 participants