unified HTML landing page (NOT a dashboard!) #531

willcl-ark · 2024-09-04T21:33:52Z

josibake · 2024-09-05T06:16:55Z

Uses the k8s API (it would be much less code to, as I had in my first iteration, to simply apply all *.yaml files in the caddy directory with subprocess). @m3dwards is this something we should also make a chart for like fork observer? I don't really know where the boundary of raw yaml files and charts lies, but this seemed simpler as a first approach

IMO we should be using helm charts to deploy everything, unless we have a specific reason why it can't be done via helm. The manifests folder is really just a leftover and should be deleted once logging has been migrated to a helm chart, so I don't think we should be adding more to it. One of the goals of using helm was to use the python K8s API less, and to get rid of all of the k8s resource yaml files in the repo in favour of using helm charts.

To me, this feels like a regression back to what we were doing before and reintroduces some of the old risks, e.g., multiple ways of doing things, difficult to manage config, hardcoded things in different places, etc.

willcl-ark · 2024-09-05T06:27:08Z

Great, that’s what I though too

willcl-ark · 2024-09-05T09:46:00Z

TODO:

add auto-port-forward and tear-down
docs

josibake · 2024-09-05T11:00:29Z

Nice, thanks for going the helm route! looks much better IMO. I have a PR that cleans up the config and puts all the resources / charts / directory paths in one place, could rebase on that, or I rebase on this if it merges first.

m3dwards · 2024-09-05T14:56:46Z

This is quite a cool idea! I like the implementation.

Could even have a command warnet not-dashboard that opens the webpage for you!

I also can access FO but not grafana yet.

willcl-ark · 2024-09-05T21:19:42Z

This is quite a cool idea! I like the implementation.

Thanks, really curious to learn more about integrating this into the bitcoincore chart itself when you have time.

Could even have a command warnet not-dashboard that opens the webpage for you!

warnet dashboard added in 585295f. Nice idea.

fork-observer test updated to now test via the caddy proxy

256dummy · 2024-09-05T22:16:59Z

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

Conflicts

No conflicts as of last run.

josibake · 2024-09-06T18:38:13Z

Concept ACK, very cool!

josibake · 2024-09-08T17:51:50Z

started a review, looks good. in the teardown section, there is a work around due to the fact that commanders are not in a helm chart. id prefer we helmify the commander first and then rebase this? should make this PR simpler. this is also the second PR ive looked at that has a work around due to the commanders not being configured/deployed in the same way as everything else.

willcl-ark · 2024-09-08T21:06:33Z

started a review, looks good. in the teardown section, there is a work around due to the fact that commanders are not in a helm chart. id prefer we helmify the commander first and then rebase this? should make this PR simpler. this is also the second PR ive looked at that has a work around due to the commanders not being configured/deployed in the same way as everything else.

Attempted in #550

m3dwards · 2024-09-09T18:01:21Z

@willcl-ark does Grafana work for you now? If so let's merge?

willcl-ark · 2024-09-09T18:33:37Z

I think this needs a rebase too? (On mobile)

josibake · 2024-09-10T12:39:52Z

now that #550 is merged, I think all this needs is a rebase and g2g?

willcl-ark · 2024-09-11T13:17:23Z

Reabsed

willcl-ark · 2024-09-11T13:18:14Z

Jesus more conflicts since I started the rebase! :'(

m3dwards · 2024-09-11T13:26:17Z

I'm sorry, I feel your pain.

If it makes you feel better, I had it from you this week too!

willcl-ark · 2024-09-11T13:27:04Z

Ok rebased again. The logging test didn't work for me locally, but it's pretty flakey still regarding whether it gets any data for me (usually), so will see what CI says

willcl-ark · 2024-09-11T13:30:36Z

src/warnet/graph.py

@@ -180,6 +184,7 @@ def inquirer_create_network(project_path: Path):
        custom_network_path,
        fork_observer,
        fork_observer_query_interval,
+        fork_observer, # This enables caddy whenever fork-observer is enabled


Hmmm, now that logging is enabled by default, perhaps caddy must also be enabled by default in this PR @m3dwards ?

I'd say yes? if the dashboard is meant to be how a user interacts with logs , then logs: enabled implies caddy:enabled in my mind.

Now I'm thinking about it I think the logic should be if logging / metrics or if fork observer then yes caddy. I'm not sure we need the ability to disable caddy.

And technically, logging isn't enabled by default as both logs and metrics are disabled in values.yaml. It's only enabled if metrics or logs are switched to on in your network definition.

