fix: fixes for issues found during local ut test for happy path for inplace

hasethuraman · hasethuraman · commit 6e9ab030a08b · 2025-09-24T15:18:16.000Z
diff --git a/hack/lib-premiumv2-migration-common.sh b/hack/lib-premiumv2-migration-common.sh
@@ -26,6 +26,10 @@ MIGRATION_FORCE_INPROGRESS_AFTER_MINUTES="${MIGRATION_FORCE_INPROGRESS_AFTER_MIN
 # Maximum PVC size (in GiB) eligible for migration (default 2TiB = 2048GiB). PVCs >= this are skipped.
 MAX_PVC_CAPACITY_GIB="${MAX_PVC_CAPACITY_GIB:-2048}"
 declare -a AUDIT_LINES=()
+MIGRATION_LABEL_KEY="${MIGRATION_LABEL%%=*}"
+MIGRATION_LABEL_VALUE="${MIGRATION_LABEL#*=}"
+BACKUP_ORIGINAL_PVC="${BACKUP_ORIGINAL_PVC:-true}"
+PVC_BACKUP_DIR="${PVC_BACKUP_DIR:-pvc-backups}"
 
 # ---------- Timeouts ----------
 BIND_TIMEOUT_SECONDS="${BIND_TIMEOUT_SECONDS:-60}"
@@ -43,6 +47,8 @@ CREATED_BY_LABEL_KEY="${CREATED_BY_LABEL_KEY:-disk.csi.azure.com/created-by}"
 MIGRATION_TOOL_ID="${MIGRATION_TOOL_ID:-azuredisk-pv1-to-pv2-migrator}"
 MIGRATION_DONE_LABEL_KEY="${MIGRATION_DONE_LABEL_KEY:-disk.csi.azure.com/migration-done}"
 MIGRATION_DONE_LABEL_VALUE="${MIGRATION_DONE_LABEL_VALUE:-true}"
