AKS: Fix scenarios around nodepool and storagepool name for Azure Con…

…tainer Storage (#6911)

src/aks-preview/HISTORY.rst

@@ -13,6 +13,11 @@ Pending
 +++++++
 * Vendor new SDK and bump API version to 2023-09-02-preview.
 
+0.5.167
++++++++
+* Fix the default storagepool name value created for Azure Container Storage.
+* Ensure the correct nodepool name is picked and labelled by Azure Container Storage while installing with `az aks create`.
+
 0.5.166
 +++++++
 * Add `--network-policy` to the `az aks update` command.

src/aks-preview/azext_aks_preview/azurecontainerstorage/_consts.py

@@ -3,6 +3,8 @@
 # Licensed under the MIT License. See License.txt in the project root for license information.
 # --------------------------------------------------------------------------------------------
 
+CONST_ACSTOR_IO_ENGINE_LABEL_KEY = "acstor.azure.com/io-engine"
+CONST_ACSTOR_IO_ENGINE_LABEL_VAL = "acstor"
 CONST_ACSTOR_K8S_EXTENSION_NAME = "microsoft.azurecontainerstorage"
 CONST_EXT_INSTALLATION_NAME = "azurecontainerstorage"
 CONST_K8S_EXTENSION_CLIENT_FACTORY_MOD_NAME = "azext_k8s_extension._client_factory"
@@ -24,5 +26,4 @@
 CONST_STORAGE_POOL_TYPE_ELASTIC_SAN = "elasticSan"
 CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK = "ephemeralDisk"
 
-CONST_STORAGE_POOL_RANDOM_LENGTH = 7
 RP_REGISTRATION_POLLING_INTERVAL_IN_SEC = 5

src/aks-preview/azext_aks_preview/azurecontainerstorage/_helpers.py

@@ -18,16 +18,13 @@
     CONST_K8S_EXTENSION_NAME,
     CONST_STORAGE_POOL_NAME_PREFIX,
     CONST_STORAGE_POOL_OPTION_NVME,
-    CONST_STORAGE_POOL_RANDOM_LENGTH,
     CONST_STORAGE_POOL_TYPE_ELASTIC_SAN,
     CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK,
     RP_REGISTRATION_POLLING_INTERVAL_IN_SEC,
 )
 
 from datetime import datetime
 from knack.log import get_logger
-import random
-import string
 import time
 
 logger = get_logger(__name__)
@@ -150,11 +147,6 @@ def perform_role_operations_on_managed_rg(cmd, subscription_id, node_resource_gr
     return False
 
 
-def generate_random_storage_pool_name():
-    random_name = CONST_STORAGE_POOL_NAME_PREFIX + ''.join(random.choices(string.ascii_lowercase, k=CONST_STORAGE_POOL_RANDOM_LENGTH))
-    return random_name
-
-
 def get_k8s_extension_module(module_name):
     try:
         # adding the installed extension in the path

src/aks-preview/azext_aks_preview/azurecontainerstorage/_validators.py

@@ -29,19 +29,6 @@
 logger = get_logger(__name__)
 
 
-def validate_nodepool_names_with_cluster_nodepools(nodepool_names, agentpool_details):
-    nodepool_list = nodepool_names.split(',')
-    for nodepool in nodepool_list:
-        if nodepool not in agentpool_details:
-            raise InvalidArgumentValueError(
-                'Nodepool: {} not found. '
-                'Please provide existing nodepool names in --azure-container-storage-nodepools.'
-                '\nUse command `az nodepool list` to get the list of nodepools in the cluster.'
-                '\nAborting installation of Azure Container Storage.'
-                .format(nodepool)
-            )
-
-
 def validate_azure_container_storage_params(
     enable_azure_container_storage,
     disable_azure_container_storage,
@@ -51,6 +38,8 @@ def validate_azure_container_storage_params(
     storage_pool_option,
     storage_pool_size,
     nodepool_list,
+    agentpool_names,
+    is_extension_installed,
 ):
     if enable_azure_container_storage and disable_azure_container_storage:
         raise MutuallyExclusiveArgumentError(
@@ -65,6 +54,7 @@ def validate_azure_container_storage_params(
             storage_pool_option,
             storage_pool_size,
             nodepool_list,
+            is_extension_installed,
         )
 
     elif enable_azure_container_storage:
@@ -74,16 +64,27 @@ def validate_azure_container_storage_params(
             storage_pool_sku,
             storage_pool_option,
             storage_pool_size,
+            is_extension_installed
         )
 
+        _validate_nodepool_names(nodepool_list, agentpool_names)
+
 
 def _validate_disable_azure_container_storage_params(
     storage_pool_name,
     storage_pool_sku,
     storage_pool_option,
     storage_pool_size,
     nodepool_list,
+    is_extension_installed,
 ):
+    if not is_extension_installed:
+        raise InvalidArgumentValueError(
+            'Invalid usage of --disable-azure-container-storage. '
+            'Azure Container Storage is not enabled on the cluster. '
+            'Aborting disabling of Azure Container Storage.'
+        )
+
     if storage_pool_name is not None:
         raise MutuallyExclusiveArgumentError(
             'Conflicting flags. Cannot define --storage-pool-name value '
@@ -121,7 +122,15 @@ def _validate_enable_azure_container_storage_params(
     storage_pool_sku,
     storage_pool_option,
     storage_pool_size,
+    is_extension_installed,
 ):
+    if is_extension_installed:
+        raise InvalidArgumentValueError(
+            'Invalid usage of --enable-azure-container-storage. '
+            'Azure Container Storage is already enabled on the cluster. '
+            'Aborting installation of Azure Container Storage.'
+        )
+
     if storage_pool_name is not None:
         pattern = r'[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*'
         is_pool_name_valid = re.fullmatch(pattern, storage_pool_name)
@@ -181,3 +190,39 @@ def _validate_enable_azure_container_storage_params(
                     'Storage pools using Ephemeral disk use all capacity available on the local device. '
                     ' --storage-pool-size will be ignored.'
                 )
+
+
+def _validate_nodepool_names(nodepool_names, agentpool_details):
+    # Validate that nodepool_list is a comma separated string
+    # consisting of valid nodepool names i.e. lower alphanumeric
+    # characters and the first character should be lowercase letter.
+    pattern = r'^[a-z][a-z0-9]*(?:,[a-z][a-z0-9]*)*$'
+    if re.fullmatch(pattern, nodepool_names) is None:
+        raise InvalidArgumentValueError(
+            "Invalid --azure-container-storage-nodepools value. "
+            "Accepted value is a comma separated string of valid nodepool "
+            "names without any spaces.\nA valid nodepool name may only contain lowercase "
+            "alphanumeric characters and must begin with a lowercase letter."
+        )
+
+    nodepool_list = nodepool_names.split(',')
+    for nodepool in nodepool_list:
+        if nodepool not in agentpool_details:
+            if len(agentpool_details) > 1:
+                raise InvalidArgumentValueError(
+                    'Nodepool: {0} not found. '
+                    'Please provide a comma separated string of existing nodepool names '
+                    'in --azure-container-storage-nodepools.'
+                    '\nNodepools available in the cluster are: {1}.'
+                    '\nAborting installation of Azure Container Storage.'
+                    .format(nodepool, ', '.join(agentpool_details))
+                )
+            else:
+                raise InvalidArgumentValueError(
+                    'Nodepool: {0} not found. '
+                    'Please provide a comma separated string of existing nodepool names '
+                    'in --azure-container-storage-nodepools.'
+                    '\nNodepool available in the cluster is: {1}.'
+                    '\nAborting installation of Azure Container Storage.'
+                    .format(nodepool, agentpool_details[0])
+                )

src/aks-preview/azext_aks_preview/azurecontainerstorage/acstor_ops.py

@@ -21,13 +21,11 @@
 )
 from azext_aks_preview.azurecontainerstorage._helpers import (
     check_if_extension_is_installed,
-    generate_random_storage_pool_name,
     get_k8s_extension_module,
     perform_role_operations_on_managed_rg,
     register_dependent_rps,
     should_create_storagepool,
 )
-from azext_aks_preview.azurecontainerstorage._validators import validate_nodepool_names_with_cluster_nodepools
 from knack.log import get_logger
 from knack.prompting import prompt_y_n
 
@@ -55,15 +53,6 @@ def perform_enable_azure_container_storage(
     if not register_dependent_rps(cmd, subscription_id):
         return
 
-    # Step 2: Check if extension already installed incase of an update call
-    if not is_cluster_create and check_if_extension_is_installed(cmd, resource_group, cluster_name):
-        logger.error(
-            "Extension type {0} already installed on cluster."
-            "\nAborting installation of Azure Container Storage."
-            .format(CONST_ACSTOR_K8S_EXTENSION_NAME)
-        )
-        return
-
     if nodepool_names is None:
         nodepool_names = "nodepool1"
     if storage_pool_type == CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK:
@@ -72,19 +61,17 @@ def perform_enable_azure_container_storage(
         if storage_pool_option == CONST_STORAGE_POOL_OPTION_SSD:
             storage_pool_option = "temp"
 
-    validate_nodepool_names_with_cluster_nodepools(nodepool_names, agentpool_details)
-
-    # Step 3: Validate if storagepool should be created.
+    # Step 2: Validate if storagepool should be created.
     # Depends on the following:
-    #   3a: Grant AKS cluster's node identity the following
+    #   2a: Grant AKS cluster's node identity the following
     #       roles on the AKS managed resource group:
     #       1. Reader
     #       2. Network Contributor
     #       3. Elastic SAN Owner
     #       4. Elastic SAN Volume Group Owner
     #       Ensure grant was successful if creation of
     #       Elastic SAN storagepool is requested.
-    #   3b: Ensure Ls series nodepool is present if creation
+    #   2b: Ensure Ls series nodepool is present if creation
     #       of Ephemeral NVMe Disk storagepool is requested.
     create_storage_pool = should_create_storagepool(
         cmd,
@@ -97,11 +84,12 @@ def perform_enable_azure_container_storage(
         nodepool_names,
     )
 
-    # Step 4: Configure the storagepool parameters
-    config_settings = []
+    # Step 3: Configure the storagepool parameters
+    # Setting nodepools as empty since CLI is handling nodepool labels
+    config_settings = [{"cli.node.nodepools": ""}]
     if create_storage_pool:
         if storage_pool_name is None:
-            storage_pool_name = generate_random_storage_pool_name()
+            storage_pool_name = storage_pool_type.lower()
         if storage_pool_size is None:
             storage_pool_size = CONST_STORAGE_POOL_DEFAULT_SIZE_ESAN if \
                 storage_pool_type == CONST_STORAGE_POOL_TYPE_ELASTIC_SAN else \
@@ -112,7 +100,6 @@ def perform_enable_azure_container_storage(
                 {"cli.storagePool.name": storage_pool_name},
                 {"cli.storagePool.size": storage_pool_size},
                 {"cli.storagePool.type": storage_pool_type},
-                {"cli.node.nodepools": nodepool_names},
             ]
         )
 
@@ -173,21 +160,19 @@ def perform_disable_azure_container_storage(
     kubelet_identity_object_id,
     perform_validation,
 ):
-    # Step 1: Check if show_k8s_extension returns an extension already installed
-    if not check_if_extension_is_installed(cmd, resource_group, cluster_name):
-        raise UnknownError(
-            "Extension type {0} not installed on cluster."
-            "\nAborting disabling of Azure Container Storage."
-            .format(CONST_ACSTOR_K8S_EXTENSION_NAME)
-        )
-
     client_factory = get_k8s_extension_module(CONST_K8S_EXTENSION_CLIENT_FACTORY_MOD_NAME)
     client = client_factory.cf_k8s_extension_operation(cmd.cli_ctx)
     k8s_extension_custom_mod = get_k8s_extension_module(CONST_K8S_EXTENSION_CUSTOM_MOD_NAME)
     no_wait_delete_op = False
-    # Step 2: Perform validation if accepted by user
+    # Step 1: Perform validation if accepted by user
     if perform_validation:
-        config_settings = [{"cli.storagePool.uninstallValidation": True}]
+        # Set cli.node.nodepools to empty to ensure
+        # no post uninstall label removal takes place
+        # since we have added the labels to the nodepool.
+        config_settings = [
+            {"cli.storagePool.uninstallValidation": True},
+            {"cli.node.nodepools": ""},
+        ]
         try:
             update_result = k8s_extension_custom_mod.update_k8s_extension(
                 cmd,
@@ -233,7 +218,7 @@ def perform_disable_azure_container_storage(
             else:
                 raise UnknownError("Validation failed. Unable to disable Azure Container Storage. Reseting cluster state.")
 
-    # Step 3: If the extension is installed and validation succeeded or skipped, call delete_k8s_extension
+    # Step 2: If the extension is installed and validation succeeded or skipped, call delete_k8s_extension
     try:
         delete_op_result = k8s_extension_custom_mod.delete_k8s_extension(
             cmd,
@@ -253,7 +238,7 @@ def perform_disable_azure_container_storage(
 
     logger.warning("Azure Container Storage has been disabled.")
 
-    # Step 4: Revoke AKS cluster's node identity the following
+    # Step 3: Revoke AKS cluster's node identity the following
     # roles on the AKS managed resource group:
     # 1. Reader
     # 2. Network Contributor