cf-java-client icon indicating copy to clipboard operation
cf-java-client copied to clipboard

Add readiness health check support in client

Open Yavor16 opened this issue 1 year ago • 14 comments

Yavor16 avatar Feb 07 '24 06:02 Yavor16

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: Yavor16 / name: UzYav (f0ce09a48a511bc0ff49f8ee35ea1298ecd6d1e9, 3bdb3041c070427a79542863e03f6b776ce703ce, 83932d60903b026fa2a51390119d2e8fa7b07b44)
  • :white_check_mark: login: pivotal-david-osullivan / name: David O'Sullivan (c748a4d45833f2f84165c65f49eacaa1dc2bd327)

Hi, @pivotal-david-osullivan, I saw that you have commited a lot of changes recently, so can you take a look at my PR.

Yavor16 avatar Feb 07 '24 11:02 Yavor16

@Yavor16 thank you for the contribution! We are working through PRs at the moment.

Could you please add integration test support to cover the new feature? Liveness health checks can be seen in the integration-test module in places such as here - test setting the type and here - setting the health check in an application push.

pivotal-david-osullivan avatar Feb 15 '24 12:02 pivotal-david-osullivan

Hi, @pivotal-david-osullivan, I can't write integration test and test it because I don't have my own cloudfoundry environment. The one I use for work, I don't have administrator access to it. If you want I can write a test but if you can run it.

Yavor16 avatar Feb 16 '24 13:02 Yavor16

Hi, @pivotal-david-osullivan, I wrote an integration test and tested it

Yavor16 avatar Feb 20 '24 09:02 Yavor16

hello @Yavor16 , sorry to ask you this late, but could you run mvn spotless:apply and commit / amend ?

It will fix the CI failure on Java 11 that runs the stylecheck

Thank you! worst case, we'll do it ourselves

anthonydahanne avatar Feb 29 '24 17:02 anthonydahanne

Hello @Yavor16 Unfortunately, after integration tests run, we had a few errors; such as:

