Skip to content

cluster, helm: Add new Helm-chart "splice-info" #332

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

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

Conversation

giner
Copy link
Contributor

@giner giner commented Apr 16, 2025

This helm charts runs a simple web-server to provide static deployment and dynamic runtime information as JSON files.

  • Static deployment configuration is published as JSON under /
  • Runtime part supports fetching DSO information from Scan and publishes it under /runtime/dso.json

@giner giner force-pushed the stas/cluster_helm_add_info branch 4 times, most recently from 672b440 to 1d1cf68 Compare April 16, 2025 11:12
@isegall-da
Copy link
Contributor

Thanks @giner . I haven't yet looked at it closely, but my main concern is that there's no tests for it.
Unfortunately that's still a bit hard to do in Splice directly, as there's no CI here (coming soon...), so I see two options: either we park this for a while, until we have enough testing infra in Splice for you to add tests, or we have someone from DA add tests in the internal repo. WDYT?

At a high level, we need to: deploy this in Pulumi (the Pulumi code is not yet in Splice though 😢 coming soon...), and add some logic to one of the preflight tests in apps\app

@giner
Copy link
Contributor Author

giner commented Apr 16, 2025

Whatever gets it merged (or rejected, also works for me :)) faster. If DA folks could add tests let's do that.

@isegall-da
Copy link
Contributor

Definitely aiming at merge, not reject. Let me see if someone can carve out a bit of time for this then.

@giner giner force-pushed the stas/cluster_helm_add_info branch from 1d1cf68 to f90ad05 Compare April 16, 2025 14:12
@ray-roestenburg-da
Copy link
Contributor

@giner CI is now open, would you like to add some tests and complete this?

@giner
Copy link
Contributor Author

giner commented Jun 12, 2025

What sort of test would be useful here?

@nicu-da
Copy link
Contributor

nicu-da commented Jun 13, 2025

@giner There's a few things missing to complete this:

  1. We could use a helm test for the generated config map at least, to make sure all the values are treated correctly. You can check https://github.com/hyperledger-labs/splice/tree/main/cluster/helm/splice-participant/tests for some helm tests examples
  2. We need to also add this to the deployment to include it as part of the DA deployments but also to be able to test the deployment of the helm chart. This should be fairly similar to the docs deployment. You can see that one here https://github.com/hyperledger-labs/splice/blob/main/cluster/pulumi/canton-network/src/docs.ts. In the same pulumi project we need to add a new spliceInfo deployment. Note that the docs is installed globally while the new chart should be installed as part of an individual SV here
    export async function installSvNode(
  3. We need to test that the new chart API is accessible after the deployment. It should be part of https://github.com/hyperledger-labs/splice/blob/a91c38d296d8742561a4f4c4c25756bebae8608b/apps/app/src/test/scala/org/lfdecentralizedtrust/splice/integration/tests/runbook/DsoPreflightIntegrationTest.scala and we just need to add a new test scenario similar to the docs one that validates the new url. For the docs one we load the UI, in the new test probably we want to do a simple http call and check the response. You can check the way we build the http client and do a http request in the cometbft client

Just let me know if you need any help around this.

@giner
Copy link
Contributor Author

giner commented Jun 13, 2025

@nicu-da , thank you. I'll check p1. As for the rest, wouldn't it make sense to do that separately? This PR is only to add the helm chart and make ot available, not making it part of the deployment.

@nicu-da
Copy link
Contributor

nicu-da commented Jun 13, 2025

@nicu-da , thank you. I'll check p1. As for the rest, wouldn't it make sense to do that separately? This PR is only to add the helm chart and make ot available, not making it part of the deployment.

@giner We would need the rest of the points as well to test that the helm chart is deployable as part of CI.

@giner
Copy link
Contributor Author

giner commented Jun 16, 2025

@nicu-da, For p1 (helm tests) I'm having hard time coming up with something that would justify the need of maintaining these tests. What sort of tests do you think would be valuable?

This helm charts runs a simple web-server to provide static deployment
and dynamic runtime information as JSON files.

- Static deployment configuration is published as JSON under /
- Runtime part supports fetching DSO information from Scan and publishes
  it under /runtime/dso.json

Signed-off-by: Stanislav German-Evtushenko <[email protected]>
@giner giner force-pushed the stas/cluster_helm_add_info branch from 430a814 to 5327dd2 Compare June 16, 2025 02:16
- Nginx requires very little resources so we can reduce "requests"
  Before: 0.25 CPU x 256 MiB RAM
  After: 0 CPU x 32 MiB RAM

Signed-off-by: Stanislav German-Evtushenko <[email protected]>
@giner giner force-pushed the stas/cluster_helm_add_info branch from 5327dd2 to 6a72f95 Compare June 16, 2025 09:10
@nicu-da
Copy link
Contributor

nicu-da commented Jun 16, 2025

@nicu-da, For p1 (helm tests) I'm having hard time coming up with something that would justify the need of maintaining these tests. What sort of tests do you think would be valuable?

My thoughts were only around the generated config map in cluster/helm/splice-info/templates/configmap-content-static.yaml as we have a lot of templating there and it's easy to mess out the output yaml

@giner
Copy link
Contributor Author

giner commented Jun 16, 2025

My thoughts were only around the generated config map in cluster/helm/splice-info/templates/configmap-content-static.yaml as we have a lot of templating there and it's easy to mess out the output yaml

Noted, will do.

As for p2 and p3. I have more or less idea of the changes I need to make however I'm not certain on how to iteratively test them. The deployment doesn't seem to be suitable for local deployment (it looks like it's not cloud agnostic, i.e. I won't be able to run it on my local k8s, and it may also require some external dependencies that I don't have) so I can't run it locally. Pushing through CI is likely to be slow because the deploy process takes time and requires an approval for each change. Any suggestions?

@nicu-da
Copy link
Contributor

nicu-da commented Jun 16, 2025

Yes unfortunately that's the current state. We will try to improve this in the future.
For now the first step is to make sure that the run expected works and that the change is sane.
Afterwards you can trigger a ci cluster deployment and if there's issues with that I can help out investigate and run locally to reduce the feedback loop.

- Add unit tests for templating configmap-content-static_test.yaml

Signed-off-by: Stanislav German-Evtushenko <[email protected]>
This change is still WIP, will be updated soon
@giner giner force-pushed the stas/cluster_helm_add_info branch from bb87d73 to 8cd2c4b Compare June 18, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants