swap out kubectl by mplsgrant · Pull Request #614 · bitcoin-dev-project/warnet

mplsgrant · 2024-09-19T08:32:24Z

Took a stab at swapping out kubectl addressing #609

It appears to mostly work; however, something is up with the signet test, and I didn't have a chance to figure out what.
There was also an oddball issues around stream returning python dicts and lists instead of json. I was able to avoid the impacts of this issue.
Some things remain less tested such as a few namespace items and the chain save-state functionality.

willcl-ark · 2024-09-19T12:45:49Z

Am I correct in thinking that all users may still require kubectl to be installed, so that the cluster can be configured? Or does the python lib bypass all that too?

I had always assumed it hinged on ~/.kube/config to know where to do the things it was going to do, but never actually checked it out. Checking the code it seems like there could be alternatives to load_kube_config() (in fact, I see here references to google auth, which sounds potentially useful to us on the credential side of things?)

Users may also want kubectl for any additional k8s applications, such as k9s, to work, unless they too can be tricked by dumping a python-generated config (from this kubernetes lib) into the user's ~.kube/config file ;)

If pure python is preferred, then this looks like a big move in the right direction!

pinheadmz · 2024-09-19T13:35:32Z

src/warnet/bitcoin.py

+    # The k8s lib seems to convert json into its python representation. The following dance seems to
+    # avoid the worst of it.
+    if resp.startswith("{") or resp.startswith("["):
+        literal = ast.literal_eval(resp)
+        json_string = json.dumps(literal)
+        return json_string


the old exec_run() function added this option to stream() for this: _preload_content=False

overall i think its better to restore the old exec_run() function in k8s.py from pre-italy and just call it here.
That's as far as I got on this project: 756df48b97acb1496b2958e12b895a00

addressed here via _preload_content=False: 16f869a

pinheadmz · 2024-09-19T13:36:59Z

src/warnet/bitcoin.py

+        logs = sclient.read_namespaced_pod_log(
+            name=pod_name,
+            namespace=get_default_namespace(),
+            container=container_name,
+            timestamps=True,
+        )


can also use the _log() internal function I added to get the log

(or pod_log())

fixed up here: 778a3eb

pinheadmz · 2024-09-19T13:37:48Z

src/warnet/bitcoin.py

    # Get the IP of node_b
-    cmd = f"kubectl get pod {tank_b} -o jsonpath='{{.status.podIP}}'"
-    tank_b_ip = run_command(cmd).strip()
+    tank_b_pod: V1Pod = sclient.read_namespaced_pod(name=tank_b, namespace=get_default_namespace())


why not use the get pod helpers in k8s.py?

fixed up here: 6d8ffa0
and here: e84416b

pinheadmz · 2024-09-19T13:47:34Z

src/warnet/control.py

-            print(f"Please consider waiting for the pod to become available. Encountered: {e}")
-    else:
-        pass  # cancelled by user
+    container_name = "bitcoincore" if pod_name.startswith("tank") else None


we can't ever rely on tank names conforming to any kind of convention

fixed up here: 04be184

pinheadmz · 2024-09-19T13:53:05Z

src/warnet/k8s.py

+    with open(KUBECONFIG) as file:
+        kubeconfig_data = yaml.safe_load(file)


will this file always exist? is this the only way to get the namespace?

I will need to revisit this.

I replaced open(KUBECONFIG) instances with two functions that handle errors: b1365d3

Regarding getting the namespace, I do believe that the namespace stored in the KUBECONFIG file is the source of truth for the namespace for user OR our default namespace which is "warnet" right now. We talked about changing that on the call to "default". I

pinheadmz · 2024-09-19T13:56:50Z

Still a few mentions git grep kubectl, but mostly just docs (should update) and bash scripts (do we still need?). The port forward command will be replaced by ingress #607

bdp-DrahtBot · 2024-09-19T21:53:21Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#621 (Make test_framework available for users when creating scenarios by pinheadmz)
#616 (Kubeconfig Step Thru by mplsgrant)
#610 (Offer to install helm into venv by mplsgrant)

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.

mplsgrant · 2024-09-19T22:29:19Z

Am I correct in thinking that all users may still require kubectl to be installed, so that the cluster can be configured? Or does the python lib bypass all that too?

@willcl-ark The python lib bypasses this. It currently relies on KUBECONFIG which is genreally at ~/.kube/config

it seems like there could be alternatives to load_kube_config()

I think the KubeConfigLoader provides access to KUBECONFIG, but it can also do config files in custom locations (maybe in memory, too?). Seems like the google credentials thing might be an easy win to modify KUBECONFIG with the correct google plugin data, but I have not had a chance to look into that yet.

Users may also want kubectl for any additional k8s applications, such as k9s

I don't think k9s or ktop requires kubectl, but I do think we should guide users towards downloading it b/c it is so useful.

Regarding setting the kubeconfig file, right now we rely on minikube to do that. I have also updated auth to create a KUBECONFIG file from a credential file if the user does not have an existing one: c666480

mplsgrant · 2024-09-19T22:34:56Z

It appears that signet remains an issue. Will need to revisit.

mplsgrant · 2024-09-21T02:53:27Z

It appears that signet remains an issue. Will need to revisit.

I fixed up the signet test issue: 02562b2

It uses shlex to split up lines. Allegedly, shlex is command-line-escape-character aware.

Also, provide some logic around overwriting existing entries and also provide a warning when the namepsace is not set.

We need to specify a container name when we have prometheus logging enabled b/c we end up with a couple containers per pod.

I went with the _preload_content=False approach

Instead rely on the position of the container. Position Rule: we get logs from the first container as reported by V1Pod's spec.container metadata.

shlex appears to handle escape sequences nicely.

I figure we should add an error type for the k8s file, that way we can try...except everything from that file using just one error type.

Incorporates better error handling

This use of get_default_namespace relies on the brittle idea that get_pods will always use get_default_namespace. Currently, get_pods currently does, in fact, use get_default_namespace. However, I don't like that we use get_default_namespace out of band relative to get_pods. I'd prefer that we do not mention namespaces at all in our message to users unless we can prove it.

pinheadmz reviewed Sep 19, 2024

View reviewed changes

pinheadmz mentioned this pull request Sep 19, 2024

Chaincode attackathon notes bitcoin-dev-project/battle-of-galen-erso#4

Closed

9 tasks

mplsgrant force-pushed the 2024-09-swap-out-kubectl branch from 59c173a to 04be184 Compare September 19, 2024 21:53

This was referenced Sep 19, 2024

Offer to install helm into venv #610

Merged

Add ingress #607

Merged

Add cmd for creating user kubeconfigs (cont. from #545) #598

Closed

rpc: add regex pattern filtering to warnet logs rpc #576

Closed

mplsgrant force-pushed the 2024-09-swap-out-kubectl branch from c666480 to 576c86d Compare September 20, 2024 15:01

mplsgrant force-pushed the 2024-09-swap-out-kubectl branch 2 times, most recently from 6431ca4 to f15fb2f Compare September 27, 2024 23:41

mplsgrant added 6 commits October 1, 2024 15:17

remove kubectl from stop_scenarios

1f79cbe

remove kubectl from snapshot_bitcoin_datadir

4204be5

auth: revamp to not use kubectl

7b19110

Also, provide some logic around overwriting existing entries and also provide a warning when the namepsace is not set.

_logs: do a container name dance 'bitcoincore'

1aed3fa

We need to specify a container name when we have prometheus logging enabled b/c we end up with a couple containers per pod.

remove todo from dag_connect

c31d0e5

clarify console text

5903cb3

mplsgrant added 24 commits October 1, 2024 15:22

_rpc: fix goofy json parsing workaround

844668a

I went with the _preload_content=False approach

k8s: add get_pod and get_service

10177ed

bitcoin: use get_pod and get_service

f4adc6e

_logs: do not rely on container name

366e42a

Instead rely on the position of the container. Position Rule: we get logs from the first container as reported by V1Pod's spec.container metadata.

clarify default namespace

9d47a89

auth: create kubeconfig from credential file

65f0717

fix colorization of log output

83f5f4d

clean up imports

fb0c109

control: catch None type; fix coloring

26d177e

test_base: use shlex to handle cmd splitting

0305247

shlex appears to handle escape sequences nicely.

k8s: add a K8sError

b277248

I figure we should add an error type for the k8s file, that way we can try...except everything from that file using just one error type.

k8s: update type in create_kubernetes_object

333e277

k8s: add open and write fns for KUBECONFIG

b943c81

k8s: update namespace and context checking fns

66d74b6

users: improve auth fn

f680268

Incorporates better error handling

docs: minor doc updates

a5cb478

bitcoin: remove debug_log

1237d67

control: add explicit ns to run --debug

84d24c1

k8s: use our internal get_pod fn

9d20ef8

k8s: use default namespace for edges()

c3ad1c0

DEFAULT_NAMESPACE: set it to "default"

6d49caf

k8s: use default namespace in apply fn

006dcc6

debug_log: bring it back

fcc9ed8

mplsgrant force-pushed the 2024-09-swap-out-kubectl branch from f15fb2f to fcc9ed8 Compare October 1, 2024 20:23

mplsgrant added 5 commits October 1, 2024 15:28

make ruff happy

a2a69f3

k8s: add continue to wait_for_init

8131456

add constants as they appear in the helm charts

df18670

use constants in the log function

f851e7c

use constant in the bitcoin rpc function

f1249c1

		with open(KUBECONFIG) as file:
		kubeconfig_data = yaml.safe_load(file)

Conversation

mplsgrant commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willcl-ark commented Sep 19, 2024

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!

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!

pinheadmz commented Sep 19, 2024

Uh oh!

bdp-DrahtBot commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

mplsgrant commented Sep 19, 2024

Uh oh!

mplsgrant commented Sep 19, 2024

Uh oh!

mplsgrant commented Sep 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mplsgrant commented Sep 19, 2024 •

edited

Loading

bdp-DrahtBot commented Sep 19, 2024 •

edited

Loading