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.

Sign up for GitHub

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

Jump to bottom

Allow configuring port of http probes #30566

Merged
merged 4 commits into from
Feb 15, 2023
Merged

Allow configuring port of http probes #30566

merged 4 commits into from
Feb 15, 2023

Conversation

iocanel
Copy link
Contributor

@iocanel iocanel commented Jan 24, 2023

Resolves: #30547

@iocanel iocanel requested a review from geoand January 24, 2023 10:01
@iocanel
Copy link
Contributor Author

iocanel commented Jan 24, 2023

cc @cescoffier

@geoand
Copy link
Contributor

geoand commented Jan 24, 2023

I'll defer to @Sgitario who has been doing a lot of work in this area

cescoffier
cescoffier approved these changes Jan 24, 2023
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM, I should be able to change the port when the management interface is used by producing my own decorator.

@cescoffier cescoffier mentioned this pull request Jan 24, 2023
Merged
4 tasks
@quarkus-bot

This comment has been minimized.

Sign in to view
@iocanel
Copy link
Contributor Author

iocanel commented Jan 24, 2023

LGTM, I should be able to change the port when the management interface is used by producing my own decorator.

This is something you can do already, without this pull request. The pull request is more about exposing control to the user via configuration properties.

From your comment I feel that you prefer to control it from an extension. This can be done by registering your own decorator, but probe port handling differs in same platforms (e.g. knative) and we wouldn't want to expose that to the management bits. So, it should be cleaner if we had a different built it, that would just allow as select the probe port name. Let me add it.

@cescoffier
Copy link
Member

In my context, at build time, I know if the management interface is enabled or not. If yes, then the health checks (and others) are not exposed on the main HTTP server (using the http port) but use a separate server exposed on the management port.

My idea was to update the extension exposing routes to the management interface and passing the port name as you did for http.

@quarkus-bot

This comment has been minimized.