[ERROR] org.cloudfoundry.client.v3.ProcessesTest.updateReadinessHealthCheckType
[ERROR]   Run 1: ProcessesTest.updateReadinessHealthCheckType expectation "expectNext(port)" failed (expected: onNext(port); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Cannot construct instance of `org.cloudfoundry.client.v3.applications.GetApplicationProcessResponse`, problem: Cannot build GetApplicationProcessResponse, some of required attributes are not set [readinessHealthCheck]

or

[ERROR] org.cloudfoundry.client.v3.ProcessesTest.update
[ERROR]   Run 1: ProcessesTest.update expectation "expectNext(process)" failed (expected: onNext(process); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Cannot construct instance of `org.cloudfoundry.client.v3.applications.GetApplicationProcessResponse`, problem: Cannot build GetApplicationProcessResponse, some of required attributes are not set [readinessHealthCheck]
 at [Source: (byte[])"{"guid":"edeef781-4335-43ac-9343-39f7409759c6","created_at":"2024-03-01T04:57:36Z","updated_at":"2024-03-01T04:57:45Z","type":"web","command":"$HOME/boot.sh","instances":1,"memory_in_mb":64,"disk_in_mb":258,"health_check":{"type":"port","data":{"timeout":null,"invocation_timeout":null}},"relationships":{"app":{"data":{"guid":"edeef781-4335-43ac-9343-39f7409759c6"}},"revision":{"data":{"guid":"a7a89e0e-ac15-43c4-87eb-9e21d3c5a785"}}},"metadata":{"labels":{},"annotations":{}},"links":{"self":{"hre"[truncated 559 bytes]; line: 1, column: 1059]))

or

[ERROR] org.cloudfoundry.operations.ApplicationsTest.pushManifestV3
[ERROR]   Run 1: ApplicationsTest.pushManifestV3 expectation "expectComplete" failed (expected: onComplete(); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Cannot construct instance of `org.cloudfoundry.client.v3.processes.ProcessResource`, problem: Cannot build ProcessResource, some of required attributes are not set [readinessHealthCheck]
 at [Source: (byte[])"{"pagination":{"total_results":1,"total_pages":1,"first":{"href":"https://api.sys.polishedpine.cf-app.com/v3/apps/67ee72fe-8b93-40fc-80e4-7ce0f1c08e9b/processes?page=1\u0026per_page=50"},"last":{"href":"https://api.sys.polishedpine.cf-app.com/v3/apps/67ee72fe-8b93-40fc-80e4-7ce0f1c08e9b/processes?page=1\u0026per_page=50"},"next":null,"previous":null},"resources":[{"guid":"67ee72fe-8b93-40fc-80e4-7ce0f1c08e9b","created_at":"2024-03-01T05:20:56Z","updated_at":"2024-03-01T05:21:09Z","type":"web","c"[truncated 944 bytes]; line: 1, column: 1442] (through reference chain: org.cloudfoundry.client.v3.applications.ListApplicationProcessesResponse$Json["resources"]->java.util.ArrayList[0])))

or

[ERROR] org.cloudfoundry.operations.ApplicationsTest.pushManifestV3MultipleApplications
[ERROR]   Run 1: ApplicationsTest.pushManifestV3MultipleApplications expectation "expectComplete" failed (expected: onComplete(); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Cannot construct instance of `org.cloudfoundry.client.v3.processes.ProcessResource`, problem: Cannot build ProcessResource, some of required attributes are not set [readinessHealthCheck]
 at [Source: (byte[])"{"pagination":{"total_results":1,"total_pages":1,"first":{"href":"https://api.sys.polishedpine.cf-app.com/v3/apps/2a9c07ee-deb5-464e-a5f6-2875b0b30c45/processes?page=1\u0026per_page=50"},"last":{"href":"https://api.sys.polishedpine.cf-app.com/v3/apps/2a9c07ee-deb5-464e-a5f6-2875b0b30c45/processes?page=1\u0026per_page=50"},"next":null,"previous":null},"resources":[{"guid":"2a9c07ee-deb5-464e-a5f6-2875b0b30c45","created_at":"2024-03-01T05:11:27Z","updated_at":"2024-03-01T05:11:44Z","type":"web","c"[truncated 944 bytes]; line: 1, column: 1442] (through reference chain: org.cloudfoundry.client.v3.applications.ListApplicationProcessesResponse$Json["resources"]->java.util.ArrayList[0])))

Best guess is you missed a required JSON field named readinessHealthCheck

anthonydahanne avatar Mar 01 '24 14:03 anthonydahanne

Hello, @anthonydahanne, I've run the failing tests locally and all of them passed. Also I've run mvn spotless:apply but the file didn't change

Yavor16 avatar Mar 06 '24 09:03 Yavor16

Here is a gist with the my test results https://gist.github.com/Yavor16/2d6b9c4786242c2bf1eb3ebebe5383da

Yavor16 avatar Mar 22 '24 09:03 Yavor16

hello @Yavor16 We got bad news: after rebasing and re testing your PR, we found out that your PR is relying on a recent CAPI version; much more recent than the default we target with current java-cf-client ; which is 2.186.0

We could merge this PR after we require a later version than 2.186.0

Thanks!

anthonydahanne avatar Apr 04 '24 02:04 anthonydahanne

Hello, @anthonydahanne So can we add a new feature to the client when the cersion you are using doesn't support it. Also how long do we have to wait for a new CAPI version.

Yavor16 avatar Apr 04 '24 07:04 Yavor16

I know about users willing to use the readiness health check blocked by this pr. From the platform side we are also interested to get more feedback for new features. It will be great if the CF Java client could support more recent versions of CAPI which will help also for the adoption of those new features. @pivotal-david-osullivan, @anthonydahanne is there a chance we find a good middle ground here?

beyhan avatar Jun 06 '24 09:06 beyhan

@beyhan , yes, we want to unblock this situation. we'll update you soon

anthonydahanne avatar Jun 13 '24 20:06 anthonydahanne

@anthonydahanne Do you have some news on that? Did you establish a plan how to move the change forward? Users are eager to use readiness health check feature

boyan-velinov avatar Aug 01 '24 07:08 boyan-velinov