-
Notifications
You must be signed in to change notification settings - Fork 54
patch addrman bucketing, remove build #261
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
Changes from all commits
d8e5342
2df9464
b00774b
837c7fb
edfc0c2
05a6eb3
ec9c4ac
7f826cb
861bdb2
0694fa0
2dba861
81c1b46
4f9e4fb
d3cbc61
41ce95d
ca0d06b
ce3800a
66444ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
|
|
||
| import yaml | ||
| from backends import BackendInterface, ServiceType | ||
| from cli.image import build_image | ||
| from kubernetes import client, config | ||
| from kubernetes.client.models.v1_pod import V1Pod | ||
| from kubernetes.client.rest import ApiException | ||
|
|
@@ -15,8 +16,9 @@ | |
| from warnet.tank import Tank | ||
| from warnet.utils import default_bitcoin_conf_args, parse_raw_messages | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
| DOCKER_REGISTRY_CORE = "bitcoindevproject/k8s-bitcoin-core" | ||
| DOCKER_REGISTRY_CORE = "bitcoindevproject/bitcoin" | ||
| DOCKER_REGISTRY_LND = "lightninglabs/lnd:v0.17.0-beta" | ||
| LOCAL_REGISTRY = "warnet/bitcoin-core" | ||
| POD_PREFIX = "tank" | ||
| BITCOIN_CONTAINER_NAME = "bitcoin" | ||
| LN_CONTAINER_NAME = "ln" | ||
|
|
@@ -157,13 +159,12 @@ def get_status(self, tank_index: int, service: ServiceType) -> RunningStatus: | |
| break | ||
| return RunningStatus.RUNNING if ready else RunningStatus.PENDING | ||
|
|
||
| def exec_run(self, tank_index: int, service: ServiceType, cmd: str, user: str = "root"): | ||
| # k8s doesn't let us run exec commands as a user, but we can use su | ||
| # because its installed in the bitcoin containers. we will need to rework | ||
| # this command if we decided to remove gosu from the containers | ||
| # TODO: change this if we remove gosu | ||
| def exec_run(self, tank_index: int, service: ServiceType, cmd: str): | ||
| pod_name = self.get_pod_name(tank_index, service) | ||
| exec_cmd = ["/bin/sh", "-c", f"su - {user} -c '{cmd}'"] | ||
| if service == ServiceType.BITCOIN: | ||
| exec_cmd = ["/bin/bash", "-c", f"{cmd}"] | ||
| elif service == ServiceType.LIGHTNING: | ||
| exec_cmd = ["/bin/sh", "-c", f"{cmd}"] | ||
| result = stream( | ||
| self.client.connect_get_namespaced_pod_exec, | ||
| pod_name, | ||
|
|
@@ -211,7 +212,7 @@ def get_bitcoin_cli(self, tank: Tank, method: str, params=None): | |
| cmd = f"bitcoin-cli -regtest -rpcuser={tank.rpc_user} -rpcport={tank.rpc_port} -rpcpassword={tank.rpc_password} {method} {' '.join(map(str, params))}" | ||
| else: | ||
| cmd = f"bitcoin-cli -regtest -rpcuser={tank.rpc_user} -rpcport={tank.rpc_port} -rpcpassword={tank.rpc_password} {method}" | ||
| return self.exec_run(tank.index, ServiceType.BITCOIN, cmd, user="bitcoin") | ||
| return self.exec_run(tank.index, ServiceType.BITCOIN, cmd) | ||
|
|
||
| def get_messages( | ||
| self, | ||
|
|
@@ -319,7 +320,6 @@ def tank_from_deployment(self, pod, pods_by_name, warnet): | |
| c_branch = env.value | ||
| if c_repo and c_branch: | ||
| t.version = f"{c_repo}#{c_branch}" | ||
| t.is_custom_build = True | ||
|
|
||
| # check if we can find a corresponding lnd pod | ||
| lnd_pod = pods_by_name.get(self.get_pod_name(index, ServiceType.LIGHTNING)) | ||
|
|
@@ -340,17 +340,37 @@ def default_bitcoind_config_args(self, tank): | |
|
|
||
| def create_bitcoind_container(self, tank) -> client.V1Container: | ||
| container_name = BITCOIN_CONTAINER_NAME | ||
| container_image = ( | ||
| tank.image if tank.is_custom_build else f"{DOCKER_REGISTRY_CORE}:{tank.version}" | ||
| ) | ||
| container_image = None | ||
|
|
||
| # Prebuilt image | ||
| if tank.image: | ||
| container_image = tank.image | ||
| # On-demand built image | ||
| elif "/" and "#" in tank.version: | ||
| # We don't have docker installed on the RPC server, where this code will be run from, | ||
| # and it's currently unclear to me if having the RPC pod build images is a good idea. | ||
| # Don't support this for now in CI by disabling in the workflow. | ||
|
|
||
| # This can be re-enabled by enabling in the workflow file and installing docker and | ||
| # docker-buildx on the rpc server image. | ||
|
|
||
| # it's a git branch, building step is necessary | ||
| repo, branch = tank.version.split("#") | ||
| build_image( | ||
| repo, | ||
| branch, | ||
| LOCAL_REGISTRY, | ||
| branch, | ||
| tank.DEFAULT_BUILD_ARGS + tank.extra_build_args, | ||
| arches="amd64", | ||
| ) | ||
| # Prebuilt major version | ||
| else: | ||
| container_image = f"{DOCKER_REGISTRY_CORE}:{tank.version}" | ||
|
|
||
| container_env = [ | ||
| client.V1EnvVar(name="BITCOIN_ARGS", value=self.default_bitcoind_config_args(tank)) | ||
| ] | ||
| # TODO: support custom builds | ||
| if tank.is_custom_build: | ||
| # TODO: check if the build already exists in the registry | ||
| # Annoyingly the api differs between providers, so this is annoying | ||
| pass | ||
|
|
||
| return client.V1Container( | ||
| name=container_name, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| import sys | ||
|
|
||
| import click | ||
| from utils.image_build import build_and_upload_images | ||
|
|
||
| from .image_build import build_image | ||
|
|
||
|
|
||
| @click.group(name="image") | ||
|
|
@@ -16,11 +17,12 @@ def image(): | |
| @click.option("--tag", required=True, type=str) | ||
| @click.option("--build-args", required=False, type=str) | ||
| @click.option("--arches", required=False, type=str) | ||
| def build(repo, branch, registry, tag, build_args, arches): | ||
| @click.option("--action", required=False, type=str) | ||
| def build(repo, branch, registry, tag, build_args, arches, action="load"): | ||
| """ | ||
| 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. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| """ | ||
| res = build_and_upload_images(repo, branch, registry, tag, build_args, arches) | ||
| res = build_image(repo, branch, registry, tag, build_args, arches, action) | ||
| if not res: | ||
| sys.exit(1) | ||
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.
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
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.
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)