Sign in to view
cescoffier
cescoffier reviewed Jan 25, 2023
View reviewed changes
...anilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesCommonHelper.java
* @return a decorator for configures the port of the http action of the probe.
*/
public static DecoratorBuildItem createProbeHttpPortDecorator(String name, String target, ProbeConfig probeConfig,
Optional<KubernetesProbePortNameBuildItem> portName,
Copy link
Member

Choose a reason for hiding this comment

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

That's awesome! I just have to emit that build item if the management interface is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly the idea.

@iocanel iocanel requested a review from Sgitario January 25, 2023 13:14
Sgitario
Sgitario reviewed Jan 25, 2023
View reviewed changes
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

I'm ok with the changes, but I have a suggestion that I think it's cleaner. Let me know what you think about.

...kubernetes/spi/src/main/java/io/quarkus/kubernetes/spi/KubernetesProbePortNameBuildItem.java Outdated Show resolved Hide resolved
@quarkus-bot

This comment has been minimized.

Sign in to view
cescoffier
cescoffier requested changes Jan 26, 2023
View reviewed changes
...loyment/src/main/java/io/quarkus/kubernetes/deployment/ChangeDeploymentTriggerDecorator.java Outdated Show resolved Hide resolved
...loyment/src/main/java/io/quarkus/kubernetes/deployment/ChangeDeploymentTriggerDecorator.java Outdated Show resolved Hide resolved
...ubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/ProbeConfig.java Outdated Show resolved Hide resolved
...ubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/ProbeConfig.java Outdated Show resolved Hide resolved
...kubernetes/spi/src/main/java/io/quarkus/kubernetes/spi/KubernetesProbePortNameBuildItem.java Outdated Show resolved Hide resolved
...ubernetes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/ProbeConfig.java Outdated Show resolved Hide resolved
@quarkus-bot

This comment has been minimized.

Sign in to view
@cescoffier
Copy link
Member

@iocanel I guess, the failures are related. WDYT?

@iocanel
Copy link
Contributor Author

iocanel commented Jan 30, 2023

@iocanel I guess, the failures are related. WDYT?

They must be. I'll have a look later today

@Sgitario Sgitario mentioned this pull request Feb 1, 2023
Closed
Sgitario
Sgitario requested changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

I think we should address the issue in ChangeDeploymentTriggerDecorator and RemoveDeploymentTriggerDecorator in a separated pull request since it's nothing to do with allowing configuring ports of HTTP probes.
Let's address this unrelated issue in #30566

@quarkus-bot

This comment has been minimized.

Sign in to view
@iocanel
Copy link
Contributor Author

iocanel commented Feb 1, 2023

I think we should address the issue in ChangeDeploymentTriggerDecorator and RemoveDeploymentTriggerDecorator in a separated pull request since it's nothing to do with allowing configuring ports of HTTP probes. Let's address this unrelated issue in #30566

I don't really mind which is the PR that deals with it. It can be #30768 too. One thing that is not clear to me is how are you able to reproduce the issue in #30768? I've only been able to reproduce the issue in this PR.

@Sgitario
Copy link
Contributor

Sgitario commented Feb 2, 2023

I think we should address the issue in ChangeDeploymentTriggerDecorator and RemoveDeploymentTriggerDecorator in a separated pull request since it's nothing to do with allowing configuring ports of HTTP probes. Let's address this unrelated issue in #30566

I don't really mind which is the PR that deals with it. It can be #30768 too. One thing that is not clear to me is how are you able to reproduce the issue in #30768? I've only been able to reproduce the issue in this PR.

That's a good question. It seems not possible to always reproduce this issue using Quarkus CI, but it is in:

@gsmet gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Feb 4, 2023
@cescoffier
Copy link
Member

@iocanel Any ETA on this one?

@iocanel
Copy link
Contributor Author

iocanel commented Feb 8, 2023

@iocanel Any ETA on this one?

We'll have a dekorate release that will unblock this one later today!

@iocanel
Copy link
Contributor Author

iocanel commented Feb 9, 2023

@cescoffier: just bumped the PR. Hopefully it will pass now.

@iocanel iocanel removed area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation labels Feb 14, 2023
@github-actions
Copy link

🎊 PR Preview bfe2ae9 has been successfully built and deployed to https://quarkus-io-pr-main-30566-preview.surge.sh/version/main/guides/

@iocanel iocanel marked this pull request as ready for review February 15, 2023 05:51
@Sgitario Sgitario self-requested a review February 15, 2023 05:51
Sgitario
Sgitario requested changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

Sorry, I approved it too soon without realising that #30566 (review) was not addressed yet.

@quarkus-bot

This comment has been minimized.

Sign in to view
@iocanel
Copy link
Contributor Author

iocanel commented Feb 15, 2023

Sorry, I approved it too soon without realising that #30566 (review) was not addressed yet.

You didn't approve the PR too soon. I did drop the commit completely, but I think that I accidentially pulled it back.

@Sgitario
Copy link
Contributor

Sorry, I approved it too soon without realising that #30566 (review) was not addressed yet.

You didn't approve the PR too soon. I did drop the commit completely, but I think that I accidentially pulled it back.

The change in RemoveDeploymentTriggerDecorator.java still remains.

iocanel and others added 2 commits February 15, 2023 14:34
@iocanel
fix: workaround decorator ordering
2dfbe8b
@iocanel @cescoffier
Apply suggestions from code review
02f4345 
Co-authored-by: Clement Escoffier <clement.escoffier@gmail.com>
@iocanel
Copy link
Contributor Author

iocanel commented Feb 15, 2023

Sorry, I approved it too soon without realising that #30566 (review) was not addressed yet.

You didn't approve the PR too soon. I did drop the commit completely, but I think that I accidentially pulled it back.

The change in RemoveDeploymentTriggerDecorator.java still remains.

It should be ok now.

Sgitario
Sgitario approved these changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

lgtm!

@cescoffier cescoffier added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Feb 15, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 15, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@cescoffier cescoffier merged commit 3216065 into quarkusio:main Feb 15, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 15, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Feb 15, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cescoffier cescoffier cescoffier approved these changes

@Sgitario Sgitario Sgitario approved these changes

@geoand geoand Awaiting requested review from geoand

Assignees
No one assigned
Labels
area/health area/kubernetes area/metrics kind/enhancement New feature or request
Projects
None yet
Milestone
3.0.0.Alpha5
Development

Successfully merging this pull request may close these issues.

Allow configuring the port for the http probe
5 participants
@iocanel @geoand @cescoffier @Sgitario @gsmet