cloudfoundry / cli

The official command line client for Cloud Foundry
https://docs.cloudfoundry.org/cf-cli
Apache License 2.0
1.75k stars 927 forks source link

get-health-check reports 1 second invocation timeout when the default is used #2311

Open mymasse opened 2 years ago

mymasse commented 2 years ago

Please fill out the issue checklist below and provide ALL the requested information.

Describe the bug and the command you saw an issue with While debugging why an application was being restarted by CF continuously we looked into the get-health-check of the application. The invocation timeout was listed as 1 and we believe this is wrong.

What happened We do not set any healtcheck timeouts on our apps and rely on the default provided by CAPI which should be 60s default_health_check_timeout. Running the get-health-check actually returns 1s.

Expected behavior We expected the command to return the correct value.

Exact Steps To Reproduce Steps to reproduce the behavior; include the exact CLI commands and verbose output:

  1. cf push <APP> an application without specifying the healthcheck timeout.
  2. Run cf get-health-check <APP>

Provide more context

**Possible code bug This line in the CLI code when invocation timeout is set to 0 defaults the output of the command to 1, this seems like the problem: https://github.com/cloudfoundry/cli/blob/d3bb1ecfc94ee12432f8defb30ab9459e83cc727/command/v7/get_health_check_command.go#L65

Notes regarding V6 and V7 CLI support:

gururajsh commented 2 weeks ago

Hi @mymasse. CLI assigns the value 1 when a 0/null value is returned from cloud controller. Not sure why the default 60s is not being sent to CLI from CC. Please feel free to open an issue with CAPI.

mymasse commented 2 weeks ago

Even return 0/null would be better than returning 1, at least it would point out that it's not set. I agree that CAPI should probably return the default value but I still think the CLI should return something else than 1 in this case since the CLI still needs to support older CAPI release.

gururajsh commented 1 week ago

There are two timeouts for healthcheck. One is the default_health_check_timeout which has default of 60s as mentioned here. This is the time the health_check waits for the app to come up. The other one is health_check invocation_timeout which has a default of 1s as mentioned here. This is the time the health_check endpoint is polled to see if the app is responding or not until the defualt_time_out is elapsed. So for ex, health_check is performed for an app A every 1s upto 60s.

The reason CAPI is sending null is to differentiate if a value is set or not by the user. This has been the logic for over 6years. So supported older version of CAPI should be good.