Skip to content

do not create endpoints when use config maps#1760

Merged
FxKu merged 2 commits into
zalando:masterfrom
evsasha:fix/use-config-maps-true
Mar 28, 2022
Merged

do not create endpoints when use config maps#1760
FxKu merged 2 commits into
zalando:masterfrom
evsasha:fix/use-config-maps-true

Conversation

@evsasha
Copy link
Copy Markdown
Contributor

@evsasha evsasha commented Jan 27, 2022

This PR disable create endpoins when option kubernetes_use_configmaps set to true.

I did not change RBAC, the operator works correctly with existing rules

  - verbs:
      - get
    apiGroups:
      - ''
    resources:
      - endpoints

and also works without them

Tested on OpenShift 4.6.

Fix #1740, Fix #1758

@evsasha
Copy link
Copy Markdown
Contributor Author

evsasha commented Jan 27, 2022

I finded one more bug.

Operator can't delete configMap with name {clustername}-leader, but it can delete configMap {clustername}-config.

I think it's because there is no such suffix

	patroniObjectSuffixes = []string{"config", "failover", "sync"}

@evsasha
Copy link
Copy Markdown
Contributor Author

evsasha commented Jan 27, 2022

I tested this fix and get correct output on cluster delete event test444 in namespace intsess

time="2022-01-27T15:49:29Z" level=debug msg="removing leftover Patroni objects (endpoints / services and configmaps)" cluster-name=intsess/intsess-test444 pkg=cluster worker=1
time="2022-01-27T15:49:29Z" level=debug msg="deleting Patroni cluster object \"configmap\" with name \"intsess/intsess-test444-config\"" cluster-name=intsess/intsess-test444 pkg=cluster worker=1
time="2022-01-27T15:49:29Z" level=debug msg="deleting Patroni cluster object \"configmap\" with name \"intsess/intsess-test444-leader\"" cluster-name=intsess/intsess-test444 pkg=cluster worker=1

@FxKu
Copy link
Copy Markdown
Member

FxKu commented Feb 1, 2022

I would expect a replacement ConfigMap to be created in case kubernetes_use_configmaps is enabled. So instead of CreateEndpoint, call CreateConfigMap. Same for Delete. What does OpenShift need here?

@evsasha
Copy link
Copy Markdown
Contributor Author

evsasha commented Feb 2, 2022

@FxKu, the way that you suggest is a bit more complex than my solution:

  1. I don't quite understand at what point configmaps are created. I need help with this point.

  2. Currently configmaps are deleted by deletePatroniClusterConfigMaps()
    Do we need different function for delete like deleteConfigmap()?

  3. I don't see configmaps in type kubeResources. Shall we provide additional types?

@ghost ghost mentioned this pull request Mar 4, 2022
@FxKu
Copy link
Copy Markdown
Member

FxKu commented Mar 25, 2022

@evsasha sorry for the late reply. I've just learned that the operator does not need to create the ConfigMaps, since Patroni does it for you. And deletion code is already in place as you mentioned. So is setting selector for the master service. Then there's only one last thing to do - defining a readinessProbe for the postgres container in the statefulSet template. I think, it makes to add it after the generation of the spiloContainer.

@FxKu FxKu added this to the 1.8 milestone Mar 25, 2022
@evsasha
Copy link
Copy Markdown
Contributor Author

evsasha commented Mar 27, 2022

Then there's only one last thing to do - defining a readinessProbe for the postgres container in the statefulSet template. I think, it makes to add it after the generation of the spiloContainer.

FxKu I don't agree with you at this point. Adding probes is not a goal of this PR. I spent some time for research and didn't get the proper result (i couldn't get it to work). I propose not to make this change in that PR.

Moreover, there is a separate task for this: #587

@FxKu
Copy link
Copy Markdown
Member

FxKu commented Mar 28, 2022

@evsasha the readinessProbe we need only in case patroniKubernetesUseConfigMaps is true. But, I'm fine doing it in a separate PR.

@FxKu
Copy link
Copy Markdown
Member

FxKu commented Mar 28, 2022

👍

1 similar comment
@jopadi
Copy link
Copy Markdown
Member

jopadi commented Mar 28, 2022

👍

@FxKu FxKu merged commit 30f2ba6 into zalando:master Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Insufficient permissions when kubernetes_use_configmaps is true When using ConfigMaps operator stiil need permission to create endpoints

3 participants