Skip to content

[DPE-2565] Open port #159

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

Merged
merged 1 commit into from
Oct 26, 2023
Merged

[DPE-2565] Open port #159

merged 1 commit into from
Oct 26, 2023

Conversation

dragomirp
Copy link
Contributor

Issue

Ports are not patched on the K8s service.

Solution

Use open port call from ops if available, patch directly if not.

namespace=service.metadata.namespace,
force=True,
field_manager=self.app.name,
patch_type=lightkube.types.PatchType.MERGE,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to merge to update the port when it changes. This nukes the placeholder port that's currently added to the service by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a placeholder port 65535 (since we declare the real one)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understood from here, 65535 is not needed (it was needed only to create the service, but now we have another port).

@dragomirp dragomirp marked this pull request as ready for review October 24, 2023 13:28
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Please mention in PR message: Close #140

P.S. IMHO we do not need placeholder port anymore => no need to merge.
E.g. postgresql services have no placeholder ports, the same for patroni.

ubuntu@juju31:~$ kubectl get services -A
NAMESPACE             NAME                            TYPE        CLUSTER-IP       EXTERNAL-IP   PORT(S)                  AGE
...
test18                postgresql-k8s                  ClusterIP   10.152.183.17    <none>        5432/TCP,8008/TCP        25h
test18                postgresql-k8s-primary          ClusterIP   10.152.183.140   <none>        8008/TCP,5432/TCP        25h
test18                postgresql-k8s-replicas         ClusterIP   10.152.183.215   <none>        8008/TCP,5432/TCP        25h
test18                patroni-postgresql-k8s-config   ClusterIP   None             <none>        <none>                   25h

P.S. Juju creates it here (and never used further).

namespace=service.metadata.namespace,
force=True,
field_manager=self.app.name,
patch_type=lightkube.types.PatchType.MERGE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a placeholder port 65535 (since we declare the real one)?

@dragomirp
Copy link
Contributor Author

P.S. IMHO we do not need placeholder port anymore => no need to merge. E.g. postgresql services have no placeholder ports, the same for patroni.

We need to merge, because that will update the pgbouncer port on config change. It will also delete the placeholder port since, it's not in the new port spec.

namespace=service.metadata.namespace,
force=True,
field_manager=self.app.name,
patch_type=lightkube.types.PatchType.MERGE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understood from here, 65535 is not needed (it was needed only to create the service, but now we have another port).

@dragomirp dragomirp merged commit ede1d8b into main Oct 26, 2023
@dragomirp dragomirp deleted the dpe-2565-open-port branch October 26, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants