Enhance HealthScope #160
Enhance HealthScope #160
Conversation
Signed-off-by: roy wang <seiwy2010@gmail.com>
| // why tc.hsWorkloadRefs passed into It func | ||
| // shared the same address? | ||
| // for _, tc := range tests { | ||
| // |
There was a problem hiding this comment.
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.
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 <seiwy2010@gmail.com>
1284a94
to
fe48c6d
Compare
|
ping @artursouza @ryanzhang-oss , could you help review this PR, thanks! |
There was a problem hiding this comment.
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? |
| } | ||
| var ( | ||
| // for general check on worload's replicas | ||
| generalReplicaCheckFiled = []string{ |
There was a problem hiding this comment.
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.
just one replica exists doesn't mean it's healthy...
There was a problem hiding this comment.
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 <seiwy2010@gmail.com>
Signed-off-by: roy wang <seiwy2010@gmail.com>
|
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.
I may be missing something here, why is that "ReadyReplicas != 0" means healthy?
There was a problem hiding this comment.
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.
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.
same here, why is that "ReadyReplicas != 0" means healthy?
#68
status.availableReplicasandstatus.readyReplicas