mmm ok. I see now in 0c4f76c which I need to remove also (didn't spot in the rebase)

See: https://github.com/bitcoin-dev-project/warnet/blob/main/src/warnet/deploy.py#L56

josibake

ACK, overall looks good. I'm not crazy about all of the code just for port forwarding, and even less excited to see arch specific code like is_windows but not a blocker for this PR since port forwarding is the best we have right now. I do think this PR is a pretty good example of why we should move to proper ingress, even if it costs more in a k8s environment.

There are a few other nits that I think would be really nice to address if you end up touching this again, mainly around hard coded constants that I feel should either be in constants.py or in the helm chart itself, but not blocking. Will happily re-review if you end up touching this again.

josibake · 2024-09-11T13:37:54Z

resources/manifests/grafana_values.yaml

@@ -16,6 +16,9 @@ grafana.ini:
  auth:
    disable_login_form: true
    disable_signout_menu: true
+  server:
+    # this is required to use Grafana behind a reverse proxy (caddy)
+    root_url: "%(protocol)s://%(domain)s:%(http_port)s/grafana/"


just for my own understanding, grafana stuff is in manifests/ for legacy reasons, yeah? was surprised its not also being deployed by helm (not a blocker for this PR tho)

josibake · 2024-09-11T13:40:33Z

src/warnet/dashboard.py

+
+
+@click.command()
+def dashboard():


meh on having a command for this. seems easier to just prompt the user in the logs? also this assumes port forwarding, which I don't think will always be the case, so this command wont work if we move to having proper ingress. lastly, id assume this webbrowser thing will end up failing for some weird reason on someone's computer (cough cough windows).

josibake · 2024-09-11T13:40:59Z

src/warnet/deploy.py

+
+    cmd = f"{HELM_COMMAND} {name} {CADDY_CHART} --namespace {namespace}"
+    if debug:
+        cmd += " --debug"


josibake · 2024-09-11T13:42:13Z

src/warnet/deploy.py

+    namespace = "warnet-logging"
+    name = "caddy"


"warnet-logging" should be a constant in constants.py imo, and name should definitely be coming from the helm chart and not hardcoded here.

This is modified from the fork-observer code which shoud also be updated.

Moved the constant, but getting the component name from the network file is not currently possible as this exists as a key only (so I would just be hardcoding the key here, named "caddy").

josibake · 2024-09-11T13:42:30Z

src/warnet/deploy.py

@@ -99,7 +128,8 @@ def deploy_fork_observer(directory: Path, debug: bool):
    if not network_file.get("fork_observer", {}).get("enabled", False):
        return

-    namespace = get_default_namespace()
+    default_namespace = get_default_namespace()
+    namespace = "warnet-logging"


same comment regarding constants.py

josibake · 2024-09-11T13:44:34Z

src/warnet/deploy.py

+def is_windows():
+    return sys.platform.startswith("win")


doesn't need to be addressed in this PR , but this is code smell to me and indicates we really should stop doing port forwarding in favour of ingress (see #529)

Yeah I think ingress can be its own PR

josibake · 2024-09-11T13:46:57Z

src/warnet/k8s.py

@@ -115,3 +115,21 @@ def get_default_namespace() -> str:
    command = "kubectl config view --minify -o jsonpath='{..namespace}'"
    kubectl_namespace = run_command(command)
    return kubectl_namespace if kubectl_namespace else DEFAULT_NAMESPACE
+
+
+def wait_for_caddy_ready(name, namespace, timeout=300):


this also feels a bit smelly to me, as this is what helm is supposed to be managing for us. I didn't see where this is being used, but im guessing it's for the port forwarding?

Yes waiting for caddy to be ready. This is a k8s thing not helm? I can't concieve of any other way you'd do this for the port forward...

Yeah, sorry if my comment was unclear, more saying this wait_for_caddy function seems specific to port forwarding, so we wouldn't need it if we weren't using port forwarding.

pinheadmz · 2024-09-11T19:38:25Z

will conflict with #585 but we can rebase whichever (add docs!)

m3dwards

LGTM

m3dwards · 2024-09-12T12:53:49Z

will conflict with #585 but we can rebase whichever (add docs!)

Will has rebased this half a dozen times so I think it's only fair to merge this one.

willcl-ark changed the title ~~faster and cleaner teardown~~ unified HTML springboard (NOT a dashboard!) Sep 4, 2024

willcl-ark force-pushed the caddy-proxy branch from 2022c97 to 917ecd3 Compare September 5, 2024 09:41

willcl-ark marked this pull request as ready for review September 5, 2024 10:36

willcl-ark changed the title ~~unified HTML springboard (NOT a dashboard!)~~ unified HTML landing page (NOT a dashboard!) Sep 5, 2024

256dummy mentioned this pull request Sep 5, 2024

Add create-network cli command #524

Merged

bdp-DrahtBot mentioned this pull request Sep 6, 2024

Finish refactor of main. Align setup, new & init commands #536

Merged

m3dwards force-pushed the caddy-proxy branch 2 times, most recently from 9ee680a to 396f87d Compare September 6, 2024 11:29

bdp-DrahtBot mentioned this pull request Sep 6, 2024

Refactor hardcoded strings into a config / constants file #528

Merged

willcl-ark force-pushed the caddy-proxy branch 3 times, most recently from b09542b to 5b1c0fd Compare September 7, 2024 13:49

willcl-ark force-pushed the caddy-proxy branch from 5b1c0fd to 6656540 Compare September 9, 2024 20:32

bdp-DrahtBot mentioned this pull request Sep 9, 2024

deploy commander using helm #550

Merged

willcl-ark force-pushed the caddy-proxy branch from 6656540 to c0d33df Compare September 11, 2024 13:17

willcl-ark force-pushed the caddy-proxy branch from c0d33df to 48df0db Compare September 11, 2024 13:26

This was referenced Sep 11, 2024

Updated install docs with guidance about testing minikube #574

Merged

create and load tank snapshots #555

Merged

willcl-ark commented Sep 11, 2024

View reviewed changes

josibake reviewed Sep 11, 2024

View reviewed changes

willcl-ark and others added 10 commits September 11, 2024 19:14

add caddy charts

bc02ce6

deploy caddy

1429a31

auto port-forward and teardown

258fb0a

document FO access via landing page

a28d262

move fork-observer and caddy into warnet-logging namespace

5df196d

deploy caddy if either fork-observer or logging enabled

31c08e1

add warnet dashboard

4879b03

test fork-observer access via caddy dashboard

48acdde

grafana running behind caddy

121d303

Update fork-observer chat note

20b97a0

willcl-ark force-pushed the caddy-proxy branch from 48df0db to 20b97a0 Compare September 11, 2024 18:31

willcl-ark marked this pull request as draft September 11, 2024 18:31

dont' copy caddy files to project dir

13f14c3

willcl-ark marked this pull request as ready for review September 11, 2024 18:55

willcl-ark force-pushed the caddy-proxy branch from 5c7b225 to 13f14c3 Compare September 11, 2024 19:03

logging: add to create inquirer

e75cace

willcl-ark mentioned this pull request Sep 11, 2024

Add ingress #587

Closed

m3dwards approved these changes Sep 12, 2024

View reviewed changes

m3dwards merged commit d4a0190 into bitcoin-dev-project:main Sep 12, 2024
11 checks passed

unified HTML landing page (NOT a dashboard!) #531

unified HTML landing page (NOT a dashboard!) #531

Conversation

willcl-ark commented Sep 4, 2024 • edited Loading

josibake commented Sep 5, 2024

willcl-ark commented Sep 5, 2024

willcl-ark commented Sep 5, 2024 • edited Loading

josibake commented Sep 5, 2024

m3dwards commented Sep 5, 2024 • edited Loading

willcl-ark commented Sep 5, 2024

256dummy commented Sep 5, 2024 • edited by bdp-DrahtBot Loading

Conflicts

josibake commented Sep 6, 2024

josibake commented Sep 8, 2024

willcl-ark commented Sep 8, 2024

m3dwards commented Sep 9, 2024

willcl-ark commented Sep 9, 2024

josibake commented Sep 10, 2024

willcl-ark commented Sep 11, 2024

willcl-ark commented Sep 11, 2024

m3dwards commented Sep 11, 2024 • edited Loading

willcl-ark commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3dwards Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

m3dwards Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josibake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinheadmz commented Sep 11, 2024 • edited Loading

m3dwards left a comment

Choose a reason for hiding this comment

m3dwards commented Sep 12, 2024

willcl-ark commented Sep 4, 2024 •

edited

Loading

willcl-ark commented Sep 5, 2024 •

edited

Loading

m3dwards commented Sep 5, 2024 •

edited

Loading

256dummy commented Sep 5, 2024 •

edited by bdp-DrahtBot

Loading

m3dwards commented Sep 11, 2024 •

edited

Loading

m3dwards Sep 11, 2024 •

edited

Loading

m3dwards Sep 11, 2024 •

edited

Loading

pinheadmz commented Sep 11, 2024 •

edited

Loading