patch addrman bucketing, remove build #261

willcl-ark · 2024-02-07T00:51:26Z

Closes: #212

Yay, another PR which should be 3!

This turned out longer than I wanted, as after patching addrman bucketing I needed to update the images. These are currently based on a plethora of dockerfiles created using various scripts, with different versions for k8s and compose. By using the same image for both backends we can in the future drop our ipv4 generator code from compose and just have everything routable and bucketable on both. We also only need to rebuild half the number of images...

In undertaking this, I thought we could take the opportunity to switch to alpine-based images, as they're (much) smaller. This neccesitated some minor changes to exec methods in the backends as there is no gosu in alpine (but there is in LND pods, debian-based?)

Finally, the knock-on effect of this was breakage of the build test. In my opinion this should be removed altogether and we should require users to simply run:

warcli image build --registry=<my_registry>/<name> --repo=<gh_user>/<repo> --branch=<branch_name> --arches=amd64 --tag=<my_tag>

...and then supply image info to the graph. This is a bit more manual than auto-building, but it's not clear currently how that would work with k8s in any case. This ends up therefore removing compose-specific code only, and makes a few code sections much simpler. We could simplify even further by removing version and only permitting image, using our registry as the default. But I've left that for now.

In order to proceed with this change, if folks want things past the patch, we should first update https://hub.docker.com/r/bitcoindevproject/bitcoin-core with new unified (and patched) images, then remove the NO MERGE commit(s), both if we want to keep image building on compose, otherwise only the penultimate.

We can then remove the following registries too for extra cleanliness:

https://hub.docker.com/r/bitcoindevproject/k8s-bitcoin-core
https://hub.docker.com/r/bitcoindevproject/warnet-bitcoin
https://hub.docker.com/r/bitcoindevproject/warnet-bitcoin-core

willcl-ark · 2024-02-07T01:02:45Z

Also worth considering: I cannot claim to fully understand the ramifications of this addrman patch. I've observed different buckets and positions being used rather than a single bucket, so I know that much works. However it's unclear to me yet whether there are any currently-unforseen consequences of bucketing by /32...

It occurred to me that we may have unwittingly altered the total number of buckets, but there may be other side-effects too.

josibake · 2024-02-07T11:13:10Z

Also worth considering: I cannot claim to fully understand the ramifications of this addrman patch. I've observed different buckets and positions being used rather than a single bucket, so I know that much works. However it's unclear to me yet whether there are any currently-unforseen consequences of bucketing by /32...

Agree, and I think we should clearly document this (at least for now). I still think this is the best route forward, but we should run a live warnet scenario for awhile and compare the raw addrman against some real nodes to get a sense of how closely the change mirrors mainnet. If this ends up introducing side effects, we'll need to go back to the drawing board, but for now this seems to unblock us.

willcl-ark · 2024-02-07T11:48:30Z

FWIW, some image size comparisons:

Release image, current, debian-slim: 159MB
Built image, current, debian-slim: > 300MB!
(estimate built could be reduced to about 175MB but not tested)

Unified, proposed, alpine: 35.3MB

src/backends/compose/compose_backend.py

src/backends/kubernetes/kubernetes_backend.py

src/utils/image_build.py

josibake · 2024-02-07T12:13:34Z

Finally, the knock-on effect of this was breakage of the build test. In my opinion this should be removed altogether and we should require users to simply run: <script>

I do agree this should be the normal mode of operation, but I don't think we should disable the build script altogether, in that I think it's still useful for CI. Needing to push images first to a registry before the CI can pass seems error prone and a PITA. AFAIK, if we are using minikube in the CI, we can build the images and use them directly in minikube, so maybe we keep the building and CI stuff to where it doesn't rely on hitting the registry? Or both: one that builds and tests the images, and another that runs with the prebuilt images, more as a regression test?

There may also be users who want to use warnet locally with docker or minikube that would benefit from the build script for local work.

willcl-ark · 2024-02-07T12:18:12Z

I do agree this should be the normal mode of operation, but I don't think we should disable the build script altogether, in that I think it's still useful for CI. Needing to push images first to a registry before the CI can pass seems error prone and a PITA.
AFAIK, if we are using minikube in the CI, we can build the images and use them directly in minikube, so maybe we keep the building and CI stuff to where it doesn't rely on hitting the registry? Or both: one that builds and tests the images, and another that runs with the prebuilt images, more as a regression test?

Hmmm, I somewhat agree. let me see if the script can be modified to use --load, then we can use it to build and load into local registry, before using in minikube on CI. Then I'd argue that's equivalent to what we have now, and the rest can still go?

There may also be users who want to use warnet locally with docker or minikube that would benefit from the build script for local work.

I think this would be addressed by the above too

josibake · 2024-02-07T12:29:55Z

Hmmm, I somewhat agree. let me see if the script can be modified to use --load, then we can use it to build and load into local registry, before using in minikube on CI. Then I'd argue that's equivalent to what we have now, and the rest can still go?