+MIGRATION_INPROGRESS_LABEL_KEY="${MIGRATION_INPROGRESS_LABEL_KEY:-disk.csi.azure.com/migration-inprogress}"
+MIGRATION_INPROGRESS_LABEL_VALUE="${MIGRATION_INPROGRESS_LABEL_VALUE:-true}"
 
 audit_add() {
   [[ "$AUDIT_ENABLE" != "true" ]] && return 0
@@ -288,6 +294,7 @@ metadata:
   name: $csi_pv
   labels:
     ${CREATED_BY_LABEL_KEY}: ${MIGRATION_TOOL_ID}
+    ${MIGRATION_LABEL_KEY}: "${MIGRATION_LABEL_VALUE}"
   annotations:
     pv.kubernetes.io/provisioned-by: disk.csi.azure.com
 spec:
@@ -561,11 +568,15 @@ extract_event_reason() {
   local ns="$1" pvc_name="$2"
   kcmd get events -n "$ns" -o json 2>/dev/null \
     | jq -r --arg name "$pvc_name" '
-        .items
-        | map(select(.involvedObject.name==$name
-                     and (.reason|test("SKUMigration(Completed|Progress|Started)"))))
-        | sort_by(.lastTimestamp)
-        | last?.reason // empty' 2>/dev/null || true
+      [ .items[]
+          | select(.involvedObject.name==$name
+                  and (.reason|test("^SKUMigration(Completed|Progress|Started)$")))
+          | {reason, ts:(.lastTimestamp // .eventTime // .deprecatedLastTimestamp // .metadata.creationTimestamp // "")}
+        ] as $arr
+        | if ($arr|length)>0
+            then ($arr|sort_by(.ts)|last|.reason)
+            else ""
+          end'
 }
 
 populate_pvcs() {
@@ -797,7 +808,7 @@ print_migration_cleanup_report() {
   local failed_header_printed=false
   local any=false
 
-  if [[ "${#MIG_PVCS[@]:-0}" -eq 0 ]]; then
+  if ! (( ${#MIG_PVCS[@]} == 0 )); then
     warn "print_migration_cleanup_report: MIG_PVCS empty (nothing to report)."
     return 0
   fi
diff --git a/hack/premium-to-premiumv2-migrator-dualpvc.sh b/hack/premium-to-premiumv2-migrator-dualpvc.sh
@@ -4,6 +4,7 @@ IFS=$'\n\t'
 
 SCRIPT_START_TS="$(date +'%Y-%m-%dT%H:%M:%S')"
 SCRIPT_START_EPOCH="$(date +%s)"
+MODE=dual
 
 # Declarations
 declare -a MIG_PVCS
@@ -13,6 +14,20 @@ declare -A SC_SET
 declare -a PV2_CREATE_FAILURES
 declare -a PV2_BIND_TIMEOUTS
 
+MIG_PVCS=()
+PREREQ_ISSUES=()
+CONFLICT_ISSUES=()
+PV2_CREATE_FAILURES=()
+PV2_BIND_TIMEOUTS=()
+
+# (after array zeroing, before ensure_no_foreign_conflicts)
+safe_array_len() {
+  local name="$1"
+  # If array not declared (future refactor), return 0 instead of tripping set -u
+  declare -p "$name" &>/dev/null || { echo 0; return; }
+  eval "echo \${#$name[@]}"
+}
+
 SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 . "${SCRIPT_DIR}/lib-premiumv2-migration-common.sh"
 
@@ -33,48 +48,52 @@ populate_pvcs
 ensure_no_foreign_conflicts() {
   info "Checking for pre-existing conflicting objects not created by this tool..."
   if kcmd get volumesnapshotclass "$SNAPSHOT_CLASS" >/dev/null 2>&1; then
-    if ! kcmd get volumesnapshotclass "$SNAPSHOT_CLASS" -o json \
-      | jq -e --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" '.metadata.labels[$k]==$v' >/dev/null; then
+    if ! kcmd get volumesnapshotclass "$SNAPSHOT_CLASS" -o json | jq -e \
+         --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" \
+         '.metadata.labels[$k]==$v' >/dev/null; then
       CONFLICT_ISSUES+=("VolumeSnapshotClass/$SNAPSHOT_CLASS (missing label)")
     fi
   fi
-  for ENTRY in "${MIG_PVCS[@]:-}"; do
+  for ENTRY in "${MIG_PVCS[@]}"; do
     local ns="${ENTRY%%|*}" pvc="${ENTRY##*|}"
-    local pv2_pvc
+    local pv2_pvc pv diskuri int_pvc int_pv snap
     pv2_pvc="$(name_pv2_pvc "$pvc")"
     if kcmd get pvc "$pv2_pvc" -n "$ns" >/dev/null 2>&1; then
-      if ! kcmd get pvc "$pv2_pvc" -n "$ns" -o json \
-        | jq -e --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" '.metadata.labels[$k]==$v' >/dev/null; then
+      if ! kcmd get pvc "$pv2_pvc" -n "$ns" -o json | jq -e \
+           --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" \
+           '.metadata.labels[$k]==$v' >/dev/null; then
         CONFLICT_ISSUES+=("PVC/$ns/$pv2_pvc (pv2) missing label")
       fi
     fi
-    local pv
     pv=$(kcmd get pvc "$pvc" -n "$ns" -o jsonpath='{.spec.volumeName}' 2>/dev/null || true)
     [[ -z "$pv" ]] && continue
-    local diskuri
     diskuri=$(kcmd get pv "$pv" -o jsonpath='{.spec.azureDisk.diskURI}' 2>/dev/null || true)
     if [[ -n "$diskuri" ]]; then
-      local int_pvc int_pv
       int_pvc="$(name_csi_pvc "$pvc")"
       int_pv="$(name_csi_pv "$pv")"
       if kcmd get pvc "$int_pvc" -n "$ns" >/dev/null 2>&1; then
-        kcmd get pvc "$int_pvc" -n "$ns" -o json | jq -e --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" '.metadata.labels[$k]==$v' >/dev/null || \
-          CONFLICT_ISSUES+=("PVC/$ns/$int_pvc (intermediate) missing label")
+        kcmd get pvc "$int_pvc" -n "$ns" -o json | jq -e \
+           --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" \
+           '.metadata.labels[$k]==$v' >/dev/null || \
+           CONFLICT_ISSUES+=("PVC/$ns/$int_pvc (intermediate) missing label")
       fi
       if kcmd get pv "$int_pv" >/dev/null 2>&1; then
-        kcmd get pv "$int_pv" -o json | jq -e --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" '.metadata.labels[$k]==$v' >/dev/null || \
-          CONFLICT_ISSUES+=("PV/$int_pv (intermediate) missing label")
+        kcmd get pv "$int_pv" -o json | jq -e \
+           --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" \
+           '.metadata.labels[$k]==$v' >/dev/null || \
+           CONFLICT_ISSUES+=("PV/$int_pv (intermediate) missing label")
       fi
     fi
-    local snap
     snap="$(name_snapshot "$pv")"
     if kcmd get volumesnapshot "$snap" -n "$ns" >/dev/null 2>&1; then
-      kcmd get volumesnapshot "$snap" -n "$ns" -o json | jq -e --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" '.metadata.labels[$k]==$v' >/dev/null || \
-        CONFLICT_ISSUES+=("VolumeSnapshot/$ns/$snap missing label")
+      kcmd get volumesnapshot "$snap" -n "$ns" -o json | jq -e \
+         --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" \
+         '.metadata.labels[$k]==$v' >/dev/null || \
+         CONFLICT_ISSUES+=("VolumeSnapshot/$ns/$snap missing label")
     fi
   done
-  if (( ${#CONFLICT_ISSUES[@]} > 0 )); then
-    err "Conflict check failed (${#CONFLICT_ISSUES[@]} items)."
+  if (( $(safe_array_len CONFLICT_ISSUES) > 0 )); then
+    err "Conflict check failed ($(safe_array_len CONFLICT_ISSUES) items)."
     printf '  - %s\n' "${CONFLICT_ISSUES[@]}"
     exit 1
   fi
@@ -99,6 +118,7 @@ metadata:
   namespace: $ns
   labels:
     ${CREATED_BY_LABEL_KEY}: ${MIGRATION_TOOL_ID}
+    ${MIGRATION_LABEL_KEY}: "${MIGRATION_LABEL_VALUE}"
 spec:
   accessModes:
   - ReadWriteOnce
@@ -130,14 +150,15 @@ EOF
   return 2
 }
 
-extract_event_reason() { extract_event_reason "$@"; } # use common lib version
-
 # ---------------- Discover Tagged PVCs ----------------
 populate_pvcs
 
 # ------------- Pre-Req & Conflicts -------------
 print_combined_validation_report_and_exit_if_needed() {
-  local prereq_count=${#PREREQ_ISSUES[@]} conflict_count=${#CONFLICT_ISSUES[@]}
+  local prereq_count
+  prereq_count=$(safe_array_len PREREQ_ISSUES)
+  local conflict_count
+  conflict_count=$(safe_array_len CONFLICT_ISSUES)
   if (( prereq_count==0 && conflict_count==0 )); then
     ok "Pre-req & conflict checks passed."
     return
diff --git a/hack/premium-to-premiumv2-migrator-inplace.sh b/hack/premium-to-premiumv2-migrator-inplace.sh
@@ -4,6 +4,7 @@ IFS=$'\n\t'
 
 SCRIPT_START_TS="$(date +'%Y-%m-%dT%H:%M:%S')"
 SCRIPT_START_EPOCH="$(date +%s)"
+MODE=inplace
 
 # In-place rollback keys
 ROLLBACK_PVC_YAML_ANN="${ROLLBACK_PVC_YAML_ANN:-disk.csi.azure.com/rollback-pvc-yaml}"
@@ -16,6 +17,21 @@ declare -a CONFLICT_ISSUES
 declare -A SC_SET
 declare -a ROLLBACK_FAILURES
 
+# Explicit zeroing (defensive; avoids 'unbound variable' under set -u even if declarations are edited later)
+MIG_PVCS=()
+PREREQ_ISSUES=()
+CONFLICT_ISSUES=()
+ROLLBACK_FAILURES=()
+
+# Helper: safe length (avoids accidental nounset trip if future refactor removes a declare)
+safe_array_len() {
+  # usage: safe_array_len arrayName
+  local name="$1"
+  declare -p "$name" &>/dev/null || { echo 0; return; }
+  # indirect expansion
+  eval "echo \${#$name[@]}"
+}
+
 SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 . "${SCRIPT_DIR}/lib-premiumv2-migration-common.sh"
 
@@ -33,39 +49,40 @@ ensure_snapshot_class   # from common lib
 ensure_no_foreign_conflicts() {
   info "Checking for pre-existing conflicting objects not created by this tool..."
   if kcmd get volumesnapshotclass "$SNAPSHOT_CLASS" >/dev/null 2>&1; then
-    if ! kcmd get volumesnapshotclass "$SNAPSHOT_CLASS" -o json \
-      | jq -e --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" '.metadata.labels[$k]==$v' >/dev/null; then
+    if ! kcmd get volumesnapshotclass "$SNAPSHOT_CLASS" -o json | jq -e \
+         --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" \
+         '.metadata.labels[$k]==$v' >/dev/null; then
       CONFLICT_ISSUES+=("VolumeSnapshotClass/$SNAPSHOT_CLASS (missing label)")
     fi
   fi
-  for ENTRY in "${MIG_PVCS[@]:-}"; do
+  for ENTRY in "${MIG_PVCS[@]}"; do
     local ns="${ENTRY%%|*}" pvc="${ENTRY##*|}"
-    local pv
+    local pv snap
     pv=$(kcmd get pvc "$pvc" -n "$ns" -o jsonpath='{.spec.volumeName}' 2>/dev/null || true)
     [[ -z "$pv" ]] && continue
-    local snap
     snap="$(name_snapshot "$pv")"
     if kcmd get volumesnapshot "$snap" -n "$ns" >/dev/null 2>&1; then
-      kcmd get volumesnapshot "$snap" -n "$ns" -o json | jq -e --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" '.metadata.labels[$k]==$v' >/dev/null || \
-        CONFLICT_ISSUES+=("VolumeSnapshot/$ns/$snap missing label")
+      kcmd get volumesnapshot "$snap" -n "$ns" -o json | jq -e \
+         --arg k "$CREATED_BY_LABEL_KEY" --arg v "$MIGRATION_TOOL_ID" \
+         '.metadata.labels[$k]==$v' >/dev/null || \
+         CONFLICT_ISSUES+=("VolumeSnapshot/$ns/$snap missing label")
     fi
   done
-  if (( ${#CONFLICT_ISSUES[@]} > 0 )); then
-    err "Conflict check failed (${#CONFLICT_ISSUES[@]} items)."
+  if (( $(safe_array_len CONFLICT_ISSUES) > 0 )); then
+    err "Conflict check failed ($(safe_array_len CONFLICT_ISSUES) items)."
     printf '  - %s\n' "${CONFLICT_ISSUES[@]}"
     exit 1
   fi
   ok "No conflicting pre-existing objects detected."
 }
 
-extract_event_reason() { extract_event_reason "$@"; } # use common lib version
-
 # ---------------- Discover Tagged PVCs ----------------
 populate_pvcs
 
 # ---------------- Pre-Requisite Validation (mirrors dual script) ----------------
 print_combined_validation_report_and_exit_if_needed() {
-  local prereq_count=${#PREREQ_ISSUES[@]} conflict_count=${#CONFLICT_ISSUES[@]}
+  local prereq_count=$(safe_array_len PREREQ_ISSUES)
+  local conflict_count=$(safe_array_len CONFLICT_ISSUES)
   if (( prereq_count==0 && conflict_count==0 )); then
     ok "Pre-req & conflict checks passed."
     return
@@ -138,6 +155,33 @@ create_pv2_inplace() {
 
   ensure_reclaim_policy_retain "$pv_before"
 
+  # -------- Raw PVC backup (full YAML) prior to deletion --------
+  if [[ "$BACKUP_ORIGINAL_PVC" == "true" ]]; then
+    local ts backup_ns_dir backup_file tmp_file
+    ts="$(date +%Y%m%d-%H%M%S)"
+    backup_ns_dir="${PVC_BACKUP_DIR}/${ns}"
+    mkdir -p "$backup_ns_dir"
+    tmp_file="${backup_ns_dir}/${pvc}-${ts}.yaml.tmp"
+    backup_file="${backup_ns_dir}/${pvc}-${ts}.yaml"
+    info "Backing up original PVC $ns/$pvc to $backup_file"
+    if ! kcmd get pvc "$pvc" -n "$ns" -o yaml >"$tmp_file" 2>/dev/null; then
+      err "Failed to fetch PVC $ns/$pvc for backup; skipping migration of this PVC."
+      audit_add "PersistentVolumeClaim" "$pvc" "$ns" "backup-failed" "kubectl get pvc $pvc -n $ns -o yaml" "dest=$backup_file reason=kubectlError"
+      return 1
+    fi
+    if [[ ! -s "$tmp_file" ]] || ! grep -q '^kind: *PersistentVolumeClaim' "$tmp_file"; then
+      err "Backup validation failed for $ns/$pvc (empty or malformed); skipping migration."
+      rm -f "$tmp_file"
+      audit_add "PersistentVolumeClaim" "$pvc" "$ns" "backup-invalid" "kubectl get pvc $pvc -n $ns -o yaml" "dest=$backup_file reason=validation"
+      return 1
+    fi
+    mv "$tmp_file" "$backup_file"
+    audit_add "PersistentVolumeClaim" "$pvc" "$ns" "backup" "rm -f $backup_file" "dest=$backup_file size=$(wc -c <"$backup_file")B"
+  else
+    info "BACKUP_ORIGINAL_PVC=false -> skipping raw PVC backup for $ns/$pvc"
+  fi
+  # ---------------------------------------------------------------
+
   info "Deleting original PVC $ns/$pvc (PV retained for rollback)"
   kcmd delete pvc "$pvc" -n "$ns" --wait=true
   audit_add "PersistentVolumeClaim" "$pvc" "$ns" "delete" "N/A" "stage=replace retainedPV=${pv_before}"
@@ -158,6 +202,7 @@ metadata:
   namespace: ${ns}
   labels:
     ${CREATED_BY_LABEL_KEY}: ${MIGRATION_TOOL_ID}
+    ${MIGRATION_LABEL_KEY}: "${MIGRATION_LABEL_VALUE}"
   annotations:
     ${ROLLBACK_PVC_YAML_ANN}: "${encoded_spec}"
     ${ROLLBACK_ORIG_PV_ANN}: "${pv_before}"
