scripts: add set log level script

[nadin-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[nadin-Starkware]

• scripts: add set log level script #10249 👈 (View in Graphite)
• main-v0.14.1

This stack of pull requests is managed by Graphite. Learn more about stacking.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions

---

scripts/set_log_level.py line 9 at r3 (raw file):

import requests
import signal
import socket

Do any of these require chagnes in requirements.txt ?

Code quote:

import argparse
import subprocess
import sys
import time
from typing import List

import requests
import signal
import socket

---

scripts/set_log_level.py line 13 at r3 (raw file):

def parse_args(args: List[str]) -> argparse.Namespace:
    parser = argparse.ArgumentParser(description="Set the log level for a crate")

Module/crate

Code quote:

for a crate

---

scripts/set_log_level.py line 15 at r3 (raw file):

    parser = argparse.ArgumentParser(description="Set the log level for a crate")
    parser.add_argument(
        "--crate_name", type=str, help="The name of the crate to set the log level for"

Please rephrase this

Code quote:

"--crate_name"

---

scripts/set_log_level.py line 20 at r3 (raw file):

    parser.add_argument(
        "--pod_name",
        type=str,

Is this a string or an int?

Code quote:

str

---

scripts/set_log_level.py line 30 at r3 (raw file):

        default=8082,
        help="Local port to bind the port-forward to (defaults to 8082)",
    )

Is this applicable only in the "port-forwarding" mode? If so, can the arg be grouped as such?

Code quote:

    parser.add_argument(
        "--local_port",
        type=int,
        default=8082,
        help="Local port to bind the port-forward to (defaults to 8082)",
    )

---

scripts/set_log_level.py line 36 at r3 (raw file):

        type=int,
        default=8082,
        help="Monitoring port exposed by the pod (defaults to 8082)",

Monitoring port exposed by the pod -> Monitoring endpoint port

Please check this code re default values:

What argparse itself does

If you enable ArgumentDefaultsHelpFormatter, argparse automatically appends the default value in that same format:

parser = argparse.ArgumentParser(
    formatter_class=argparse.ArgumentDefaultsHelpFormatter
)


Then you can write:

parser.add_argument("--count", type=int, default=10, help="Number of items")


And argparse will display:

--count COUNT   Number of items (default: 10)

Code quote:

 help="Monitoring port exposed by the pod (defaults to 8082)",

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @nadin-Starkware)

---

scripts/set_log_level.py line 58 at r3 (raw file):

    Returns the Popen handle so the caller can terminate it later.
    Raises RuntimeError if the port never becomes ready.

This is inaccurate: there's a timeout mechanism

Code quote:

 if the port never becomes ready.

---

scripts/set_log_level.py line 73 at r3 (raw file):

                return proc
        except OSError:
            time.sleep(0.4)

Where did this number come from?

Code quote:

time.sleep(0.4)

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 9 unresolved discussions (waiting on @nadin-Starkware)

---

scripts/set_log_level.py line 130 at r3 (raw file):

            print(f"✅ Successfully set log level for {args.crate_name} to {args.log_level}")
        else:
            print(f"Unsupported method {args.method}. Use 'get' or 'post'.")

IIUC providing a wrong value will result in arg parsing error, before this code is even invoked, i.e., this will never run to begin with.

Code quote:

print(f"Unsupported method {args.method}. Use 'get' or 'post'.")

[nadin-Starkware]

Reviewable status: 0 of 1 files reviewed, 9 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

scripts/set_log_level.py line 9 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Do any of these require chagnes in requirements.txt ?

No

---

scripts/set_log_level.py line 13 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Module/crate

Done.

---

scripts/set_log_level.py line 15 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please rephrase this

Done.

---

scripts/set_log_level.py line 20 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Is this a string or an int?

Pod name? It's a string

---

scripts/set_log_level.py line 30 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Is this applicable only in the "port-forwarding" mode? If so, can the arg be grouped as such?

Done.

---

scripts/set_log_level.py line 36 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Monitoring port exposed by the pod -> Monitoring endpoint port

Please check this code re default values:

What argparse itself does

If you enable ArgumentDefaultsHelpFormatter, argparse automatically appends the default value in that same format:

parser = argparse.ArgumentParser(
    formatter_class=argparse.ArgumentDefaultsHelpFormatter
)


Then you can write:

parser.add_argument("--count", type=int, default=10, help="Number of items")


And argparse will display:

--count COUNT   Number of items (default: 10)

Done.

---

scripts/set_log_level.py line 58 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

This is inaccurate: there's a timeout mechanism

Done.

---

scripts/set_log_level.py line 73 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Where did this number come from?

Short enough to keep the overall wait reasonable - ​with the default max_attempts = 5, the worst-case delay is about 5 × 0.4 s = 2 s

---

scripts/set_log_level.py line 130 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

IIUC providing a wrong value will result in arg parsing error, before this code is even invoked, i.e., this will never run to begin with.

I agree. I originally included it in case the argument parsing shifted, but it’s removed now

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @nadin-Starkware)

---

scripts/set_log_level.py line 20 at r3 (raw file):

Previously, nadin-Starkware (Nadin Jbara) wrote…

Pod name? It's a string

Sorry meant to mark the port number (which should be an int, no?)

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @nadin-Starkware)

---

scripts/set_log_level.py line 73 at r3 (raw file):

Previously, nadin-Starkware (Nadin Jbara) wrote…

Short enough to keep the overall wait reasonable - ​with the default max_attempts = 5, the worst-case delay is about 5 × 0.4 s = 2 s

Define a proper const then? 🙏

---

scripts/set_log_level.py line 61 at r4 (raw file):

        default=8082,
        help="Monitoring endpoint port",
    )

I am probably missing something. What is the intended usage of this? How can I run the script with:

1. port forwarding enabled
2. port forwarding disabled
   ?

Code quote:

def add_port_forward_args(parser: argparse.ArgumentParser) -> None:
    """Add port-forwarding related CLI options to the parser."""

    pf_group = parser.add_argument_group("port-forwarding options")

    pf_group.add_argument(
        "--pod_name",
        type=str,
        default="",
        help="Pod to port-forward to; omit when no port forwarding is needed",
    )

    pf_group.add_argument(
        "--local_port",
        type=int,
        default=8082,
        help="Local port to bind the port-forward to",
    )

    pf_group.add_argument(
        "--monitoring_port",
        type=int,
        default=8082,
        help="Monitoring endpoint port",
    )

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @nadin-Starkware)

---

scripts/set_log_level.py line 102 at r4 (raw file):

    # If a pod name is supplied, establish a port-forward before making the request
    port_forward_proc = None

Is that a flag indicating whether the operation succeeded?
Can it be set solely in the try and expect cases, and not here?

Code quote:

    # If a pod name is supplied, establish a port-forward before making the request
    port_forward_proc = None

[nadin-Starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

scripts/set_log_level.py line 20 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Sorry meant to mark the port number (which should be an int, no?)

Indeed (and it is)

---

scripts/set_log_level.py line 73 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Define a proper const then? 🙏

Done.

---

scripts/set_log_level.py line 61 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I am probably missing something. What is the intended usage of this? How can I run the script with:

1. port forwarding enabled
2. port forwarding disabled
   ?

1. Pass pod_name arg
2. Don't pass pod_name arg

---

scripts/set_log_level.py line 102 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Is that a flag indicating whether the operation succeeded?
Can it be set solely in the try and expect cases, and not here?

It's not a success flag; it stores the kubectl process handle.
It starts as None, so the finally block can know whether there is a process to clean up.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @nadin-Starkware)

---

scripts/set_log_level.py line 61 at r4 (raw file):

Previously, nadin-Starkware (Nadin Jbara) wrote…

1. Pass pod_name arg
2. Don't pass pod_name arg

what happens if I pass pod_name but don't pass local_port or monitoring_port?

---

scripts/set_log_level.py line 150 at r5 (raw file):

    finally:
        # Clean up the port-forward process if we started one
        if port_forward_proc:

Can this check be replaced with if args.pod_name ?

Code quote:

if port_forward_proc:

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @nadin-Starkware)

---

scripts/set_log_level.py line 109 at r5 (raw file):

    base_port = args.local_port if args.pod_name else target_port

    if args.pod_name:

I suggest defining an explicit variable setup_port_forwarding = args.pod_name == True (or w/e the syntax is)

Code quote:

if args.pod_name

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @nadin-Starkware)

---

scripts/set_log_level.py line 150 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Can this check be replaced with if args.pod_name ?

(or the usage of the new var)

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @nadin-Starkware)

---

scripts/set_log_level.py line 150 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

(or the usage of the new var)

Motivation: it's clearer from the var name what's happening

[nadin-Starkware]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware)

---

scripts/set_log_level.py line 61 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

what happens if I pass pod_name but don't pass local_port or monitoring_port?

It will use the default value - 8082

---

scripts/set_log_level.py line 109 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I suggest defining an explicit variable setup_port_forwarding = args.pod_name == True (or w/e the syntax is)

Done.

---

scripts/set_log_level.py line 150 at r5 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Motivation: it's clearer from the var name what's happening

args.pod_name only tells us the user requested a port-forward; it doesn’t guarantee the kubectl process actually started.

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nadin-Starkware)