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

Determine app health status for plugin architecture #5454

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ffjlabo
Copy link
Member

@ffjlabo ffjlabo commented Dec 26, 2024

What this PR does:

Added a method to determine application health state.

Why we need it:

In the pipedv0, there is a similar method called DetermineAppHealthStatus, but I don't want to use it to avoid implementation logic based on the kind.

// DetermineAppHealthStatus updates its own health status, which is determined based on its resources status.
func (s *ApplicationLiveStateSnapshot) DetermineAppHealthStatus() {
switch s.Kind {
case ApplicationKind_KUBERNETES:
s.determineKubernetesAppHealthStatus()
case ApplicationKind_CLOUDRUN:
s.determineCloudRunAppHealthStatus()
case ApplicationKind_ECS:
s.determineECSAppHealthStatus()
case ApplicationKind_LAMBDA:
s.determineLambdaAppHealthStatus()
}
}

Which issue(s) this PR fixes:

Part of #5363

Does this PR introduce a user-facing change?:

  • How are users affected by this change:
  • Is this breaking change:
  • How to migrate (if breaking change):

@@ -29,6 +29,7 @@ message ApplicationLiveStateSnapshot {
UNKNOWN = 0;
HEALTHY = 1;
OTHER = 2;
UNHEALTHY = 3;
Copy link
Member Author

Choose a reason for hiding this comment

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

We decided to use UNHEALTHY instead of OTHER in pipedv1

Copy link

codecov bot commented Dec 26, 2024

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 26.13%. Comparing base (f6a472e) to head (5aec9a7).

Files with missing lines Patch % Lines
...app/pipedv1/livestatereporter/livestatereporter.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5454      +/-   ##
==========================================
+ Coverage   26.09%   26.13%   +0.03%     
==========================================
  Files         457      457              
  Lines       49081    49105      +24     
==========================================
+ Hits        12808    12833      +25     
+ Misses      35251    35250       -1     
  Partials     1022     1022              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ffjlabo
Copy link
Member Author

ffjlabo commented Dec 26, 2024

lint/web is failed, so will fix it.

Comment on lines 155 to 165
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}

if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}
Copy link
Member

Choose a reason for hiding this comment

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

[IMO] How about prioritizing UNHEALTHY?

In your code, the app's health status depends on resource order.
(e.g. If an Unknown resource is found earlier than an Unhealthy resource, then it's Unknown)

In the following code,
1. If any Unhealthy is found, then the app is Unhealthy.
2. If no Unhealthy was found and any Unknown is found, then the app is Unknown

Suggested change
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}
if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}
}
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@t-kikuc
Thanks, I agree with it.
Fixed at b723484

Signed-off-by: Yoshiki Fujikane <[email protected]>
Signed-off-by: Yoshiki Fujikane <[email protected]>
Comment on lines +37 to +38
case ApplicationLiveStateSnapshot.Status.UNHEALTHY:
return <OtherIcon className={classes.unhealthy} />;
Copy link
Member Author

Choose a reason for hiding this comment

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

Use OtherIcon similar to OTHER's one as the UNHEALTHY icon for now.

This is a fix for lint/web.

@ffjlabo ffjlabo enabled auto-merge (squash) December 26, 2024 06:38
@ffjlabo ffjlabo requested a review from t-kikuc December 26, 2024 06:38
t-kikuc
t-kikuc previously approved these changes Dec 26, 2024
Copy link
Member

@t-kikuc t-kikuc left a comment

Choose a reason for hiding this comment

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

Thank you!!

Comment on lines 149 to 170
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
return
}

for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}
}

for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}

s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
}
Copy link
Member

@khanhtc1202 khanhtc1202 Dec 26, 2024

Choose a reason for hiding this comment

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

Suggested change
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
return
}
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}
}
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}
s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
}
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
return
}
unhealthy := false
unknown := false
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
unhealthy = true
}
if r.HealthStatus == ResourceState_UNKNOWN {
unknown = true
}
}
s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
if unhealthy {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
}
if unknown {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
}
}

@ffjlabo @t-kikuc ok, let's me suggest a clear version of what you both trying to do 😄

Copy link
Member

@khanhtc1202 khanhtc1202 Dec 26, 2024

Choose a reason for hiding this comment

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

Suggested change
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
return
}
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}
}
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}
s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
}
func (s *ApplicationLiveStateSnapshot) DetermineApplicationHealthStatus() {
app := s.ApplicationLiveState
if app == nil {
return
}
s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNHEALTHY {
s.HealthStatus = ApplicationLiveStateSnapshot_UNHEALTHY
return
}
}
for _, r := range app.Resources {
if r.HealthStatus == ResourceState_UNKNOWN {
s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN
return
}
}
}

Or another version at least could be this to avoid miss set health status.

Copy link
Member Author

@ffjlabo ffjlabo Dec 26, 2024

Choose a reason for hiding this comment

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

Thanks both @khanhtc1202 @t-kikuc
I fixed it to reduce one for statement.
5aec9a7

Comment on lines +191 to +195
name: "healthy: no resources",
snapshot: &ApplicationLiveStateSnapshot{
ApplicationLiveState: &ApplicationLiveState{},
},
want: ApplicationLiveStateSnapshot_HEALTHY,
Copy link
Member

Choose a reason for hiding this comment

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

What happen if ApplicationLiveState = nil, not a dummy pointer?

Copy link
Member Author

Choose a reason for hiding this comment

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

@khanhtc1202
The case is ApplicationLiveStateSnapshot_UNKNOWN as the last of the testcases, similar to the current implementation.

Signed-off-by: Yoshiki Fujikane <[email protected]>
@ffjlabo ffjlabo force-pushed the determine-app-health-status-for-plugin-architecture branch from 07acfbc to 5aec9a7 Compare December 26, 2024 08:47
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