-
Notifications
You must be signed in to change notification settings - Fork 1k
LoadBalancer toggles for master and replica pooler pods #1799
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
Conversation
Signed-off-by: Sergey Shatunov <me@prok.pw>
Signed-off-by: Sergey Shatunov <me@prok.pw>
|
||
c.setProcessName("updating %v service", role) | ||
|
||
if c.Services[role] == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this check removed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove all the c.Service[role]
mentions in this function, hence the new oldService
argument. If you look at the place where updateService
is called c.Services[role] / c.ConnectionPooler[role].Service = svc is set just before. So this check is not needed - actually it was never really needed 😃 .
} | ||
|
||
func (c *Cluster) servicePort(role PostgresRole) string { | ||
func (c *Cluster) servicePort(role PostgresRole) int32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is better renamed generateServicePort
- otherwise the logic of returning 5432
when a service does not exist is vauge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes something like getServicePort
would be better. But, now it's also in line with the other functions like serviceName
, serviceAddress
. I think, I would keep the name.
pkg/cluster/k8sres.go
Outdated
c.logger.Warningf("No service for role %s", role) | ||
return "" | ||
c.logger.Warningf("No service for role %s - defaulting to port 5432", role) | ||
return 5432 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 |
1 similar comment
👍 |
Continuation from #943.
Closes #943
Fixes #1774
Adding LB support for pooler pods. Required implementing update logic for pooler services.