Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add readiness health check support in client #1213

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

Yavor16
Copy link

@Yavor16 Yavor16 commented Feb 7, 2024

No description provided.

Copy link

linux-foundation-easycla bot commented Feb 7, 2024

CLA Missing ID CLA Not Signed

@Yavor16 Yavor16 force-pushed the add-readiness-healthcheck-back-2 branch from ab94811 to 833a8a1 Compare February 7, 2024 07:16
@Yavor16
Copy link
Author

Yavor16 commented Feb 7, 2024

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

@pivotal-david-osullivan
Copy link
Contributor

@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.

@Yavor16
Copy link
Author

Yavor16 commented Feb 16, 2024

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
Copy link
Author

Yavor16 commented Feb 20, 2024

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

@anthonydahanne
Copy link
Contributor

anthonydahanne commented Feb 29, 2024

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
Copy link
Contributor

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

pacphi and others added 20 commits March 6, 2024 10:12
* make sure that we also compile the integration tests during Github CI
* when the extension has a mandatory field to initialize, because ExtendWith only works with default constructor
* add the extension registration declaratively (using an annotation)
adding create security group impl and tests

adding get security group api and tests

adding get security group impl + test

adding list security groups impl + api

adding update security group api

adding update security group api v3 impl

adding security groups delete api v3 impl and tests

adding bind running/staging security group v3 api

adding bind impl

adding unbind security group

adding list running/staging api and tests

adding create security group test

adding integration tests

adding integration tests

fixing setup/teardown

fixing integration tests

refactoring integration tests
* tests from radoslav commits
* cause they have interesting things to display
* on top of Hans Schulz  "mask off non-permission bits"
porting delete orphaned routes

pushing delete route

cleaning up leftovers in tests

applying default style and fixing missing imports
@Yavor16
Copy link
Author

Yavor16 commented Mar 6, 2024

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
Copy link
Author

Yavor16 commented Mar 22, 2024

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

@pivotal-david-osullivan pivotal-david-osullivan force-pushed the add-readiness-healthcheck-back-2 branch 2 times, most recently from a07bdfd to c748a4d Compare April 2, 2024 14:59
@anthonydahanne
Copy link
Contributor

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!

@Yavor16
Copy link
Author

Yavor16 commented Apr 4, 2024

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.

@beyhan
Copy link
Member

beyhan commented Jun 6, 2024

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?

@anthonydahanne
Copy link
Contributor

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

@boyan-velinov
Copy link

@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
Copy link

Hello @anthonydahanne
I saw some activity around the PR and also a commit related to API v3: 0e3d9ab
Would you mind to share the plans for accepting readiness healthcheck and the overall strategy for integration tests against newer CAPI?

@pivotal-david-osullivan
Copy link
Contributor

The commit referenced adds some support for fields in newer CAPI versions. Integration tests are completing successfully for readiness health check on older supported CAPI, we are almost done with tests for the newer version. Once they are passing I plan to add a commit to this PR with some further additions and then we can merge it.

@Yavor16 Yavor16 force-pushed the add-readiness-healthcheck-back-2 branch from 60e1dc1 to 2845c36 Compare December 5, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants