search

cloudfoundry / java-buildpack

Cloud Foundry buildpack for running Java applications
Apache License 2.0
435 stars 2.58k forks source link

Re-add Takipi Agent #998

Closed M-Tsur closed 1 year ago

linux-foundation-easycla[bot] commented 1 year ago

CLA Signed

The committers listed above are authorized under a signed CLA.

M-Tsur commented 1 year ago

@dmikusa Could you please direct me as to how I can test the change properly? Unfortunately, I haven't used cloud foundry in the past and the people that did are no longer with the company, and I have understood that things have changed from which services I can use to test it. Is there a way to test this locally or do I need a provider like AWS/GCP/etc?

dmikusa commented 1 year ago

Hi @M-Tsur - there should still be unit tests. We just disabled the framework for the agent previously, we didn't remove any code, so if you can run the unit tests successfully then you're probably in pretty good shape.

Doing an end-to-end test would require access to a CF environment. The easiest way is probably to create a user-provided service. You can see the credential requirements for that UPS here, https://github.com/cloudfoundry/java-buildpack/blob/main/docs/framework-takipi_agent.md#user-provided-service. The service name is going to need to contain takipi.

With that service created & bound to an app, when you restage the buildpack you should see it run and contribute the agent. If you provide legitimate service credentials, the app should start and talk to the service as well. I do not often have legitimate service credentials, so I just confirm that the agent is contributed, added to the start command, and if possible that I can see it attempting to start when the app starts.

Hope that helps!

M-Tsur commented 1 year ago

@dmikusa Great, thank you. I will try that 🙏

M-Tsur commented 1 year ago

I have ran the unit tests and was able to confirm that everything passes. I also tested this against a cloud foundry provider and it also worked by installing the takipi agent and connecting to our system.

@dmikusa Cloud you or someone on the team review this PR please?

dmikusa commented 1 year ago

@pivotal-david-osullivan FYI ^^

pivotal-david-osullivan commented 1 year ago

I tried to test this integration @M-Tsur and the agent downloads as expected, however when the app starts I see the following error:

ERR OverOps does not support Class Data Sharing and Type Speculation. Please run with both flags: -XX:-UseTypeSpeculation -Xshare:off

These are referenced in the docs as required for Java 9+, we should probably handle this so that the user does not have to set these for every application?

I added these flags to progress further and then the agent logs this error:

ERR Error creating a new folder on path /home/vcap/app/.java-buildpack/takipi_agent/log/agents
ERR File not found

Should the agent be configured to log to stdout rather than a container file maybe?

M-Tsur commented 1 year ago

@pivotal-david-osullivan Thank you for testing this out. In regards to the first issue, the reason that we don't add those flags automatically is because they are JVM arguments that might not be allowed by all users, so we leave it to the user to add them knowingly. We can update the resources in the repo around what is required, or we can do it automatically if you think this would fit more with the expected behavior when using cloud foundry.

In regards to the second issue, the agent actually logs to a container file, or passes the logs to the collector machine (which is another required component of OverOps/Takipi). Unfortunately, you got the 2 errors which failed during initialization, so we don't have the log file at this moment, thus we log to stdout to prevent the user from missing this information.

If you think we should update any of those in regards to the cloud foundry expected experience, please let me know.

M-Tsur commented 1 year ago

Hi @pivotal-david-osullivan - any suggestions regarding the above ^^ ?

M-Tsur commented 1 year ago

@dmikusa @pivotal-david-osullivan sorry to nag, but do you have any update on this?

pivotal-david-osullivan commented 1 year ago

Hi @M-Tsur, the log file issue seems fine in that case 👍

I do think it would be better to add the required arguments for the Java 9+ cases, since the agent won't start successfully without it. It should mean that the agent works well in the majority of 'default' cases.

You could mention the arg additions in the doc page. Let me know what you think?

M-Tsur commented 1 year ago

I will work on adding the flags in the java 9+ cases and also update the docs to indicate it adds those flags. I will convert this to draft and let you know once the PR is ready for review.

M-Tsur commented 1 year ago

@pivotal-david-osullivan I have finished working on this - I have added the flags in case Java 9+ is being used, added a note about this behavior in the Takipi Agent docs and also fixed the issue with the logs in case the folder doesn't exist.

I also tested everything and it seems to work fine.

Please let me know if you think I should add anything else or if I need to adjust anything in the code.

pivotal-david-osullivan commented 1 year ago

That looks good @M-Tsur and tested fine, I made a small comment about the command example since it is removed from the CF CLI. I will merge if you can make this change or I can make it afterwards.

Thank you for the contribution!

M-Tsur commented 1 year ago

@pivotal-david-osullivan I changed the doc. Thank you!