[#102] improvement: improve startup script #103

orenccl · 2024-11-07T14:51:47Z

What changes were proposed in this pull request?

Removed the debug option set -x.
Fixed warnings identified by IntelliJ, such as changing cd "${playground_dir}" >/dev/null to cd "${playground_dir}" >/dev/null || exit 1 to prevent failures when cd fails.
Updated docker run --pull always hello-world >/dev/null 2>&1 to docker run --rm --pull always hello-world:latest >/dev/null 2>&1 to avoid pulling multiple images and ensure the container is removed after it finishes.
Changed naming conventions and log formats for consistency.
Added disk, RAM, and CPU checks.
Added the --skip-checks option for a quicker startup.
Corrected the log suffix format from %Y%m%d%H%m%s to %Y%m%d%H%M%s.
Removed the unnecessary Confirm the requirement is available in your OS [Y/n]:.
Combined port check status into one line instead of multiple lines.

Why are the changes needed?

To improve the user experience.
fix #102
fix #113

Does this PR introduce any user-facing change?

Added a new option --skip-checks|-s for a quicker startup.

How was this patch tested?

Tested with the following commands:

./playground start
./playground status
./playground stop
./playground start --skip-checks
./playground start -s

These tests were run to ensure the script works as intended.

orenccl · 2024-11-08T04:41:13Z

Hi @unknowntpo,

I've resolved the conflicts based on your recently merged PR #56.

Would you please review this? If possible, I would appreciate if you could also test it in your environment.

Thanks!

unknowntpo · 2024-11-08T08:48:54Z

@orenccl
Okay, I'll take a look :)

xunliu · 2024-11-08T09:27:43Z

playground.sh

-  docker run --pull always hello-world >/dev/null 2>&1
+  echo "[INFO] Testing Docker environment by running hello-world..."
+  # Use always to test network connection
+  docker run --rm --pull always hello-world:latest >/dev/null 2>&1


Please specify hello-world fixed tag, do not use latest

xunliu · 2024-11-08T09:31:47Z

playground.sh

+      checkDockerDisk
+      checkDockerRam
+      checkDockerCpu
+


Please remove this blank line.

xunliu · 2024-11-08T09:33:27Z

playground.sh


  case "$runtime" in
  k8s)
-	helm uninstall --namespace gravitino-playground gravitino-playground 
+  helm uninstall --namespace gravitino-playground gravitino-playground


I think need to add spaces in the line head, keep code sytle.

xunliu · 2024-11-08T09:37:14Z

playground.sh

+skipChecks=false
+
+case "$3" in
+--skip-checks)
+  skipChecks=true;
+  ;;
+-s)
+  skipChecks=true;
+  ;;


Do we need skipChecks startup param? It feels too complicated.
I think we must check docker or k8s envorinments.

Thank you for the feedback. I'll remove the skipChecks parameter as suggested. The test didn't take much time, so this change should be straightforward.

xunliu · 2024-11-08T09:45:14Z

hi @orenccl Thank you for your improve playground
I think we didn't neet specify docker|k8s param, we can check use environment,

if user only have docker, then launch playground on Docker
if use only have k8s, then launch playground on Docker
if use both have docker and k8s, then output a menu, let use select one to launch.

orenccl · 2024-11-09T08:52:43Z

unknowntpo · 2024-11-09T13:15:03Z

if user only have docker, then launch playground on Docker

if use only have k8s, then launch playground on Docker

if use both have docker and k8s, then output a menu, let use select one to launch.

@xunliu I think letting user to run both k8s and docker is more handy.

xunliu · 2024-11-09T13:21:29Z

playground.sh

+
+testK8s() {
+  echo "[INFO] Testing K8s environment..."
+  kubectl cluster-info


I think we need to check if there exists kubectl first.

Before the start, status, or stop commands are run, the checkCurrentRuntime function will be executed.

The checkCurrentRuntime function checks whether kubectl is present.

xunliu · 2024-11-09T13:22:20Z

playground.sh

@@ -64,140 +127,277 @@ checkHelm() {
  if [ $isExist ]; then
    true # Placeholder, do nothing
  else
-    echo "ERROR: Helm command not found, Please install helm v3."
+    echo "[ERROR] Helm check failed: Helm command not found, Please install helm v3."
    exit
  fi
  # check version
  # version will be like:
  # Version:"v3.15.2"
  regex="Version:\"(v[0-9]\.[0-9]+\.[0-9])\""
  version=$(helm version)


I think we needs check if exist helm first.

This section checks if Helm exists:

isExist=$(command -v helm) if [ "$isExist" ]; then

I've unified the usage of which and command -v to use command -v for consistent style.

if command -v helm >/dev/null 2>&1; then

xunliu · 2024-11-09T13:28:32Z

playground.sh

      --set projectRoot=$(pwd)
    ;;
  docker)
-    logSuffix=$(date +%Y%m%d%H%m%s)
-    docker-compose up --detach


docker-compose or docker compose？
My macOS only have docker-compose.
docker compose is Linux OS?
Please check it.

Maybe you needs to check operation OS, and add different process.

It's not about the OS; it's about the Docker version. Newer versions of Docker have a built-in docker compose command, whereas older versions do not. For older versions, you need to use docker-compose, which is no longer being actively updated.

I added a function to automatically detect the appropriate Docker Compose command.

Reference:
Docker Compose Migration Guide

orenccl · 2024-11-11T10:01:54Z

Update this PR:

Please let me know if there are any problems.

jerryshao · 2024-11-18T03:21:47Z

Hi @orenccl would you please rebase this PR to fix the conflicts?

danhuawang · 2024-11-18T07:18:59Z

Update this PR:

Unified port number for Docker and Kubernetes.

update readme

Please let me know if there are any problems.

@orenccl Can you help check this issue when I ran the fileset example in jupyter? By the way ,we have 4 examples in jupyter, can you help verify them by access http://localhost:18888 ,especially k8s runtime.

orenccl · 2024-11-18T13:54:44Z

Hi @danhuawang,

Sorry about this. I found that the commit Unified port number for Docker and Kubernetes actually caused some issues with internal communication within the Kubernetes cluster.

This should be addressed in a separate issue, so I have reverted it and focused on the original problem. I've also updated the README to clarify the container and host port mapping, so users won't be confused about which port to forward.

Could you review this when you have free time?

danhuawang · 2024-11-19T12:26:41Z

playground.sh

  else
-    echo "ERROR: There was an issue running kubectl cluster-info, please check you K8s cluster."
+    # Check available space in the root partition if the directory doesn't exist (special case for WSL)
+    availableSpaceKB=$(df --output=avail / | awk 'NR==2 {print $1}')


I encounter this issue when I launch with docker runtime. (MacOS)

df: unrecognized option `--output=avail' usage: df [--libxo] [-b | -g | -H | -h | -k | -m | -P] [-acIilnY] [-,] [-T type] [-t type] [file | filesystem ...] ./playground.sh: line 90: / 1024 / 1024: syntax error: operand expected (error token is "/ 1024 / 1024")

It seems that macOS does not support --output=avail. Refer to https://ss64.com/mac/df.html.

Instead of using --output=avail, change it to -k so that it works on both macOS and Linux. Refer to https://linuxcommand.org/lc3_man_pages/df1.html.

However, I don't have a macOS environment. Could you check this for me?

I checked macOS ,it's ok now.

danhuawang · 2024-11-20T03:51:24Z

playground.sh

+    exit 1
+  fi
+
+  case "${runtime}" in
  k8s)
    kubectl -n gravitino-playground get pods -o wide


It's better to replace hard code namespace with ${playgroundRuntimeName} like the helm install command in start method

danhuawang · 2024-11-20T11:31:41Z

@orenccl conflicts need to be resolved.

danhuawang · 2024-11-21T02:12:45Z

@qqqttt123 Any comments?

xunliu · 2024-11-26T00:54:41Z

hi @orenccl
We plan to remove helm support, #110
So, maybe this PR needs to wait for review.

orenccl · 2024-11-26T01:14:20Z

@xunliu
Sure, no problem.

orenccl · 2024-12-17T17:27:15Z

I will update this PR soon since #110 has already been merged.

orenccl · 2024-12-18T13:58:52Z

@xunliu @danhuawang
Ready for review now! Please take a look when you have time

orenccl marked this pull request as draft November 8, 2024 02:56

orenccl force-pushed the improvement/startupScriptOptimize branch 2 times, most recently from f13393f to 68d5c63 Compare November 8, 2024 03:00

orenccl marked this pull request as ready for review November 8, 2024 04:26

xunliu reviewed Nov 8, 2024

View reviewed changes

orenccl marked this pull request as draft November 8, 2024 09:57

orenccl force-pushed the improvement/startupScriptOptimize branch from 44ee83f to 852bd35 Compare November 9, 2024 06:47

orenccl marked this pull request as ready for review November 9, 2024 08:53

xunliu reviewed Nov 9, 2024

View reviewed changes

orenccl marked this pull request as draft November 11, 2024 09:02

orenccl marked this pull request as ready for review November 11, 2024 10:01

orenccl marked this pull request as draft November 18, 2024 07:33

orenccl force-pushed the improvement/startupScriptOptimize branch from 025cd68 to 9c324df Compare November 18, 2024 07:34

orenccl marked this pull request as ready for review November 18, 2024 13:55

danhuawang reviewed Nov 19, 2024

View reviewed changes

danhuawang reviewed Nov 20, 2024

View reviewed changes

orenccl marked this pull request as draft November 20, 2024 12:45

orenccl force-pushed the improvement/startupScriptOptimize branch 3 times, most recently from bcb81b6 to 6dd049a Compare November 20, 2024 14:00

orenccl marked this pull request as ready for review November 20, 2024 14:01

orenccl marked this pull request as draft November 26, 2024 01:13

orenccl force-pushed the improvement/startupScriptOptimize branch from 6dd049a to 122639c Compare December 18, 2024 13:57

orenccl marked this pull request as ready for review December 18, 2024 13:58

orenccl mentioned this pull request Dec 23, 2024

README ./playground.sh commands should not contain "docker" #113

Closed

orenccl marked this pull request as draft December 23, 2024 14:12

orenccl force-pushed the improvement/startupScriptOptimize branch from 122639c to 5b762bc Compare December 23, 2024 14:12

orenccl marked this pull request as ready for review December 23, 2024 14:19

orenccl requested a review from danhuawang December 23, 2024 14:19

improve start up script

201e3a0

orenccl force-pushed the improvement/startupScriptOptimize branch from 5b762bc to 201e3a0 Compare December 25, 2024 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#102] improvement: improve startup script #103

[#102] improvement: improve startup script #103

orenccl commented Nov 7, 2024 •

edited

Loading

orenccl commented Nov 8, 2024

unknowntpo commented Nov 8, 2024 •

edited

Loading

xunliu Nov 8, 2024

orenccl Nov 8, 2024

xunliu Nov 8, 2024

xunliu Nov 8, 2024

xunliu Nov 8, 2024

orenccl Nov 8, 2024

xunliu commented Nov 8, 2024

orenccl commented Nov 9, 2024

unknowntpo commented Nov 9, 2024

xunliu Nov 9, 2024

orenccl Nov 10, 2024

xunliu Nov 9, 2024

orenccl Nov 10, 2024

xunliu Nov 9, 2024

orenccl Nov 10, 2024 •

edited

Loading

orenccl commented Nov 11, 2024 •

edited

Loading

jerryshao commented Nov 18, 2024

danhuawang commented Nov 18, 2024 •

edited

Loading

orenccl commented Nov 18, 2024

danhuawang Nov 19, 2024

orenccl Nov 19, 2024

danhuawang Nov 20, 2024

danhuawang Nov 20, 2024

orenccl Nov 20, 2024

danhuawang commented Nov 20, 2024

danhuawang commented Nov 21, 2024

xunliu commented Nov 26, 2024

orenccl commented Nov 26, 2024

orenccl commented Dec 17, 2024

orenccl commented Dec 18, 2024

[#102] improvement: improve startup script #103

Are you sure you want to change the base?

[#102] improvement: improve startup script #103

Conversation

orenccl commented Nov 7, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

orenccl commented Nov 8, 2024

unknowntpo commented Nov 8, 2024 • edited Loading

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

xunliu commented Nov 8, 2024

orenccl commented Nov 9, 2024

unknowntpo commented Nov 9, 2024

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

orenccl Nov 10, 2024 • edited Loading

Choose a reason for hiding this comment

orenccl commented Nov 11, 2024 • edited Loading

jerryshao commented Nov 18, 2024

danhuawang commented Nov 18, 2024 • edited Loading

orenccl commented Nov 18, 2024

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

danhuawang commented Nov 20, 2024

danhuawang commented Nov 21, 2024

xunliu commented Nov 26, 2024

orenccl commented Nov 26, 2024

orenccl commented Dec 17, 2024

orenccl commented Dec 18, 2024

orenccl commented Nov 7, 2024 •

edited

Loading

unknowntpo commented Nov 8, 2024 •

edited

Loading

orenccl Nov 10, 2024 •

edited

Loading

orenccl commented Nov 11, 2024 •

edited

Loading

danhuawang commented Nov 18, 2024 •

edited

Loading