Determine app health status for plugin architecture #5454

ffjlabo · 2024-12-26T06:02:32Z

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.

pipecd/pkg/model/application_live_state.go

Lines 62 to 74 in 6734011

    
           // 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):

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo · 2024-12-26T06:03:29Z

pkg/model/application_live_state.proto

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


We decided to use UNHEALTHY instead of OTHER in pipedv1

codecov · 2024-12-26T06:07:14Z

Codecov Report

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

Project coverage is 26.14%. Comparing base (f6a472e) to head (a236221).
Report is 3 commits behind head on master.

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.14%   +0.05%     
==========================================
  Files         457      457              
  Lines       49081    49107      +26     
==========================================
+ Hits        12808    12841      +33     
+ Misses      35251    35244       -7     
  Partials     1022     1022

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

ffjlabo · 2024-12-26T06:09:29Z

lint/web is failed, so will fix it.

t-kikuc · 2024-12-26T06:12:40Z

pkg/model/application_live_state.go

+	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
+		}
+	}


[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

}

}

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

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo · 2024-12-26T06:37:44Z

web/src/components/application-detail-page/application-detail/health-status-icon/index.tsx

+      case ApplicationLiveStateSnapshot.Status.UNHEALTHY:
+        return <OtherIcon className={classes.unhealthy} />;


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

This is a fix for lint/web.

t-kikuc

Thank you!!

pkg/model/application_live_state.go

khanhtc1202 · 2024-12-26T07:35:45Z

pkg/model/application_live_state.go

+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
+}


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 😄

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.

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

khanhtc1202 · 2024-12-26T08:01:52Z

pkg/model/application_live_state_test.go

+			name: "healthy: no resources",
+			snapshot: &ApplicationLiveStateSnapshot{
+				ApplicationLiveState: &ApplicationLiveState{},
+			},
+			want: ApplicationLiveStateSnapshot_HEALTHY,


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

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

Thanks for clarify, commented in https://github.com/pipe-cd/pipecd/pull/5454/files#r1898201564

Signed-off-by: Yoshiki Fujikane <[email protected]>

khanhtc1202 · 2024-12-27T02:47:57Z

pkg/model/application_live_state.go

+	if app == nil {
+		return
+	}


Suggested change

if app == nil {

return

}

if app == nil {

s.HealthStatus = ApplicationLiveStateSnapshot_UNKNOWN

return

}

We should trickly assign the UNKNOWN stage here. The current logic looks like if this live state is nill, then the state will be kept "as is", which is unclearly UNKNOWN since UNKOWN is 0 in the enum. We should make it clear to avoid misunderstanding.

FIxed at f3bbedd

khanhtc1202 · 2024-12-27T02:48:37Z

pkg/model/application_live_state.go

+		return
+	}
+
+	s.HealthStatus = ApplicationLiveStateSnapshot_HEALTHY


Move this to L169, to make reading scope smaller.

~~Fixed at 400e581~~

Fixed at a236221

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo · 2024-12-27T03:03:24Z

@khanhtc1202 I fixed them 🙏

Signed-off-by: Yoshiki Fujikane <[email protected]>

khanhtc1202

Thank you 👍

ffjlabo · 2024-12-27T03:16:27Z

@t-kikuc Sorry, plz re-approve 🙏

Determine app health status for plugin architecture

6734011

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo requested review from nghialv, khanhtc1202, kentakozuka, t-kikuc and Warashi as code owners December 26, 2024 06:02

github-actions bot added area/web area/go area/pipedv1 labels Dec 26, 2024

ffjlabo commented Dec 26, 2024

View reviewed changes

t-kikuc reviewed Dec 26, 2024

View reviewed changes

ffjlabo added 2 commits December 26, 2024 15:27

Determine UI for Unhealthy

8e16586

Signed-off-by: Yoshiki Fujikane <[email protected]>

Prioritize unhealthy over unknown

b723484

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo commented Dec 26, 2024

View reviewed changes

