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

Add fields for HealthCheck #902

Merged
merged 3 commits into from
Nov 10, 2020

Conversation

aswinsuryan
Copy link
Contributor

*HealthCheck Ip is added in EndpointSpec
*Latency is added in GatewayStatus

Fixes: #821
Signed-off-by: Aswin Surayanarayanan [email protected]

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr902/aswinsuryan/health-check-api

pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

sridhargaddam
sridhargaddam previously approved these changes Oct 28, 2020
@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

*HealthCheck Ip is added in EndpointSpec
*Latency is added in GatewayStatus

Fixes: submariner-io#821
Signed-off-by: Aswin Surayanarayanan <[email protected]>
@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

AverageRTT time.Duration `json:"averageRTT"`
MaxRTT time.Duration `json:"maxRTT"`
StdDevRTT time.Duration `json:"stddevRTT"`
LastRTT float64 `json:"lastRTT"`
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for changing these to float and ms units?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value in nano seconds seems to be not understandable, like

Latency:
Average RTT: 585160
Last RTT: 403876
Max RTT: 839461
Min RTT: 403876
Stddev RTT: 185170

So to make it similar to normal ping response changed it to float and ms unit

Latency:
Average RTT: 0.724
Last RTT: 0.635
Max RTT: 0.857
Min RTT: 0.635
Stddev RTT: 0.095

Copy link
Contributor

@tpantelis tpantelis Nov 6, 2020

Choose a reason for hiding this comment

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

OK - but it was already approved in the previous form so now need re-review.

@nyechiel seems reasonable - what do you think?

@tpantelis
Copy link
Contributor

The purpose of this PR is to add fields and it has had several review cycles already. I think the last commit to add implementation n should be in another PR.

@aswinsuryan
Copy link
Contributor Author

The purpose of this PR is to add fields and it has had several review cycles already. I think the last commit to add implementation n should be in another PR.

SInce I changed the API , I thought it can go together . I will revert to the original one.

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@tpantelis tpantelis self-requested a review November 6, 2020 15:29
@tpantelis tpantelis dismissed stale reviews from sridhargaddam and themself November 6, 2020 15:30

needs re-review

@tpantelis tpantelis requested a review from mangelajo November 6, 2020 15:31
@tpantelis tpantelis dismissed mangelajo’s stale review November 6, 2020 15:31

needs re-review

@tpantelis tpantelis requested a review from nyechiel November 6, 2020 15:31
@submariner-bot
Copy link
Contributor

🤖 Updated branch: z_pr902/aswinsuryan/health-check-api

@tpantelis tpantelis merged commit d637585 into submariner-io:master Nov 10, 2020
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr902/aswinsuryan/health-check-api]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants