Skip to content

Commit

Permalink
{AKS} Fix azdev style issues for azurecontainerstorage (Azure#7118)
Browse files Browse the repository at this point in the history
  • Loading branch information
@FumingZhang
FumingZhang authored Dec 29, 2023
1 parent 4eefcfc commit 7e6e5c0
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 78 deletions.
67 changes: 42 additions & 25 deletions src/aks-preview/azext_aks_preview/azurecontainerstorage/_helpers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,42 @@
# Licensed under the MIT License. See License.txt in the project root for license information.
# --------------------------------------------------------------------------------------------

from azure.cli.core.azclierror import UnknownError
from azure.cli.command_modules.acs._roleassignments import (
add_role_assignment,
build_role_scope,
delete_role_assignments,
)
import time
from datetime import datetime

from azext_aks_preview._client_factory import get_providers_client_factory
from azext_aks_preview.azurecontainerstorage._consts import (
CONST_ACSTOR_K8S_EXTENSION_NAME,
CONST_EXT_INSTALLATION_NAME,
CONST_K8S_EXTENSION_CLIENT_FACTORY_MOD_NAME,
CONST_K8S_EXTENSION_CUSTOM_MOD_NAME,
CONST_K8S_EXTENSION_NAME,
CONST_STORAGE_POOL_NAME_PREFIX,
CONST_STORAGE_POOL_OPTION_NVME,
CONST_STORAGE_POOL_TYPE_ELASTIC_SAN,
CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK,
RP_REGISTRATION_POLLING_INTERVAL_IN_SEC,
)

from datetime import datetime
from azure.cli.command_modules.acs._roleassignments import (
add_role_assignment,
build_role_scope,
delete_role_assignments,
)
from azure.cli.core.azclierror import UnknownError
from knack.log import get_logger
import time

logger = get_logger(__name__)


def register_dependent_rps(cmd, subscription_id) -> bool:
required_rp = 'Microsoft.KubernetesConfiguration'
from azure.mgmt.resource.resources.models import ProviderRegistrationRequest, ProviderConsentDefinition
from azure.mgmt.resource.resources.models import (
ProviderConsentDefinition, ProviderRegistrationRequest)

properties = ProviderRegistrationRequest(third_party_provider_consent=ProviderConsentDefinition(consent_to_authorization=False))
properties = ProviderRegistrationRequest(
third_party_provider_consent=ProviderConsentDefinition(
consent_to_authorization=False
)
)
client = get_providers_client_factory(cmd.cli_ctx)
is_registered = False
try:
Expand All @@ -50,16 +54,17 @@ def register_dependent_rps(cmd, subscription_id) -> bool:
is_registered = _is_rp_registered(cmd, required_rp, subscription_id)
time.sleep(RP_REGISTRATION_POLLING_INTERVAL_IN_SEC)
if (datetime.utcnow() - start).seconds >= timeout_secs:
logger.error("Timed out while waiting for the {0} resource provider to be registered.".format(required_rp))
logger.error(
"Timed out while waiting for the %s resource provider to be registered.", required_rp
)
break

except Exception as e:
except Exception as e: # pylint: disable=broad-except
logger.error(
"Installation of Azure Container Storage requires registering to the following resource provider: {0}. "
"We were unable to perform the registration on your behalf due to the following error: {1}\n"
"Please check with your admin on permissions, "
"or try running registration manually with: `az provider register --namespace {0}` command."
.format(required_rp, e.msg)
"Installation of Azure Container Storage requires registering to the following resource provider: %s. "
"We were unable to perform the registration on your behalf due to the following error: %s\n"
"Please check with your admin on permissions, or try running registration manually with: "
"`az provider register --namespace %s` command.", required_rp, e, required_rp
)

return is_registered
Expand All @@ -75,7 +80,13 @@ def should_create_storagepool(
agentpool_details,
nodepool_name,
):
role_assignment_success = perform_role_operations_on_managed_rg(cmd, subscription_id, node_resource_group, kubelet_identity_object_id, True)
role_assignment_success = perform_role_operations_on_managed_rg(
cmd,
subscription_id,
node_resource_group,
kubelet_identity_object_id,
True
)
return_val = True

if not role_assignment_success:
Expand Down Expand Up @@ -108,7 +119,13 @@ def should_create_storagepool(
return return_val


def perform_role_operations_on_managed_rg(cmd, subscription_id, node_resource_group, kubelet_identity_object_id, assign):
def perform_role_operations_on_managed_rg(
cmd,
subscription_id,
node_resource_group,
kubelet_identity_object_id,
assign
):
managed_rg_role_scope = build_role_scope(node_resource_group, None, subscription_id)
roles = ["Reader", "Network Contributor", "Elastic SAN Owner", "Elastic SAN Volume Group Owner"]
result = True
Expand Down Expand Up @@ -136,7 +153,7 @@ def perform_role_operations_on_managed_rg(cmd, subscription_id, node_resource_gr

if not result:
break
except Exception as ex:
except Exception: # pylint: disable=broad-except
break
else:
return True
Expand All @@ -156,8 +173,8 @@ def get_k8s_extension_module(module_name):
from importlib import import_module
azext_custom = import_module(module_name)
return azext_custom
except ImportError as ie:
raise UnknownError(
except ImportError:
raise UnknownError( # pylint: disable=raise-missing-from
"Please add CLI extension `k8s-extension` for performing Azure Container Storage operations.\n"
"Run command `az extension add --name k8s-extension`"
)
Expand All @@ -180,7 +197,7 @@ def check_if_extension_is_installed(cmd, resource_group, cluster_name) -> bool:
extension_type = extension.extension_type.lower()
if extension_type != CONST_ACSTOR_K8S_EXTENSION_NAME:
return_val = False
except:
except: # pylint: disable=bare-except
return_val = False

return return_val
Expand Down
83 changes: 41 additions & 42 deletions src/aks-preview/azext_aks_preview/azurecontainerstorage/_validators.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,22 @@
# Licensed under the MIT License. See License.txt in the project root for license information.
# --------------------------------------------------------------------------------------------

import re

from azext_aks_preview.azurecontainerstorage._consts import (
CONST_STORAGE_POOL_OPTION_SSD,
CONST_STORAGE_POOL_SKU_PREMIUM_LRS,
CONST_STORAGE_POOL_SKU_PREMIUM_ZRS,
CONST_STORAGE_POOL_TYPE_AZURE_DISK,
CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK,
CONST_STORAGE_POOL_TYPE_ELASTIC_SAN,
CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK,
)

from azure.cli.core.azclierror import (
ArgumentUsageError,
InvalidArgumentValueError,
MutuallyExclusiveArgumentError,
)

from knack.log import get_logger
import re


elastic_san_supported_skus = [
CONST_STORAGE_POOL_SKU_PREMIUM_LRS,
Expand Down Expand Up @@ -142,20 +141,25 @@ def _validate_enable_azure_container_storage_params(

if storage_pool_sku is not None:
if storage_pool_type == CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK:
raise ArgumentUsageError('Cannot set --storage-pool-sku when --enable-azure-container-storage is ephemeralDisk.')
elif storage_pool_type == CONST_STORAGE_POOL_TYPE_ELASTIC_SAN and \
storage_pool_sku not in elastic_san_supported_skus:
raise ArgumentUsageError(
'Cannot set --storage-pool-sku when --enable-azure-container-storage is ephemeralDisk.'
)
if (
storage_pool_type == CONST_STORAGE_POOL_TYPE_ELASTIC_SAN and
storage_pool_sku not in elastic_san_supported_skus
):
supported_skus_str = ", ".join(elastic_san_supported_skus)
raise ArgumentUsageError(
'Invalid --storage-pool-sku value. '
'Supported value for --storage-pool-sku are {0} '
f'Supported value for --storage-pool-sku are {supported_skus_str} '
'when --enable-azure-container-storage is set to elasticSan.'
.format(supported_skus_str)
)

if storage_pool_type != CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK and \
storage_pool_option is not None:
raise ArgumentUsageError('Cannot set --storage-pool-option when --enable-azure-container-storage is not ephemeralDisk.')
raise ArgumentUsageError(
'Cannot set --storage-pool-option when --enable-azure-container-storage is not ephemeralDisk.'
)

if storage_pool_type == CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK and \
storage_pool_option == CONST_STORAGE_POOL_OPTION_SSD:
Expand All @@ -171,25 +175,23 @@ def _validate_enable_azure_container_storage_params(
'Value for --storage-pool-size should be defined '
'with size followed by Gi or Ti e.g. 512Gi or 2Ti.'
)

else:
if storage_pool_type == CONST_STORAGE_POOL_TYPE_ELASTIC_SAN:
pool_size_qty = float(storage_pool_size[:-2])
pool_size_unit = storage_pool_size[-2:]

if (
(pool_size_unit == "Gi" and pool_size_qty < 1024) or
(pool_size_unit == "Ti" and pool_size_qty < 1)
):
raise ArgumentUsageError(
'Value for --storage-pool-size must be at least 1Ti when '
'--enable-azure-container-storage is elasticSan.')

elif storage_pool_type == CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK:
logger.warning(
'Storage pools using Ephemeral disk use all capacity available on the local device. '
' --storage-pool-size will be ignored.'
)
if storage_pool_type == CONST_STORAGE_POOL_TYPE_ELASTIC_SAN:
pool_size_qty = float(storage_pool_size[:-2])
pool_size_unit = storage_pool_size[-2:]

if (
(pool_size_unit == "Gi" and pool_size_qty < 1024) or
(pool_size_unit == "Ti" and pool_size_qty < 1)
):
raise ArgumentUsageError(
'Value for --storage-pool-size must be at least 1Ti when '
'--enable-azure-container-storage is elasticSan.')

elif storage_pool_type == CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK:
logger.warning(
'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):
Expand All @@ -210,19 +212,16 @@ def _validate_nodepool_names(nodepool_names, agentpool_details):
if nodepool not in agentpool_details:
if len(agentpool_details) > 1:
raise InvalidArgumentValueError(
'Nodepool: {0} not found. '
f'Nodepool: {nodepool} not found. '
'Please provide a comma separated string of existing nodepool names '
'in --azure-container-storage-nodepools.'
'\nNodepools available in the cluster are: {1}.'
f"\nNodepools available in the cluster are: {', '.join(agentpool_details)}."
'\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])
)
raise InvalidArgumentValueError(
f'Nodepool: {nodepool} not found. '
'Please provide a comma separated string of existing nodepool names '
'in --azure-container-storage-nodepools.'
f'\nNodepool available in the cluster is: {agentpool_details[0]}.'
'\nAborting installation of Azure Container Storage.'
)
23 changes: 12 additions & 11 deletions src/aks-preview/azext_aks_preview/azurecontainerstorage/acstor_ops.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,12 @@
CONST_STORAGE_POOL_TYPE_EPHEMERAL_DISK,
)
from azext_aks_preview.azurecontainerstorage._helpers import (
check_if_extension_is_installed,
get_k8s_extension_module,
perform_role_operations_on_managed_rg,
register_dependent_rps,
should_create_storagepool,
)
from knack.log import get_logger
from knack.prompting import prompt_y_n

logger = get_logger(__name__)

Expand Down Expand Up @@ -139,16 +137,16 @@ def perform_enable_azure_container_storage(
create_result = LongRunningOperation(cmd.cli_ctx)(result)
if create_result.provisioning_state == "Succeeded":
logger.warning("Azure Container Storage successfully installed.")
except Exception as ex:
except Exception as ex: # pylint: disable=broad-except
if is_cluster_create:
logger.error("Azure Container Storage failed to install.\nError: {0}".format(ex.message))
logger.error("Azure Container Storage failed to install.\nError: %s", ex)
logger.warning(
"AKS cluster is created. "
"Please run `az aks update` along with `--enable-azure-container-storage` "
"to enable Azure Container Storage."
)
else:
logger.error("AKS update to enable Azure Container Storage failed.\nError: {0}".format(ex.message))
logger.error("AKS update to enable Azure Container Storage failed.\nError: %s", ex)


def perform_disable_azure_container_storage(
Expand Down Expand Up @@ -194,7 +192,7 @@ def perform_disable_azure_container_storage(
# we don't need to long wait while performing the delete operation.
# Setting no_wait_delete_op = True.
no_wait_delete_op = True
except Exception as ex:
except Exception as ex: # pylint: disable=broad-except
config_settings = [{"cli.storagePool.uninstallValidation": False}]
k8s_extension_custom_mod.update_k8s_extension(
cmd,
Expand All @@ -208,15 +206,16 @@ def perform_disable_azure_container_storage(
no_wait=True,
)

if ex.message.__contains__("pre-upgrade hooks failed"):
if "pre-upgrade hooks failed" in str(ex):
raise UnknownError(
"Validation failed. "
"Please ensure that storagepools are not being used. "
"Unable to disable Azure Container Storage. "
"Reseting cluster state."
)
else:
raise UnknownError("Validation failed. Unable to disable Azure Container Storage. Reseting cluster state.")
) from ex
raise UnknownError(
"Validation failed. Unable to disable Azure Container Storage. Reseting cluster state."
) from ex

# Step 2: If the extension is installed and validation succeeded or skipped, call delete_k8s_extension
try:
Expand All @@ -234,7 +233,9 @@ def perform_disable_azure_container_storage(
if not no_wait_delete_op:
LongRunningOperation(cmd.cli_ctx)(delete_op_result)
except Exception as delete_ex:
raise UnknownError("Failure observed while disabling Azure Container Storage.\nError: {0}".format(delete_ex.message))
raise UnknownError(
"Failure observed while disabling Azure Container Storage."
) from delete_ex

logger.warning("Azure Container Storage has been disabled.")

Expand Down

0 comments on commit 7e6e5c0

Please sign in to comment.