ffjlabo enabled auto-merge (squash) December 26, 2024 06:38

ffjlabo requested a review from t-kikuc December 26, 2024 06:38

t-kikuc previously approved these changes Dec 26, 2024

View reviewed changes

khanhtc1202 reviewed Dec 26, 2024

View reviewed changes

pkg/model/application_live_state.go Outdated Show resolved Hide resolved

khanhtc1202 reviewed Dec 26, 2024

View reviewed changes

pkg/model/application_live_state.go Show resolved Hide resolved

khanhtc1202 reviewed Dec 26, 2024

View reviewed changes

ffjlabo dismissed t-kikuc’s stale review via 07acfbc December 26, 2024 08:33

ffjlabo requested review from t-kikuc and khanhtc1202 December 26, 2024 08:35

Improve the way to prioritize

5aec9a7

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo force-pushed the determine-app-health-status-for-plugin-architecture branch from 07acfbc to 5aec9a7 Compare December 26, 2024 08:47

khanhtc1202 reviewed Dec 27, 2024

View reviewed changes

ffjlabo added 2 commits December 27, 2024 11:54

Set UNKNOWN intentioally

f3bbedd

Signed-off-by: Yoshiki Fujikane <[email protected]>

Move the location for seeting HEALTHY status

400e581

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo requested a review from khanhtc1202 December 27, 2024 03:02

Move to correct place

a236221

Signed-off-by: Yoshiki Fujikane <[email protected]>

khanhtc1202 approved these changes Dec 27, 2024

View reviewed changes

t-kikuc approved these changes Dec 27, 2024

View reviewed changes

ffjlabo merged commit ab37feb into master Dec 27, 2024
14 of 16 checks passed

ffjlabo deleted the determine-app-health-status-for-plugin-architecture branch December 27, 2024 03:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Determine app health status for plugin architecture #5454

Determine app health status for plugin architecture #5454

ffjlabo commented Dec 26, 2024

ffjlabo Dec 26, 2024

codecov bot commented Dec 26, 2024 •

edited

Loading

ffjlabo commented Dec 26, 2024

t-kikuc Dec 26, 2024

ffjlabo Dec 26, 2024

ffjlabo Dec 26, 2024

t-kikuc left a comment

khanhtc1202 Dec 26, 2024 •

edited

Loading

khanhtc1202 Dec 26, 2024 •

edited

Loading

ffjlabo Dec 26, 2024 •

edited

Loading

khanhtc1202 Dec 26, 2024

ffjlabo Dec 26, 2024

khanhtc1202 Dec 27, 2024

khanhtc1202 Dec 27, 2024

ffjlabo Dec 27, 2024

khanhtc1202 Dec 27, 2024

ffjlabo Dec 27, 2024 •

edited

Loading

ffjlabo commented Dec 27, 2024

khanhtc1202 left a comment

ffjlabo commented Dec 27, 2024

	// 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()
	}
	}

		case ApplicationLiveStateSnapshot.Status.UNHEALTHY:
		return <OtherIcon className={classes.unhealthy} />;

Determine app health status for plugin architecture #5454

Determine app health status for plugin architecture #5454

Conversation

ffjlabo commented Dec 26, 2024

Choose a reason for hiding this comment

codecov bot commented Dec 26, 2024 • edited Loading

Codecov Report

ffjlabo commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-kikuc left a comment

Choose a reason for hiding this comment

khanhtc1202 Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

khanhtc1202 Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

ffjlabo Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

ffjlabo commented Dec 27, 2024

khanhtc1202 left a comment

Choose a reason for hiding this comment

ffjlabo commented Dec 27, 2024

codecov bot commented Dec 26, 2024 •

edited

Loading

khanhtc1202 Dec 26, 2024 •

edited

Loading

khanhtc1202 Dec 26, 2024 •

edited

Loading

ffjlabo Dec 26, 2024 •

edited

Loading

ffjlabo Dec 27, 2024 •

edited

Loading