Don't review: simple reader testing + building csi from scratch#1166
Don't review: simple reader testing + building csi from scratch#1166raj-prince wants to merge 1 commit intomainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
Summary of ChangesHello @raj-prince, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on enhancing the developer experience for building, deploying, and testing the GCS FUSE CSI driver. It includes minor adjustments to the Docker build process to resolve potential permission conflicts and integrates clear logging markers for custom builds. Furthermore, it provides a comprehensive set of scripts and Kubernetes manifests to automate the creation of a GKE test environment, deploy a test application, and offer a collection of command-line shortcuts for efficient interaction and troubleshooting of the CSI driver components. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: raj-prince The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several changes primarily for testing purposes, including adding debug logs to Go files and creating new shell scripts for environment setup. The modification in the Makefile to specify the user in docker run commands is a good improvement for security and file permissions. The newly added shell scripts contain hardcoded values, which limits their reusability, and some aliases could be refactored for better readability and portability. Additionally, temporary debug logs have been added across multiple Go files, which should be removed before merging to maintain clean production logs.
| klog.InitFlags(nil) | ||
| flag.Parse() | ||
|
|
||
| klog.Info("*** CUSTOM BUILD: CSI Driver - Manual Test Version ***") |
There was a problem hiding this comment.
This appears to be a temporary logging statement for debugging or testing purposes. Such logs should be removed before merging to avoid cluttering production logs.
| klog.InitFlags(nil) | ||
| flag.Parse() | ||
|
|
||
| klog.Info("*** CUSTOM BUILD: Metadata Prefetch - Manual Test Version ***") |
There was a problem hiding this comment.
This appears to be a temporary logging statement for debugging or testing purposes. Such logs should be removed before merging to avoid cluttering production logs.
| klog.InitFlags(nil) | ||
| flag.Parse() | ||
|
|
||
| klog.Info("*** CUSTOM BUILD: Sidecar Mounter - Manual Test Version ***") |
There was a problem hiding this comment.
This appears to be a temporary logging statement for debugging or testing purposes. Such logs should be removed before merging to avoid cluttering production logs.
| klog.InitFlags(nil) | ||
| flag.Parse() | ||
|
|
||
| klog.Info("*** CUSTOM BUILD: Webhook - Manual Test Version ***") |
There was a problem hiding this comment.
This appears to be a temporary logging statement for debugging or testing purposes. Such logs should be removed before merging to avoid cluttering production logs.
| gcloud beta container --project "gcs-tess" clusters create "rapid-ga-test-cluster-us-west4a" \ | ||
| --zone "us-west4-a" \ | ||
| --no-enable-basic-auth \ | ||
| --cluster-version "1.33.5-gke.2072000" \ | ||
| --release-channel "regular" \ | ||
| --machine-type "c4-standard-192" \ | ||
| --image-type "COS_CONTAINERD" \ | ||
| --disk-type "hyperdisk-balanced" \ | ||
| --disk-size "400" \ | ||
| --metadata disable-legacy-endpoints=true \ | ||
| --service-account "default" \ | ||
| --max-pods-per-node "110" \ | ||
| --num-nodes "8" \ | ||
| --logging=SYSTEM,WORKLOAD \ | ||
| --monitoring=SYSTEM,STORAGE,POD,DEPLOYMENT,STATEFULSET,DAEMONSET,HPA,CADVISOR,KUBELET \ | ||
| --enable-ip-alias \ | ||
| --network "projects/gcs-tess/global/networks/default" \ | ||
| --subnetwork "projects/gcs-tess/regions/us-west4/subnetworks/default" \ | ||
| --cluster-secondary-range-name "geertj-pods" \ | ||
| --no-enable-intra-node-visibility \ | ||
| --default-max-pods-per-node "110" \ | ||
| --enable-ip-access \ | ||
| --security-posture=standard \ | ||
| --workload-vulnerability-scanning=disabled \ | ||
| --addons HorizontalPodAutoscaling,HttpLoadBalancing,GcePersistentDiskCsiDriver \ | ||
| --enable-autoupgrade \ | ||
| --enable-autorepair \ | ||
| --max-surge-upgrade 1 \ | ||
| --max-unavailable-upgrade 0 \ | ||
| --binauthz-evaluation-mode=DISABLED \ | ||
| --enable-shielded-nodes \ | ||
| --shielded-integrity-monitoring \ | ||
| --no-shielded-secure-boot \ | ||
| --node-locations "us-west4-a" \ | ||
| --enable-gvnic \ | ||
| --workload-pool=gcs-tess.svc.id.goog No newline at end of file |
There was a problem hiding this comment.
This script has several issues that affect its usability and correctness:
- It is missing a shebang (e.g.,
#!/bin/bash) at the beginning of the file. - It contains many hardcoded values (e.g., project ID, cluster name, zone). These should be parameterized using variables to make the script reusable.
- The file is missing a newline at the end.
I suggest refactoring it to use variables for configuration and adding a shebang.
#!/bin/bash
set -euo pipefail
# Configuration
PROJECT="gcs-tess"
CLUSTER_NAME="rapid-ga-test-cluster-us-west4a"
ZONE="us-west4-a"
CLUSTER_VERSION="1.33.5-gke.2072000"
RELEASE_CHANNEL="regular"
MACHINE_TYPE="c4-standard-192"
IMAGE_TYPE="COS_CONTAINERD"
DISK_TYPE="hyperdisk-balanced"
DISK_SIZE="400"
NUM_NODES="8"
WORKLOAD_POOL="${PROJECT}.svc.id.goog"
CLUSTER_SECONDARY_RANGE_NAME="geertj-pods"
gcloud beta container --project "${PROJECT}" clusters create "${CLUSTER_NAME}" \
--zone "${ZONE}" \
--no-enable-basic-auth \
--cluster-version "${CLUSTER_VERSION}" \
--release-channel "${RELEASE_CHANNEL}" \
--machine-type "${MACHINE_TYPE}" \
--image-type "${IMAGE_TYPE}" \
--disk-type "${DISK_TYPE}" \
--disk-size "${DISK_SIZE}" \
--metadata disable-legacy-endpoints=true \
--service-account "default" \
--max-pods-per-node "110" \
--num-nodes "${NUM_NODES}" \
--logging=SYSTEM,WORKLOAD \
--monitoring=SYSTEM,STORAGE,POD,DEPLOYMENT,STATEFULSET,DAEMONSET,HPA,CADVISOR,KUBELET \
--enable-ip-alias \
--network "projects/${PROJECT}/global/networks/default" \
--subnetwork "projects/${PROJECT}/regions/us-west4/subnetworks/default" \
--cluster-secondary-range-name "${CLUSTER_SECONDARY_RANGE_NAME}" \
--no-enable-intra-node-visibility \
--default-max-pods-per-node "110" \
--enable-ip-access \
--security-posture=standard \
--workload-vulnerability-scanning=disabled \
--addons HorizontalPodAutoscaling,HttpLoadBalancing,GcePersistentDiskCsiDriver \
--enable-autoupgrade \
--enable-autorepair \
--max-surge-upgrade 1 \
--max-unavailable-upgrade 0 \
--binauthz-evaluation-mode=DISABLED \
--enable-shielded-nodes \
--shielded-integrity-monitoring \
--no-shielded-secure-boot \
--node-locations "${ZONE}" \
--enable-gvnic \
--workload-pool="${WORKLOAD_POOL}"
| PROJECT_ID="gcs-tess" | ||
| PROJECT_NUMBER="222564316065" | ||
| NAMESPACE="gcs-csi-test" | ||
| # BUCKET_NAME="gcs-fuse-warp-test-bucket" | ||
| BUCKET_NAME="princer-zonal-us-west4-a" | ||
| KSA_NAME="gcs-csi-test-sa" |
There was a problem hiding this comment.
The script contains hardcoded values for project ID, project number, namespace, bucket name, and KSA name. This makes the script difficult to reuse in different environments or for different tests. Consider parameterizing these values using environment variables with defaults or command-line arguments.
| PROJECT_ID="gcs-tess" | |
| PROJECT_NUMBER="222564316065" | |
| NAMESPACE="gcs-csi-test" | |
| # BUCKET_NAME="gcs-fuse-warp-test-bucket" | |
| BUCKET_NAME="princer-zonal-us-west4-a" | |
| KSA_NAME="gcs-csi-test-sa" | |
| PROJECT_ID="${PROJECT_ID:-gcs-tess}" | |
| PROJECT_NUMBER="${PROJECT_NUMBER:-222564316065}" | |
| NAMESPACE="${NAMESPACE:-gcs-csi-test}" | |
| # BUCKET_NAME="gcs-fuse-warp-test-bucket" | |
| BUCKET_NAME="${BUCKET_NAME:-princer-zonal-us-west4-a}" | |
| KSA_NAME="${KSA_NAME:-gcs-csi-test-sa}" |
| if [[ "$SKIP_SETUP" == "false" ]]; then | ||
| # Create GCS bucket | ||
| echo -e "\n1. Creating GCS bucket..." | ||
| gsutil mb -p $PROJECT_ID -l us-central1 gs://$BUCKET_NAME || echo "Bucket may already exist" |
There was a problem hiding this comment.
The command gsutil mb ... || echo "..." suppresses the exit code of gsutil mb. If gsutil mb fails for a reason other than the bucket already existing (e.g., authentication issue), set -e will not be triggered, and the script will continue, potentially leading to failures later on. A more robust approach would be to check if the bucket exists before attempting to create it.
| gsutil mb -p $PROJECT_ID -l us-central1 gs://$BUCKET_NAME || echo "Bucket may already exist" | |
| gsutil ls -b "gs://$BUCKET_NAME" &>/dev/null || gsutil mb -p "$PROJECT_ID" -l us-central1 "gs://$BUCKET_NAME" |
| alias desc-csi='kubectl describe daemonset gcsfusecsi-node' | ||
|
|
||
| # Check all custom builds | ||
| alias check-custom='echo "=== CSI Driver ===" && kubectl logs daemonset/gcsfusecsi-node -c gcs-fuse-csi-driver -n gcs-fuse-csi-driver --tail=100 | grep "CUSTOM BUILD" | head -2 && echo -e "\n=== Webhook ===" && kubectl logs deployment/gcs-fuse-csi-driver-webhook -n gcs-fuse-csi-driver --tail=100 | grep "CUSTOM BUILD" | head -2 && echo -e "\n=== Sidecar ===" && kubectl logs gcs-csi-test-pod -c gke-gcsfuse-sidecar -n gcs-csi-test --tail=100 2>/dev/null | grep "CUSTOM BUILD" | head -2 || echo "No sidecar pod found"' |
There was a problem hiding this comment.
The check-custom alias is very long and complex, which makes it hard to read and maintain. It's better to define it as a function for improved readability.
| alias check-custom='echo "=== CSI Driver ===" && kubectl logs daemonset/gcsfusecsi-node -c gcs-fuse-csi-driver -n gcs-fuse-csi-driver --tail=100 | grep "CUSTOM BUILD" | head -2 && echo -e "\n=== Webhook ===" && kubectl logs deployment/gcs-fuse-csi-driver-webhook -n gcs-fuse-csi-driver --tail=100 | grep "CUSTOM BUILD" | head -2 && echo -e "\n=== Sidecar ===" && kubectl logs gcs-csi-test-pod -c gke-gcsfuse-sidecar -n gcs-csi-test --tail=100 2>/dev/null | grep "CUSTOM BUILD" | head -2 || echo "No sidecar pod found"' | |
| check-custom() { | |
| echo "=== CSI Driver ===" | |
| kubectl logs daemonset/gcsfusecsi-node -c gcs-fuse-csi-driver -n gcs-fuse-csi-driver --tail=100 | grep "CUSTOM BUILD" | head -2 | |
| echo -e "\n=== Webhook ===" | |
| kubectl logs deployment/gcs-fuse-csi-driver-webhook -n gcs-fuse-csi-driver --tail=100 | grep "CUSTOM BUILD" | head -2 | |
| echo -e "\n=== Sidecar ===" | |
| kubectl logs gcs-csi-test-pod -c gke-gcsfuse-sidecar -n gcs-csi-test --tail=100 2>/dev/null | grep "CUSTOM BUILD" | head -2 || echo "No sidecar pod found" | |
| } |
| alias rebuild='cd /home/princer_google_com/dev/gcs-fuse-csi-driver && export PROJECT_ID=gcs-tess REGISTRY=gcr.io/gcs-tess/princer STAGINGVERSION=v999.999.999 BUILD_GCSFUSE_FROM_SOURCE=true && make build-image-and-push-multi-arch REGISTRY=$REGISTRY STAGINGVERSION=$STAGINGVERSION' | ||
| alias reinstall='cd /home/princer_google_com/dev/gcs-fuse-csi-driver && export REGISTRY=gcr.io/gcs-tess/princer STAGINGVERSION=v999.999.999 && make uninstall && make install' | ||
| alias update-csi='cd /home/princer_google_com/dev/gcs-fuse-csi-driver && export REGISTRY=gcr.io/gcs-tess/princer STAGINGVERSION=v999.999.999 && kubectl set image daemonset/gcsfusecsi-node gcs-fuse-csi-driver=$REGISTRY/gcs-fuse-csi-driver:$STAGINGVERSION -n gcs-fuse-csi-driver && kubectl set image deployment/gcs-fuse-csi-driver-webhook gcs-fuse-csi-driver-webhook=$REGISTRY/gcs-fuse-csi-driver-webhook:$STAGINGVERSION -n gcs-fuse-csi-driver' | ||
| alias recreate-pod='cd /home/princer_google_com/dev/gcs-fuse-csi-driver && ./setup-test-pod.sh --skip-setup' |
There was a problem hiding this comment.
The aliases rebuild, reinstall, update-csi, and recreate-pod contain a hardcoded absolute path /home/princer_google_com/dev/gcs-fuse-csi-driver. This makes the script specific to one user's environment and not portable. Consider using a variable for the path, which can be configured by the user. Also, using a subshell (...) for commands that change directory is a good practice to avoid changing the current directory of the user's shell.
| alias rebuild='cd /home/princer_google_com/dev/gcs-fuse-csi-driver && export PROJECT_ID=gcs-tess REGISTRY=gcr.io/gcs-tess/princer STAGINGVERSION=v999.999.999 BUILD_GCSFUSE_FROM_SOURCE=true && make build-image-and-push-multi-arch REGISTRY=$REGISTRY STAGINGVERSION=$STAGINGVERSION' | |
| alias reinstall='cd /home/princer_google_com/dev/gcs-fuse-csi-driver && export REGISTRY=gcr.io/gcs-tess/princer STAGINGVERSION=v999.999.999 && make uninstall && make install' | |
| alias update-csi='cd /home/princer_google_com/dev/gcs-fuse-csi-driver && export REGISTRY=gcr.io/gcs-tess/princer STAGINGVERSION=v999.999.999 && kubectl set image daemonset/gcsfusecsi-node gcs-fuse-csi-driver=$REGISTRY/gcs-fuse-csi-driver:$STAGINGVERSION -n gcs-fuse-csi-driver && kubectl set image deployment/gcs-fuse-csi-driver-webhook gcs-fuse-csi-driver-webhook=$REGISTRY/gcs-fuse-csi-driver-webhook:$STAGINGVERSION -n gcs-fuse-csi-driver' | |
| alias recreate-pod='cd /home/princer_google_com/dev/gcs-fuse-csi-driver && ./setup-test-pod.sh --skip-setup' | |
| alias rebuild='(cd ${GCS_FUSE_CSI_DRIVER_PATH:-/home/princer_google_com/dev/gcs-fuse-csi-driver} && export PROJECT_ID=gcs-tess REGISTRY=gcr.io/gcs-tess/princer STAGINGVERSION=v999.999.999 BUILD_GCSFUSE_FROM_SOURCE=true && make build-image-and-push-multi-arch REGISTRY=$REGISTRY STAGINGVERSION=$STAGINGVERSION)' | |
| alias reinstall='(cd ${GCS_FUSE_CSI_DRIVER_PATH:-/home/princer_google_com/dev/gcs-fuse-csi-driver} && export REGISTRY=gcr.io/gcs-tess/princer STAGINGVERSION=v999.999.999 && make uninstall && make install)' | |
| alias update-csi='(cd ${GCS_FUSE_CSI_DRIVER_PATH:-/home/princer_google_com/dev/gcs-fuse-csi-driver} && export REGISTRY=gcr.io/gcs-tess/princer STAGINGVERSION=v999.999.999 && kubectl set image daemonset/gcsfusecsi-node gcs-fuse-csi-driver=$REGISTRY/gcs-fuse-csi-driver:$STAGINGVERSION -n gcs-fuse-csi-driver && kubectl set image deployment/gcs-fuse-csi-driver-webhook gcs-fuse-csi-driver-webhook=$REGISTRY/gcs-fuse-csi-driver-webhook:$STAGINGVERSION -n gcs-fuse-csi-driver)' | |
| alias recreate-pod='(cd ${GCS_FUSE_CSI_DRIVER_PATH:-/home/princer_google_com/dev/gcs-fuse-csi-driver} && ./setup-test-pod.sh --skip-setup)' |
1 participant
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: