xpk storage detach (#420)

* drop manifest requirement when attaching gcsfuse storage

* docs

* rename `storage delete` to `storage detach

* do not require manifest in fuse workflow tests

* fix: redundant --cluster parameter

* change 'user' to 'you'

* unify gcsfuse template generation algo to match filestore

* gcsfuse storage attach add missing size in docs

* dromp manifest requirement for attaching filestore storage

* remove obsolete `--manifest` param

* fix: linting and typing

* fix: linting

* separate filestore instance name from storage volume names

* fix: typo

* minor fixes

* remove obsolete error messages

* docs

* fix: readme inconsistencies

* fix: add project id to storage manifest file path

* refactor create_storage_crds

* use Availability.ZONAL instead of hardcoded string value

* move loading filestore location logic to load_location function

* fix: fuse test messages

* fix tests: always remove fuse storage after failing tests

* filestore delete

* fix error caused by same storage names

* checking if filestore has attached storages before deletion

* fix: bugs in filestore name passing

* xpk storage delete docs

* remove filestore deletion at the end of tests, because it is now handled by xpk storage delete

* missing kind arguments for storage delete

* fix: func name typo from merging

* fix: typo in func name from merging (again)

* add loading instance when prompted for fullname

* add verification of filestore deletion to tests

* automatic detach when deleting filestore, user prompt

* change filestore delete msg

Author: gcie

.github/workflows/reusable_storage_tests_by_type.yaml

@@ -185,12 +185,16 @@ jobs:
       run: python3 xpk.py workload list --cluster ${{inputs.cluster-name}} --zone=${{inputs.zone}} --wait-for-job-completion $FS_DELETE_WORKLOAD --timeout 300
     - name: Delete the delete workload on the cluster
       run: python3 xpk.py workload delete --workload $FS_DELETE_WORKLOAD --cluster ${{inputs.cluster-name}} --zone=${{inputs.zone}}
-    - name: Delete storage
+    - name: Detach storage volumes
       if: always()
-      run: python3 xpk.py storage delete ${{secrets.STORAGE_NAME}} --zone=${{inputs.zone}} --cluster=${{inputs.cluster-name}}
+      run: python3 xpk.py storage detach ${{secrets.STORAGE_NAME}} --cluster=${{inputs.cluster-name}} --zone=${{inputs.zone}}
     - name: Verify VolumeBundle deleted
       run: |
         ! kubectl get volumebundle | grep ${{secrets.STORAGE_NAME}}
-    - name: Delete the filestore instance
-      if: always() && inputs.storage-command == 'create'
-      run: gcloud filestore instances delete ${{secrets.STORAGE_NAME}} --zone=${{inputs.zone}} --force
+    - name: Delete GCP Filestore Storage instance
+      if: always() && inputs.storage-command == 'create' && inputs.storage-type == 'gcpfilestore'
+      run: python3 xpk.py storage delete ${{secrets.STORAGE_NAME}} --cluster=${{inputs.cluster-name}} --zone=${{inputs.zone}}
+    - name: Verify deletion of GCP Filestore Storage instance
+      if: inputs.storage-command == 'create' && inputs.storage-type == 'gcpfilestore'
+      run: |
+        ! gcloud filestore instances list | grep ${{secrets.STORAGE_NAME}}

README.md

@@ -520,10 +520,20 @@ python3 xpk.py workload create \
   --project=$PROJECT --cluster=$CLUSTER --zone=$ZONE \
   --tpu-type=v5litepod-16 --storage=test-storage
 ```
+
+### Detaching storage
+
+```shell
+python3 xpk.py storage detach $STORAGE_NAME \
+  --project=$PROJECT --cluster=$CLUSTER --zone=$ZONE
+```
+
 ### Deleting storage
 
+XPK allows you to remove Filestore instances easily with `xpk storage delete` command. **Warning:** this deletes all data contained in the Filestore!
+
 ```shell
-python3 xpk.py storage delete $STORAGE_NAME \
+python3 xpk.py storage delete test-fs-instance \
   --project=$PROJECT --cluster=$CLUSTER --zone=$ZONE
 ```
 

src/xpk/commands/storage.py

@@ -17,6 +17,7 @@
 from argparse import Namespace
 
 from kubernetes import client as k8s_client
+from kubernetes.client import ApiClient
 from kubernetes.client.rest import ApiException
 
 from ..core import gcsfuse
@@ -42,26 +43,30 @@
     STORAGE_CRD_PLURAL,
     XPK_API_GROUP_NAME,
     XPK_API_GROUP_VERSION,
+    Storage,
     create_storage_crds,
     get_storage,
     list_storages,
     print_storages_for_cluster,
 )
-from ..utils.console import xpk_exit, xpk_print
+from ..utils.console import get_user_input, xpk_exit, xpk_print
 from ..utils.kubectl import apply_kubectl_manifest
 
 
 def storage_create(args: Namespace) -> None:
   add_zone_and_project(args)
   if args.type == GCP_FILESTORE_TYPE:
-    filestore_client = FilestoreClient(args.zone, args.name, args.project)
+    if args.instance is None:
+      args.instance = args.name
+
+    filestore_client = FilestoreClient(args.zone, args.instance, args.project)
     filestore_exists = filestore_client.check_instance_exists()
     if filestore_exists:
-      xpk_print(f"Filestore instance {args.name} already exists.")
+      xpk_print(f"Filestore instance {args.instance} already exists.")
       xpk_exit(1)
     filestore_network = get_cluster_network(args)
     xpk_print(
-        f"Creating Filestore instance {args.name} in network:"
+        f"Creating Filestore instance {args.instance} in network:"
         f" {filestore_network}"
     )
     filestore_client.create_instance(
@@ -85,6 +90,40 @@ def storage_create(args: Namespace) -> None:
     apply_kubectl_manifest(k8s_api_client, manifest)
 
 
+def storage_delete(args: Namespace) -> None:
+  add_zone_and_project(args)
+  k8s_api_client = setup_k8s_env(args)
+  storages = list_storages(k8s_api_client)
+  filestore_client = FilestoreClient(args.zone, args.name, args.project)
+
+  if not filestore_client.check_instance_exists():
+    xpk_print(f"Filestore instance {args.name} does not exist.")
+    xpk_exit(1)
+
+  filestore_instance_name = filestore_client.get_instance_fullname()
+
+  children = [
+      storage
+      for storage in storages
+      if storage.bucket.startswith(filestore_instance_name)
+  ]
+
+  if children and not args.force:
+    detach = get_user_input(
+        "Deleting a filestore storage will destroy your filestore instance and"
+        " all its data in all volumes will be lost. Do you wish to delete the"
+        f" filestore instance {filestore_instance_name}?\n y (yes) / n (no):\n'"
+    )
+    if not detach:
+      xpk_print("Deleting storage canceled.")
+      xpk_exit(0)
+
+  for child in children:
+    delete_storage_resources(k8s_api_client, child)
+
+  filestore_client.delete_filestore_instance()
+
+
 def storage_attach(args: Namespace) -> None:
   add_zone_and_project(args)
   if args.type == GCP_FILESTORE_TYPE:
@@ -142,6 +181,12 @@ def storage_list(args: Namespace) -> None:
   print_storages_for_cluster(storages)
 
 
+def storage_detach(args: Namespace) -> None:
+  k8s_api_client = setup_k8s_env(args)
+  storage = get_storage(k8s_api_client, args.name)
+  delete_storage_resources(k8s_api_client, storage)
+
+
 def delete_resource(api_call, resource_name: str, resource_kind: str) -> None:
   """
   Deletes a Kubernetes resource and handles potential API exceptions.
@@ -167,27 +212,34 @@ def delete_resource(api_call, resource_name: str, resource_kind: str) -> None:
   xpk_print(f"Deleted {resource_kind}:{resource_name}")
 
 
-def storage_delete(args: Namespace) -> None:
-  k8s_api_client = setup_k8s_env(args)
+def delete_storage_resources(k8s_api_client: ApiClient, storage: Storage):
+  """
+  Deletes storage PV, PVC, SC and custom resources (if they exist).
+
+  Args:
+    k8s_api_client: An ApiClient object for interacting with the Kubernetes API.
+    storage: Storage to delete
+  """
   api_instance = k8s_client.CustomObjectsApi(k8s_api_client)
   core_api = k8s_client.CoreV1Api()
   storage_api = k8s_client.StorageV1Api()
-  storage = get_storage(k8s_api_client, args.name)
+
   delete_resource(
       lambda name: core_api.delete_namespaced_persistent_volume_claim(
           name, "default"
       ),
       storage.pvc,
       "Persistent Volume Claim",
   )
+
   delete_resource(
       core_api.delete_persistent_volume, storage.pv, "Persistent Volume"
   )
 
   if storage.type == GCP_FILESTORE_TYPE:
     delete_resource(
         storage_api.delete_storage_class,
-        get_storage_class_name(args.name),
+        get_storage_class_name(storage.name),
         "Storage Class",
     )
 
@@ -199,7 +251,7 @@ def storage_delete(args: Namespace) -> None:
           version=KJOB_API_GROUP_VERSION,
           plural=KJOB_API_VOLUME_BUNDLE_PLURAL,
       ),
-      args.name,
+      storage.name,
       "VolumeBundle",
   )
 
@@ -210,6 +262,6 @@ def storage_delete(args: Namespace) -> None:
           version=XPK_API_GROUP_VERSION,
           plural=STORAGE_CRD_PLURAL,
       ),
-      args.name,
+      storage.name,
       "Storage",
   )

src/xpk/core/filestore.py

@@ -110,6 +110,7 @@ def load_instance(self) -> None:
 
   def get_instance_location(self) -> str:
     """Get Filestore instance's location"""
+    self.load_instance()
     return str(self.instance.name.split("/")[3])
 
   def create_instance(
@@ -174,6 +175,21 @@ def create_instance(
         f"Filestore instance {self.get_instance_fullname(location)} created"
     )
 
+  def delete_filestore_instance(self):
+    # Initialize request
+    name = self.get_instance_fullname()
+    request = filestore_v1.DeleteInstanceRequest(name=name)
+
+    # Make the request
+    operation = self._client.delete_instance(request)
+    xpk_print("Waiting for filestore deletion to complete...")
+    try:
+      operation.result()
+    except GoogleCloudError as e:
+      xpk_print(f"Error while deleting Filestore instance: {e}")
+      xpk_exit(1)
+    xpk_print(f"Filestore instance {name} deleted")
+
   def create_sc(self, name: str, network: str) -> dict:
     """Create a yaml representing filestore StorageClass."""
     data = templates.load(FS_SC_PATH)

src/xpk/parser/storage.py

@@ -20,6 +20,7 @@
     storage_attach,
     storage_create,
     storage_delete,
+    storage_detach,
     storage_list,
 )
 from .common import (
@@ -40,8 +41,9 @@ def set_storage_parser(storage_parser: argparse.ArgumentParser) -> None:
   )
   add_storage_attach_parser(storage_subcommands)
   add_storage_list_parser(storage_subcommands)
-  add_storage_delete_parser(storage_subcommands)
+  add_storage_detach_parser(storage_subcommands)
   add_storage_create_parser(storage_subcommands)
+  add_storage_delete_parser(storage_subcommands)
 
 
 def add_storage_attach_parser(
@@ -199,6 +201,14 @@ def add_storage_create_parser(
       'Optional Arguments',
       'Optional arguments for storage create.',
   )
+  opt_args.add_argument(
+      '--instance',
+      type=str,
+      help=(
+          '(optional) Name of the filestore instance. If not set, then the'
+          ' "name" parameter is infered as an instance name.'
+      ),
+  )
   opt_args.add_argument(
       '--vol',
       type=str,
@@ -250,6 +260,31 @@ def add_storage_list_parser(
   )
 
 
+def add_storage_detach_parser(
+    storage_subcommands_parser: argparse.ArgumentParser,
+):
+  storage_detach_parser: argparse.ArgumentParser = (
+      storage_subcommands_parser.add_parser(
+          'detach', help='Detach XPK Storage.'
+      )
+  )
+  storage_detach_parser.set_defaults(func=storage_detach)
+  add_shared_arguments(storage_detach_parser)
+
+  req_args = storage_detach_parser.add_argument_group(
+      'Required Arguments',
+      'Arguments required for storage detach.',
+  )
+  req_args.add_argument('name', type=str)
+  add_cluster_arguments(req_args, required=True)
+
+  opt_args = storage_detach_parser.add_argument_group(
+      'Optional Arguments',
+      'Optional arguments for storage delete.',
+  )
+  add_kind_cluster_arguments(opt_args)
+
+
 def add_storage_delete_parser(
     storage_subcommands_parser: argparse.ArgumentParser,
 ):
@@ -272,4 +307,10 @@ def add_storage_delete_parser(
       'Optional Arguments',
       'Optional arguments for storage delete.',
   )
+  opt_args.add_argument(
+      '--force',
+      '-f',
+      action='store_true',
+      help='Force filestore instance deletion even if it has attached storages',
+  )
   add_kind_cluster_arguments(opt_args)