enhance health scope with informative health condition #194

captainroy-hy · 2020-09-03T12:09:00Z

Make health scope show more information in HealthCondition.

// HealthCondition represents informative health condition.
type HealthCondition struct {
	// ComponentName represents the component name if target is a workload
	ComponentName  string                         `json:"componentName,omitempty"`
	TargetWorkload runtimev1alpha1.TypedReference `json:"targetWorkload,omitempty"`
	HealthStatus   HealthStatus                   `json:"healthStatus"`
	Diagnosis      string                         `json:"diagnosis,omitempty"`
	// WorkloadStatus represents status of workloads whose HealthStatus is UNKNOWN.
	WorkloadStatus string `json:"workloadStatus,omitempty"`
}

Add CheckByHealthCheckTrait interface
Modify e2e-test & unit tests

add unit test Signed-off-by: roy wang <seiwy2010@gmail.com>

Signed-off-by: roy wang <seiwy2010@gmail.com>

Add CheckByHealthCheckTrait interface Signed-off-by: roy wang <seiwy2010@gmail.com>

Signed-off-by: roy wang <seiwy2010@gmail.com>

wonderflow

Good job

wonderflow · 2020-09-03T13:20:48Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go

-	r.Target.UID = cwObj.GetUID()
+	compName, exist := cwObj.GetLabels()[oam.LabelAppComponent]
+	if !exist {
+		compName = "UNKNOWN NAME"


keep it empty is better if there's not label found

yes, I will fix it.

wonderflow · 2020-09-03T14:18:37Z

test/e2e-test/health_scope_test.go

 				// TODO(artursouza): enable this check once crossplane is updated.
 				//if len(healthScope.Spec.WorkloadReferences) == 0 {
 				//	return false
 				//}

-				return healthScope.Status.Health == "healthy"


why don't keep the compatibility here?

wonderflow · 2020-09-03T14:20:26Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope_controller_test.go

@@ -253,21 +249,6 @@ var _ = Describe("Test GetScopeHealthStatus", func() {
 					return nil
 				},
 			},
-			{
-				caseName:       "2 general workloads but one is unhealthy",


why delete this case？

because the general check logic (check any workloads having status.readyReplicas) is removed, so this case doesn't make sense anymore.

wonderflow · 2020-09-03T14:23:18Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope_controller.go

 	errUpdateHealthScopeStatus = "cannot update health scope status"
 )

+const (
+	infoFmtScopeDiagnosis = "total workloads: %d, healthy workloads: %d, unhealthy workloads: %d, unknown workloads: %d"


I suggest we can also provide a structural way here, makes it easy for other traits re-use this info.

wonderflow · 2020-09-03T14:27:04Z

apis/core/v1alpha2/core_scope_types.go

@@ -41,7 +53,22 @@ type HealthScopeSpec struct {
 type HealthScopeStatus struct {
 	runtimev1alpha1.ConditionedStatus `json:",inline"`

-	Health string `json:"health"`
+	// ScopeHealthCondition represents health condition summary of the scope
+	ScopeHealthCondition HealthCondition `json:"scopeHealthCondition"`


they should be different struct as some of the field like componentName don't work here.

yes, let me add a specific struct for ScopeHealthCondition, we can replace
infoFmtScopeDiagnosis = "total workloads: %d, healthy workloads: %d, unhealthy workloads: %d, unknown workloads: %d" with the new struct.

@wonderflow any suggestion on the scope health condition struct? e.g. additional info that may be consumed by traits.

// ScopeHealthCondition represents health condition summary of a scope. type ScopeHealthCondition struct { HealthStatus HealthStatus `json:"healthStatus"` Total int64 `json:"total,omitempty"` HealthyWorkloads int64 `json:"healthyWorkloads,omitempty"` UnhealthyWorkloads int64 `json:"unhealthyWorkloads,omitempty"` UnknownWorkloads int64 `json:"unknownWorkloads,omitempty"` }

The struct is good

wonderflow · 2020-09-03T14:31:54Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go

+	compName, exist := deployment.GetLabels()[oam.LabelAppComponent]
+	if !exist {
+		compName = "UNKNOWN NAME"
+	}
+	r.ComponentName = compName


I see this code twice, can we make it a common function, just put the runtime.Object as argument

wonderflow · 2020-09-03T14:32:55Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go

+	compName, exist := statefulset.GetLabels()[oam.LabelAppComponent]
+	if !exist {
+		compName = "UNKNOWN NAME"
+	}
+	r.ComponentName = compName


wonderflow · 2020-09-03T14:33:08Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go

+	compName, exist := daemonset.GetLabels()[oam.LabelAppComponent]
+	if !exist {
+		compName = "UNKNOWN NAME"
+	}


wonderflow · 2020-09-03T14:35:12Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go

+	wl := &unstructured.Unstructured{}
+	wl.SetGroupVersionKind(wlRef.GroupVersionKind())
+	if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: wlRef.Name}, wl); err != nil {
+		healthCondition.Diagnosis += errors.Wrap(err, errHealthCheck).Error()


We should not combine string, I don't see we have more than one error message to combine?

Yes, I will fix it.

wonderflow · 2020-09-03T14:36:49Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope_controller.go

-			var wlStatusC *HealthCondition
+			var wlHealthCondition *HealthCondition
+
+			wlHealthCondition = r.traitChecker.Check(ctx, r.client, resRef, healthScope.GetNamespace())


why traitChecker has higher priority than built-in ones？

I think it's possible for user to override health check logic on these "built-in" workloads.

modify e2e-test & unit test Signed-off-by: roy wang <seiwy2010@gmail.com>

wonderflow · 2020-09-04T06:49:04Z

ping @artursouza this PR may contain some un-compatible changes, please confirm if it's OK? Does healthscope already be in use in any cases of MSFT?

artursouza · 2020-09-04T21:50:03Z

It is OK to break compatibility. Let me look at the PR now.

artursouza

This is great!

artursouza · 2020-09-04T21:52:59Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go

-	if deployment.Status.ReadyReplicas == 0 {
-		r.Diagnosis = fmt.Sprintf(errFmtResourceNotReady, deployment.Status)
+	// Health criteria
+	if deployment.Status.ReadyReplicas != requiredReplicas {


What if there are more ReadyReplicas?

Maybe we can treat this situation as "unhealthy" just like this case "readyReplicas greater than replicas" ?

Signed-off-by: roy wang <seiwy2010@gmail.com>

captainroy-hy · 2020-09-06T10:30:22Z

hi @wonderflow @ryanzhang-oss May I trouble you to help review this pr? thx!

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go

wonderflow · 2020-09-07T02:42:15Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go

+}
+
+func getComponentNameFromLabel(o metav1.Object) string {
+	compName, exist := o.GetLabels()[oam.LabelAppComponent]


Will we have risk of panic here? if o.GetLabels return nil

nil of map type would not lead a panic but nil of metav1.Object interface would. I should add nil check for o metav1.Object

wonderflow · 2020-09-07T02:43:24Z

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go

+}
+
+// CheckUnknownWorkload handles unknown type workloads.
+func CheckUnknownWorkload(ctx context.Context, c client.Client, wlRef runtimev1alpha1.TypedReference, ns string) *WorkloadHealthCondition {


I didn't see any test case for this function?

It's tested here. Let me separate it into healthscope_test.go

Signed-off-by: roy wang <seiwy2010@gmail.com>

wonderflow

LGTM

Signed-off-by: roy wang <seiwy2010@gmail.com>

captainroy-hy added 3 commits September 3, 2020 20:54

use merging instead of overriding

645a967

add unit test Signed-off-by: roy wang <seiwy2010@gmail.com>

move AddLabel func into oam/helper

2707bbc

Signed-off-by: roy wang <seiwy2010@gmail.com>

Make health condition of HealthScope more informative

528ec10

Add CheckByHealthCheckTrait interface Signed-off-by: roy wang <seiwy2010@gmail.com>

captainroy-hy force-pushed the informative-health-scope branch from f0851b3 to 528ec10 Compare September 3, 2020 12:27

fix e2e-test

9925757

Signed-off-by: roy wang <seiwy2010@gmail.com>

wonderflow reviewed Sep 3, 2020

View reviewed changes

modify health scope condition

8a5852a

modify e2e-test & unit test Signed-off-by: roy wang <seiwy2010@gmail.com>

artursouza reviewed Sep 4, 2020

View reviewed changes

fix typo

8b62ad5

Signed-off-by: roy wang <seiwy2010@gmail.com>

captainroy-hy force-pushed the informative-health-scope branch from 8e7c793 to 8b62ad5 Compare September 5, 2020 03:00

wonderflow reviewed Sep 7, 2020

View reviewed changes

pkg/controller/v1alpha2/core/scopes/healthscope/healthscope.go Show resolved Hide resolved

wonderflow reviewed Sep 7, 2020

View reviewed changes

add unit test for health scope

6ad4984

Signed-off-by: roy wang <seiwy2010@gmail.com>

captainroy-hy force-pushed the informative-health-scope branch from ac0a7b1 to 6ad4984 Compare September 7, 2020 07:09

wonderflow approved these changes Sep 7, 2020

View reviewed changes

captainroy-hy mentioned this pull request Sep 8, 2020

[BUG] refine output for vela app:status kubevela/kubevela#150

Closed

resolve conflicts on healthscope_controller

8986d28

Signed-off-by: roy wang <seiwy2010@gmail.com>

wonderflow merged commit 78b84f2 into crossplane:master Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhance health scope with informative health condition #194

enhance health scope with informative health condition #194

enhance health scope with informative health condition #194

enhance health scope with informative health condition #194

Conversation

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

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

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

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