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

[AMORO-1957] Support liveness port testing for standby instance #2134

Closed
wants to merge 1 commit into from

Conversation

huyuanfeng2018
Copy link
Contributor

@huyuanfeng2018 huyuanfeng2018 commented Oct 17, 2023

Why are the changes needed?

Close ##1957

Brief change log

  • Start a thread to open the port for k8s when the container starts k8s use for livenessprobe

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

In my environment it works fine.

image

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not documented)

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...om/netease/arctic/server/ArcticManagementConf.java 99.51% <100.00%> (+0.01%) ⬆️
.../netease/arctic/server/ArcticServiceContainer.java 68.11% <0.00%> (-5.51%) ⬇️

... and 4 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

Copy link
Contributor

@baiyangtx baiyangtx 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 using a separate port as a liveness port is not a good solution. If the goal is to solve the Kubernetes pod liveness probe issue, using the "ps" command is also a viable option. This PR proposes reusing the HTTP port and obtaining the instance status through an HTTP API.

@wangtaohz
Copy link
Contributor

wangtaohz commented Oct 19, 2023

I think using a separate port as a liveness port is not a good solution. If the goal is to solve the Kubernetes pod liveness probe issue, using the "ps" command is also a viable option. This PR proposes reusing the HTTP port and obtaining the instance status through an HTTP API.

Do you think using the HTTP API mentioned here #2082 is a resonable solution? @baiyangtx

@huyuanfeng2018
Copy link
Contributor Author

I think using a separate port as a liveness port is not a good solution. If the goal is to solve the Kubernetes pod liveness probe issue, using the "ps" command is also a viable option. This PR proposes reusing the HTTP port and obtaining the instance status through an HTTP API.

Do you think using the HTTP API mentioned here #2082 is a resonable solution? @baiyangtx

Maybe take a look at this:
https://github.com/NetEase/amoro/blob/27bbc6991ce6d495a16527a083f45d89eb1a0268/ams/server/src/main/java/com/netease/arctic/server/dashboard/controller/HealthCheckController.java#L27

@baiyangtx
Copy link
Contributor

I think using a separate port as a liveness port is not a good solution. If the goal is to solve the Kubernetes pod liveness probe issue, using the "ps" command is also a viable option. This PR proposes reusing the HTTP port and obtaining the instance status through an HTTP API.

Do you think using the HTTP API mentioned here #2082 is a resonable solution? @baiyangtx

Can this interface be accessed on standby nodes? @wangtaohz

@wangtaohz
Copy link
Contributor

Can this interface be accessed on standby nodes? @wangtaohz

I don't think it can, just like any other HTTP API, only the leader AMS provides HTTP service.

@baiyangtx
Copy link
Contributor

Since this PR is not planed to proceed for now, can I close the PR temporarily? @huyuanfeng2018

@huyuanfeng2018
Copy link
Contributor Author

Since this PR is not planed to proceed for now, can I close the PR temporarily? @huyuanfeng2018

ok

@baiyangtx baiyangtx closed this Nov 6, 2023
@huyuanfeng2018 huyuanfeng2018 deleted the amoro_1957 branch November 19, 2023 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants