-
Notifications
You must be signed in to change notification settings - Fork 55
Circuit breaker plugin #688
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
Circuit breaker plugin #688
Conversation
weird error trying to
|
Yeah, I believe this issue is related to #689. To resolve it, I |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
ce43d60
to
0418553
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a few things to check. Coudn't get the branch to run locally probably just because of the image error?
Also checkout the legacy branch for how we did integration tests for circuitbreaker:
https://github.com/bitcoin-dev-project/warnet/blob/legacy/test/ln_test.py#L34-L39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, thanks for the update. Be sure to check out the legacy ln test that covered circuitbreaker: https://github.com/bitcoin-dev-project/warnet/blob/legacy/test/ln_test.py
I don't think the hello
network is very useful actually, and it would be better to add the circuit breaker plugin to one of the networks in test/data
so it can actually be run and tested by ci.
Thanks. I have added circuitbreaker to a network and some unit tests for it has been included as well |
resources/charts/bitcoincore/charts/lnd/templates/configmap.yaml
Outdated
Show resolved
Hide resolved
{{- if .Values.circuitbreaker.enabled }} | ||
- name: circuitbreaker | ||
image: {{ .Values.circuitbreaker.image | quote }} | ||
imagePullPolicy: IfNotPresent | ||
args: | ||
- "--network={{ .Values.global.chain }}" | ||
- "--rpcserver=localhost:{{ .Values.RPCPort }}" | ||
- "--tlscertpath=/tls.cert" | ||
- "--macaroonpath=/root/.lnd/data/chain/bitcoin/{{ .Values.global.chain }}/admin.macaroon" | ||
- "--httplisten=0.0.0.0:{{ .Values.circuitbreaker.httpPort }}" | ||
volumeMounts: | ||
- name: shared-volume | ||
mountPath: /root/.lnd/ | ||
- name: config | ||
mountPath: /tls.cert | ||
subPath: tls.cert | ||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there an issue including circuitbreaker as extraContainers container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was not able to access the value of the port and network variables as the extraContainers
is being parsed by a YAML parser before Helm gets to process the templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm the whole point of those extraContainers
was for the cb plugin ... :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understand, but I could not access a dynamic network or port while using it. The current approach also gives the option of enabling circuitbreaker if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinheadmz I assume it is acceptable to define network(chain) in network.yaml or node-defaults.yaml based on user desired scenario. if so then extraContainers should be possible...
test/ln_basic_test.py
Outdated
try: | ||
subprocess.run( | ||
[ | ||
"kubectl", | ||
"expose", | ||
"pod", | ||
pod_name, | ||
"--name", | ||
service_name, | ||
"--port", | ||
str(self.cb_port), | ||
"--target-port", | ||
str(self.cb_port), | ||
], | ||
check=True, | ||
) | ||
except subprocess.CalledProcessError as e: | ||
self.log.error(f"Failed to create service: {e.stderr}") | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will do this instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is overkill. See the legacy test, you can just execute an API request form inside the cb container. (this exact code wont work any more but maybe gives a good hint)
Lines 34 to 39 in 01195c2
def get_cb_forwards(self, index): | |
cmd = "wget -q -O - 127.0.0.1:9235/api/forwarding_history" | |
res = self.wait_for_rpc( | |
"exec_run", [index, ServiceType.CIRCUITBREAKER.value, cmd, self.network_name] | |
) | |
return json.loads(res) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into the legacy test but was unable to replicate it or access circuitbreaker
through localhost(127.0.0.1)
without port forwarding. I’ll do more research to figure out how to properly access it, but I’m also open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could add a util funciton to k8s.py to execute an arbitrary command inside a container (surprised we dont have that alreaady actually....) and then execute the command curl 127.0.0.1/whatever/cb/api...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right or do something like this in the test:
cmd = f"kubectl -n {namespace} exec {tank} --container {BITCOINCORE_CONTAINER} -- bitcoin-cli {method} {' '.join(map(str, params))}"
return run_command(cmd)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will try this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @pinheadmz , the command has been updated as you suggested and now uses run_command
imported from warnet.process
.
This is ready for another review
5df0de9
to
635be37
Compare
b616664
to
e9ffa29
Compare
I still think the test can be simplified a great deal, IMO you dont need those helper functions, observe deploy the test network outside of the test
execute api calls inside the circuit breaker container and retrieve the output
|
e9ffa29
to
e231987
Compare
Thanks for the feedback! I've simplified the test as suggested, removed the complex service setup/port forwarding and replaced it with direct kubectl exec calls to query the circuit breaker API. Let me know if you'd like any further tweaks |
e231987
to
5905f45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5905f45
Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 5905f4523a348c193164abb9c85c7a37d0f871c6
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmhAUXQACgkQ5+KYS2KJ
yTo2thAAzCOMBFbrOoLzhryqKoz2RWAWIkfpt7tZLbPI48sF7cJ2ikBqIYsWvBdx
UUvArwthp8qJRpcYP5VDOCbPqXyQBH5f+pGRduRq4ilEMK4c/QYDyKdPM3VqwWer
edKSGdSIQUi5B/kHwcu+TL+gJPkfbRdv1uvLID551S1yyqPhaCBVazEDzEwg9UFz
2JsZeWmpVYYxvVVkFRh1D3yJn9l5zHBchRPWH4klVHEdvVRoXzq++CMTiv6AxPf/
6DxngBa4FW8m6yxJJyZh0JucymMgrCt2kvP1E11FjF65EXxuMhHUoe1+BPLdrdll
fKGE6id8BsvyTFTWmsZwxOCLkNeLfEAaAsvoIaf9gL01iZZ3dD7w+SPwNvlW3MzE
yufzKFl3GvWSw3pPiMQjdFkwFFs5Yg5SAlhk15nW0GhsINtDpuEDAI+eE6HHGC2V
hlh0BZvhQAWDS36cPIPgQ038fS/uqGE6D4BRRzFlXRSm+rx/ugsx+j9jIlDALy/W
+UsItHB/EqlTne7jkaqEHAZaQ3uDhHSBMchWIylm7XwWXTVTARnsV490wlH3nwM/
1don7jyhxO1za8LwkAH330yRpQPolm9w2oDzXdNoIMbQZl3oqGkV2ljoUlK4qNhi
kAlbrx5SHjhfWuAtWKqKqIyB5UB0J8DWHWjc8cfrbVcc+IF/5Eg=
=WLKL
-----END PGP SIGNATURE-----
pinheadmz's public key is on openpgp.org
Thank you so much! great work!!!! |
Thanks a lot for the guidance as well. It was great working on this |
Add Circuit Breaker sidecar integration for LND nodes