Skip to content

{AKS} Fix azdev style issues for azurecontainerstorage #7118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yanzhudd merged 1 commit into from
Dec 29, 2023
Merged

{AKS} Fix azdev style issues for azurecontainerstorage #7118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 42 additions & 25 deletions src/aks-preview/azext_aks_preview/azurecontainerstorage/_helpers.py
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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