Skip to content

Enable lnd pod logging #691

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

Merged
merged 7 commits into from
Mar 17, 2025
Merged

Conversation

macgyver13
Copy link
Contributor

@macgyver13 macgyver13 commented Mar 9, 2025

This update provides a new strategy for adding containers to a lnd pod via extraContainers. A reference implementation is integrated into test/data/ln and test/data/logging.

  • Add lnd-exporter metric capture to test/data/ln (activated by metricsExport: true)
  • Extend lnd pod template to support extraContainers
  • Removes broken reference to circuitBreaker

@willcl-ark
Copy link
Contributor

willcl-ark commented Mar 9, 2025

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@macgyver13
Copy link
Contributor Author

macgyver13 commented Mar 9, 2025

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #688 (Circuit breaker plugin by Camillarhi)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

#691 should proceed before #688 IMO

Copy link
Contributor

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good so far. Try to make as few changes as possible to test/data/ln since there are tests that expect that network to have specific properties. Ideally you'd just be able to add exporters to the existing LN nodes in that network and then add a test (either in ln_test.py or 'ln_basic_test.py) similar to the fork observer check in services_test.py

I'll try to allow-list you so CI runs automatically when you push. Be sure to run the tests locally as well!

macgyver13 added 2 commits March 10, 2025 20:39
- Reduced complexity of ln network.yaml config to align with ln_basic_test
@macgyver13 macgyver13 requested a review from pinheadmz March 12, 2025 13:08
Copy link
Contributor

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!!

Screenshot 2025-03-12 at 10 56 56 AM

imagePullPolicy: IfNotPresent
env:
- name: ADMIN_MACAROON_HEX
value: 0201036c6e6402f801030a1062beabbf2a614b112128afa0c0b4fdd61201301a160a0761646472657373120472656164120577726974651a130a04696e666f120472656164120577726974651a170a08696e766f69636573120472656164120577726974651a210a086d616361726f6f6e120867656e6572617465120472656164120577726974651a160a076d657373616765120472656164120577726974651a170a086f6666636861696e120472656164120577726974651a160a076f6e636861696e120472656164120577726974651a140a057065657273120472656164120577726974651a180a067369676e6572120867656e657261746512047265616400000620b17be53e367290871681055d0de15587f6d1cd47d1248fe2662ae27f62cfbdc6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for a follow up PR maybe we can figure out how to make this value (and the tls.cert) a global value for all helm charts in the system

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lnd-exporter assumes this value by default so it can technically be removed everywhere in warnet where lnd-exporter is referenced - just added for completeness.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hm. Probably not the best idea to maintain a constant value in two separate repos, in case it ever changes in warnet ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that is messy so lets go with your global helm chart approach, I know that is already implemented for chain (regtest)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand what the global helm approach would be but if it can be deleted then let's do that?

Copy link
Contributor

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK aa4d3b2

Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK aa4d3b20873fb48bd7a3325f0fc74c74540312aa
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmfRoZwACgkQ5+KYS2KJ
yTqnxg/+KN6pfvIT6qTB57z745+ndDBiljs6GeG2DDI8TuiskFnRwcZqsZUGsN9I
OnFBNx4NbPdvLKTQAJHLm/OpGVvn51Nj28EeRbv85hOP9CYg1UVS3dShkHahf3I3
2IHSdNnEQDjOsMsPIV11e6VY2AXr57SkhhdFy+XWq5XK5XSFvOdsoguNeg8SWjYn
yr7JC5Pdt8q4XBWZvvThLYGw/iRGdF6qnmEwYuvjD95Ik2d7XtCple8rcRMaCRkF
JrqlHCZCsaxfrViY5slpO92sxvgqGD/A0eGh5OJUZ0i+WXMhlNHUqA9f15tZL3Bs
c1UXplm7gQ40T+7SyBtYLf/xqpIfUwzW4MNFh3xf+B+HSjn+WqltQL4QY0pz4qyV
TaMCJ8pd1cwbgqIVgSgFAPPQR3YzkncdNfbm3IHRw74rlqkf+WdZIgp4X/wgA5Sv
hmpTYPZ0FHCrNilJMlGruXlPD1Fr2MXLDFyq06w8W8yG1fb2YVHaGDWil44ftsRs
Z4P7mWt+bafBgeNsisnaAlsV+htGUuLjlPyxD3GgMZBysmuQsTlBpJ27tzA6oC4f
qK5qTCSOVrkQGOIWHRhP5FmdqUOT0nT/tDSEEALpyq5R/oCP/CJJacG1aeLPbiA8
CH//oVf2G0tbbSRiuStwzk5CYF7XtFfBxlZcJBrHU5k8K0G+tR8=
=81wU
-----END PGP SIGNATURE-----

pinheadmz's public key is on openpgp.org

@m3dwards
Copy link
Collaborator

utACK aa4d3b2

macgyver13 added 2 commits March 13, 2025 09:13
-shared volume for containers in same pod
-lookup configmap based on label for separate pods
…puter.

pipeline artifacts did not provide any useful information
@pinheadmz
Copy link
Contributor

ACK this is great thanks!

I added #696 locally and will probably merge them together. Then I viewed the lnd dashboard after making a few changes to the test/data/ln network so there'd be something to look at:

diff --git a/resources/plugins/simln/charts/simln/templates/pod.yaml b/resources/plugins/simln/charts/simln/templates/pod.yaml
index 69790c9e..bda0344f 100644
--- a/resources/plugins/simln/charts/simln/templates/pod.yaml
+++ b/resources/plugins/simln/charts/simln/templates/pod.yaml
@@ -36,7 +36,7 @@ spec:
       args:
         - >
           cd /working;
-          sim-cli
+          sim-cli --capacity-multiplier=30
       volumeMounts:
         - name: {{ .Values.workingVolume.name }}
           mountPath: {{ .Values.workingVolume.mountPath }}
diff --git a/test/data/ln/network.yaml b/test/data/ln/network.yaml
index 5d30686d..bbf07636 100644
--- a/test/data/ln/network.yaml
+++ b/test/data/ln/network.yaml
@@ -19,8 +19,8 @@ nodes:
             block: 300
             index: 1
           target: tank-0004-ln
-          capacity: 100000
-          push_amt: 50000
+          capacity: 10000000
+          push_amt: 500000
   - name: tank-0004
     addnode:
       - tank-0000
@@ -30,8 +30,16 @@ nodes:
             block: 300
             index: 2
           target: tank-0005-ln
-          capacity: 50000
-          push_amt: 25000
+          capacity: 5000000
+          push_amt: 250000
   - name: tank-0005
     addnode:
-      - tank-0000
\ No newline at end of file
+      - tank-0000
+
+plugins:
+  postDeploy:
+    simln:
+      entrypoint: "../../../resources/plugins/simln"
+
+caddy:
+  enabled: true
\ No newline at end of file
diff --git a/test/data/ln/node-defaults.yaml b/test/data/ln/node-defaults.yaml
index 62e05199..d82ef96f 100644
--- a/test/data/ln/node-defaults.yaml
+++ b/test/data/ln/node-defaults.yaml
@@ -5,8 +5,8 @@ image:
   repository: bitcoindevproject/bitcoin
   pullPolicy: IfNotPresent
   tag: "27.0"
-collectLogs: false
-metricsExport: false
+collectLogs: true
+metricsExport: true
 
 #LN configs
 ln:
@@ -16,7 +16,7 @@ lnd:
     color=#000000
   config: |
     bitcoin.timelockdelta=33
-  metricsExport: false
+  metricsExport: true
   prometheusMetricsPort: 9332
   extraContainers:
     - name: lnd-exporter

@pinheadmz pinheadmz merged commit 8a2e34c into bitcoin-dev-project:main Mar 17, 2025
15 checks passed
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.

4 participants