Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

enhance health scope with informative health condition #194

Merged

Conversation

captainroy-hy
Copy link
Contributor

  • 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>
Copy link
Member

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

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

Good job

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I will fix it.

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

return healthScope.Status.Health == "healthy"
Copy link
Member

Choose a reason for hiding this comment

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

why don't keep the compatibility here?

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

Choose a reason for hiding this comment

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

why delete this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

errUpdateHealthScopeStatus = "cannot update health scope status"
)

const (
infoFmtScopeDiagnosis = "total workloads: %d, healthy workloads: %d, unhealthy workloads: %d, unknown workloads: %d"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it.

@@ -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"`
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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"`
}

Copy link
Member

Choose a reason for hiding this comment

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

The struct is good

Comment on lines 159 to 163
compName, exist := deployment.GetLabels()[oam.LabelAppComponent]
if !exist {
compName = "UNKNOWN NAME"
}
r.ComponentName = compName
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 198 to 202
compName, exist := statefulset.GetLabels()[oam.LabelAppComponent]
if !exist {
compName = "UNKNOWN NAME"
}
r.ComponentName = compName
Copy link
Member

Choose a reason for hiding this comment

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

again here

Comment on lines 235 to 238
compName, exist := daemonset.GetLabels()[oam.LabelAppComponent]
if !exist {
compName = "UNKNOWN NAME"
}
Copy link
Member

Choose a reason for hiding this comment

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

and here

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()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will fix it.

var wlStatusC *HealthCondition
var wlHealthCondition *HealthCondition

wlHealthCondition = r.traitChecker.Check(ctx, r.client, resRef, healthScope.GetNamespace())
Copy link
Member

Choose a reason for hiding this comment

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

why traitChecker has higher priority than built-in ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

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

Copy link
Member

@artursouza artursouza left a comment

Choose a reason for hiding this comment

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

This is great!

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

@artursouza artursouza Sep 4, 2020

Choose a reason for hiding this comment

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

What if there are more ReadyReplicas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

captainroy-hy commented Sep 6, 2020

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

}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: roy wang <seiwy2010@gmail.com>
@wonderflow wonderflow merged commit 78b84f2 into crossplane:master Sep 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants