Fix remote harbor healthcheck functionality #21674
Conversation
The review contain two changes: The first is adding implementation for HealthCheck in src/pkg/reg/adapter/harbor/v2/adapter.go The second general issue, in which a change made in src/controller/registry/controller.go by removing the continue inside the loop, the continue prevent from updating regMgr.Update with the status change Signed-off-by: Gal Netanel <[email protected]>
gnetanel
force-pushed
the
fix_remote_harbor_healthcheck_functionality
branch
from
ee3d029 to
7bebf28
Compare
There was a problem hiding this comment.
Thanks for your contribution.
Left my suggestions below and Also commented my thoughts on this issue in #21673 (comment)
| @@ -232,7 +232,6 @@ func (c *controller) StartRegularHealthCheck(ctx context.Context, closing, done | |||
| isHealthy, err := c.IsHealthy(ctx, registry) | |||
| if err != nil { | |||
| log.Errorf("failed to check health of registry %d: %v", registry.ID, err) | |||
| continue | |||
There was a problem hiding this comment.
I would suggest not changing this. Since this not only depends on harbor-adapter but also other registry adapters
There was a problem hiding this comment.
Indeed,
The commit has two parts, a missing implementation for harbor and general issue which is the above.
If C.IsHealthy return error, code continue and the status of endpoint won't get updated, i.e: it would remain Healthy while endpoint is not accessible (I manage to cause this by changing remote hardware endpoint core deployment to 0 replica and also by disconnecting it from the network)
If the 'continue' will be removed, the code has the right logic to check isHealthy variable and update accordingly, , if the continue will remain, the Status of endpoint won't get updated, so i think this indeed apply to all registries and should be changed.
Comprehensive Summary of your change
The review contain two changes:
The first is adding implementation for HealthCheck in src/pkg/reg/adapter/harbor/v2/adapter.go The second general issue, in which a change made in src/controller/registry/controller.go by removing the continue inside the loop, the continue prevent from updating regMgr.Update with the status change
Issue being fixed
Fixes #21673
Please indicate you've done the following: