-
Notifications
You must be signed in to change notification settings - Fork 80
Enhance HealthScope #160
Enhance HealthScope #160
Conversation
Signed-off-by: roy wang <[email protected]>
// why tc.hsWorkloadRefs passed into It func | ||
// shared the same address? | ||
// for _, tc := range tests { | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tc
is a local variable in loop, and the address behind it is an iterator. So when the loop iterated, the value of iterator which is tc
will change while the address keep the same. Generally, add such line in loop will fix:
tc := tc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I used tc:=tc
originally but it didn't work because I put tc:=tc
inside It("",func(){tc:=tc ...})
. It should put it before It
otherwise the scope of It
will always get the same address of tc
. Fixed. Thx!
Signed-off-by: roy wang <[email protected]>
1284a94
to
fe48c6d
Compare
ping @artursouza @ryanzhang-oss , could you help review this PR, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for the known K8s resources but this is not extensible. We had discussed about having a HealthCheckTrait to decouple health check from health scope. HealthScope is the aggregation of health statuses while the health status itself is the output of one kind of health check.
pkg/controller/v1alpha2/core/scopes/healthscope/healthscope_controller.go
Outdated
Show resolved
Hide resolved
BTW, I am not against this PR but this cannot be extended to future workloads. |
IsHealthy: true, | ||
} | ||
} | ||
//TODO(roywang) does every workload have status? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really
} | ||
var ( | ||
// for general check on worload's replicas | ||
generalReplicaCheckFiled = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not all resources have these two fields
for _, fp := range generalReplicaCheckFiled { | ||
if value, err := pavedV.GetNumber(fp); err == nil { | ||
// just check ready/available replica exists | ||
if value != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one replica exists doesn't mean it's healthy...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems such kind of general checking rule doesn't make sense... Maybe I should remove generalReplicaCheck
and leave it to HealthCheckTrait
to satisfy diverse health checking needs.
@artursouza Yes, I agree that implementation in this PR is not extensible, and |
@captainroy-hy Yes, I think this PR is good, and we could support |
fix typo & modify unit test Signed-off-by: roy wang <[email protected]>
Signed-off-by: roy wang <[email protected]>
Hi @ryanzhang-oss I remove general health check and fix hardcode kind name issue. Could you please help review this PR, thanks! |
} | ||
r.Target.UID = deployment.GetUID() | ||
|
||
if deployment.Status.ReadyReplicas == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be missing something here, why is that "ReadyReplicas != 0" means healthy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just stay in line with current logic on health checking .And a more rigorous check is x.Status.ReadyReplicas == x.Spec.Replicas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could fix it in the following PRs
} | ||
r.Target.UID = statefulset.GetUID() | ||
|
||
if statefulset.Status.ReadyReplicas == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, why is that "ReadyReplicas != 0" means healthy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is good, let me merge it, and fix other things in the following PRs.
#68
status.availableReplicas
andstatus.readyReplicas