Sounds excellent!

willcl-ark · 2024-02-07T15:58:16Z

Sorry for many rebases, somehow broke the scenarios test during the work...

willcl-ark · 2024-02-07T23:08:17Z

It also just occurred to me that with a unified image we could have a workflow to (re)build the docker images when we tag in this repo (or whatever)

willcl-ark · 2024-02-07T23:12:53Z

src/utils/image_build.py

-        build_args = (
-            '"--disable-tests --with-incompatible-bdb --without-gui --disable-bench '
-            "--disable-fuzz-binary --enable-suppress-external-warnings "
-            '--without-miniupnpc --without-natpmp"'


Don't need these as we don't install the deps for them in the image, so they don't get build automatically.

This is not working currently, but can be re-enabled in the near future (TM)

But disable it for now in CI

josibake

looks good, left a few comments

josibake · 2024-02-08T11:05:17Z

src/templates/entrypoint.sh

    chown -R debian-tor:debian-tor /home/debian-tor
    # Start tor in the background
-    gosu debian-tor tor &
+    su-exec debian-tor:debian-tor tor &


do we still need su-exec, now that we are always running as the root user?

leaving for Tor (for now)

josibake · 2024-02-08T11:05:48Z

src/templates/Dockerfile_unified_alpine

+    libzmq \
+    shadow \
+    sqlite-dev \
+    su-exec


can remove, yeah?

leaving for Tor (for now)

josibake · 2024-02-08T11:07:00Z

src/utils/image_build.py

        f" --tag {image_full_name}"
-        f" --file Dockerfile_k8 ."
+        f" --file src/templates/Dockerfile ."
        f" --push"


would be nicer if you added the {action} arg in this commit and then updated the justfile in the next, but not a big deal, was just very confused by the next commit haha

yeah. But rebasing is tRiCkY

josibake · 2024-02-08T11:07:44Z

src/cli/image.py

-    Build bitcoind and bitcoin-cli from <repo>/<branch> and deploy to <registry> as <tag>
-    This requires docker and buildkit to be enabled.
+    Build bitcoind and bitcoin-cli from <repo>/<branch> as <registry>:<tag>.
+    Optionally deploy to remote registry using --action=push, otherwise image is loaded to local registry.


note for a follow up , would be nice if we gave examples in our docs of how to use this script, just to make it more clear

josibake · 2024-02-08T11:10:21Z

src/backends/kubernetes/kubernetes_backend.py

 from warnet.status import RunningStatus
 from warnet.tank import Tank
 from warnet.utils import default_bitcoin_conf_args, parse_raw_messages



tbh im not sure this will work on k8s. generally in k8s, the pattern for building images is you have a custom image with the correct permissions (Kaniko is the most popular one), so you pass of a build context to a container running in the cluster, and that container returns the image. this has some additional nice properties of allowing you to build multiple images in parallel (i think?). either way, i think this will take some more finessing to get working.

I think this will just have the RPC container build the image no?

But I can't see what exactly you're commenting on here (thank GH)

willcl-ark · 2024-02-08T11:21:33Z

OK final force push done.

Now if this gets approved, I'll update images before removing the top commit and merging

josibake · 2024-02-08T11:34:44Z

ACK d3cc379

looks RFM, will let @m3dwards and @pinheadmz take a look first tho

This image is based on alpine linux, and will work on k8s and compose backends, as well as forming the base of custom builds. One image to rule them all, and in the darkness bind them. Or something.

m3dwards · 2024-02-08T12:36:54Z

ACK 66444ff

I think this is great.

josibake · 2024-02-08T13:35:54Z

reACK 66444ff

diff looks good!

pinheadmz · 2024-02-08T13:49:16Z

.github/workflows/test.yml

+        backend: [compose]
    steps:
      - uses: actions/checkout@v3
-      - if: matrix.backend == 'compose'


Did you have to remove this "if"? If we can just keep everything the same and remove "k8s" from the matrix on tests we wanna skip, that'll be cleaner and easier to undo

When we add K8s back here, we'll likely need docker installed to build images. So we'd need to remove the if then. (Unless we find a weird way of building)

pinheadmz · 2024-06-05T01:25:54Z

post post post post post post post post merge nit: #376

vercel bot temporarily deployed to Preview February 7, 2024 00:52 Inactive

willcl-ark mentioned this pull request Feb 7, 2024

Remove ip generator code #254

Closed

willcl-ark changed the title ~~patch: addrman buckets ipv4 based on /32~~ patch addrman bucketing, remove build Feb 7, 2024

willcl-ark changed the title ~~patch addrman bucketing, remove build~~ NOMERGE patch addrman bucketing, remove build Feb 7, 2024

willcl-ark marked this pull request as ready for review February 7, 2024 00:56

willcl-ark requested a review from josibake February 7, 2024 10:11

willcl-ark mentioned this pull request Feb 7, 2024

tank IPs are still 100.0.0.0/8 in K8s #253

Closed

josibake reviewed Feb 7, 2024

View reviewed changes

src/backends/compose/compose_backend.py Outdated Show resolved Hide resolved

src/backends/kubernetes/kubernetes_backend.py Outdated Show resolved Hide resolved

src/utils/image_build.py Show resolved Hide resolved

willcl-ark force-pushed the addrman-patch-remove-build branch from 57d04ad to 4fdc8ba Compare February 7, 2024 12:14

vercel bot temporarily deployed to Preview February 7, 2024 12:15 Inactive

m3dwards approved these changes Feb 7, 2024

View reviewed changes

willcl-ark force-pushed the addrman-patch-remove-build branch from 4fdc8ba to b7e280f Compare February 7, 2024 15:12

vercel bot temporarily deployed to Preview February 7, 2024 15:13 Inactive

vercel bot temporarily deployed to Preview February 7, 2024 15:15 Inactive

willcl-ark force-pushed the addrman-patch-remove-build branch from ab84b51 to aa72a8f Compare February 7, 2024 15:17

vercel bot temporarily deployed to Preview February 7, 2024 15:18 Inactive

vercel bot temporarily deployed to Preview February 7, 2024 15:23 Inactive

patch: addrman buckets ipv4 based on /32

d8e5342

willcl-ark force-pushed the addrman-patch-remove-build branch from 777fcb7 to 3eb92dc Compare February 7, 2024 15:56

vercel bot temporarily deployed to Preview February 7, 2024 15:57 Inactive

willcl-ark force-pushed the addrman-patch-remove-build branch from 10591c5 to 816baff Compare February 7, 2024 16:07

vercel bot temporarily deployed to Preview February 7, 2024 16:12 Inactive

willcl-ark force-pushed the addrman-patch-remove-build branch from 816baff to 1485f49 Compare February 7, 2024 19:57

willcl-ark commented Feb 7, 2024

View reviewed changes

willcl-ark force-pushed the addrman-patch-remove-build branch from 342c3e8 to 0c733bd Compare February 8, 2024 11:03

vercel bot temporarily deployed to Preview February 8, 2024 11:04 Inactive

willcl-ark added 11 commits February 8, 2024 11:07

switch to root user exec for alpine

ec9c4ac

update image-builder for alpine

7f826cb

justfile: add build command using new Dockerfile

861bdb2

image: rehome image_build under cli

0694fa0

image: fixup image cli command

2dba861

compose: add inline branch-build

81c1b46

ci: disable k8s builder

4f9e4fb

This is not working currently, but can be re-enabled in the near future (TM)

k8s: add inline branch-build

d3cbc61

But disable it for now in CI

server: lock during build stage to permit inline builds

41ce95d

cli: fix network start command return type

ca0d06b

ci: point minikube local registry to outer docker registry in CI

ce3800a

josibake reviewed Feb 8, 2024

View reviewed changes

willcl-ark force-pushed the addrman-patch-remove-build branch from 0c733bd to d3cc379 Compare February 8, 2024 11:20

vercel bot temporarily deployed to Preview February 8, 2024 11:21 Inactive

willcl-ark mentioned this pull request Feb 8, 2024

ASMap v2 #217

Closed

images: use new registry with unified image

66444ff

This image is based on alpine linux, and will work on k8s and compose backends, as well as forming the base of custom builds. One image to rule them all, and in the darkness bind them. Or something.

willcl-ark force-pushed the addrman-patch-remove-build branch from d3cc379 to 66444ff Compare February 8, 2024 12:26

willcl-ark changed the title ~~NOMERGE patch addrman bucketing, remove build~~ patch addrman bucketing, remove build Feb 8, 2024

vercel bot temporarily deployed to Preview February 8, 2024 12:27 Inactive

josibake merged commit 02e358d into bitcoin-dev-project:main Feb 8, 2024

pinheadmz reviewed Feb 8, 2024

View reviewed changes

patch addrman bucketing, remove build #261

patch addrman bucketing, remove build #261

Uh oh!

Conversation

willcl-ark commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willcl-ark commented Feb 7, 2024

Uh oh!

josibake commented Feb 7, 2024

Uh oh!

willcl-ark commented Feb 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josibake commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willcl-ark commented Feb 7, 2024

Uh oh!

josibake commented Feb 7, 2024

Uh oh!

willcl-ark commented Feb 7, 2024

Uh oh!

willcl-ark commented Feb 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josibake left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented Feb 8, 2024

Uh oh!

josibake commented Feb 8, 2024

Uh oh!

m3dwards commented Feb 8, 2024

Uh oh!

josibake commented Feb 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Jun 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

willcl-ark commented Feb 7, 2024 •

edited

Loading

josibake commented Feb 7, 2024 •

edited

Loading