Skip to content

[AKS] add tag for nodepool #12145

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
mmyyrroonn merged 8 commits into from
Feb 26, 2020
Merged

[AKS] add tag for nodepool #12145

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1ef7791
add tag for nodepool
zqingqing1 Feb 12, 2020
385ca30
resolve conflict
zqingqing1 Feb 12, 2020
531059a
test added
zqingqing1 Feb 14, 2020
3680db8
Merge branch 'master' of https://github.com/Azure/azure-cli into add-…
zqingqing1 Feb 24, 2020
a4fc4a7
Merge branch 'dev' of https://github.com/Azure/azure-cli into add-vms…
zqingqing1 Feb 24, 2020
f224801
fix test
zqingqing1 Feb 24, 2020
f640905
strong response check
zqingqing1 Feb 25, 2020
0afc8e5
fix version again
zqingqing1 Feb 25, 2020
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
2 changes: 2 additions & 0 deletions src/azure-cli/HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ Release History
**AKS**

* Each cluster gets a separate service principal to improve isolation
* Add --nodepool-tags to node pool when creating azure kubernetes cluster
* Add --tags when adding or updating a nodepool to cluster

**AppConfig**

Expand Down
5 changes: 4 additions & 1 deletion src/azure-cli/azure/cli/command_modules/acs/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
validate_create_parameters, validate_k8s_client_version, validate_k8s_version, validate_linux_host_name,
validate_list_of_integers, validate_ssh_key, validate_connector_name, validate_max_pods, validate_nodes_count,
validate_nodepool_name, validate_vm_set_type, validate_load_balancer_sku, validate_load_balancer_outbound_ips,
validate_load_balancer_outbound_ip_prefixes, validate_taints, validate_ip_ranges, validate_acr,
validate_load_balancer_outbound_ip_prefixes, validate_taints, validate_ip_ranges, validate_acr, validate_nodepool_tags,
validate_load_balancer_outbound_ports, validate_load_balancer_idle_timeout, validate_vnet_subnet_id)

aci_connector_os_type = ['Windows', 'Linux', 'Both']
Expand Down Expand Up @@ -198,6 +198,7 @@ def load_arguments(self, _):
c.argument('skip_subnet_role_assignment', action='store_true')
c.argument('api_server_authorized_ip_ranges', type=str, validator=validate_ip_ranges)
c.argument('attach_acr', acr_arg_type)
c.argument('nodepool_tags', nargs='*', validator=validate_nodepool_tags, help='space-separated tags: key[=value] [key[=value] ...]. Use "" to clear existing tags.')
Copy link
Contributor

@mmyyrroonn mmyyrroonn Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use tags_type directly and overwrite the help= message.

Copy link
Member Author

@zqingqing1 zqingqing1 Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I tried to use tags_type, it won't work. Since the validator for tags_type, will read ns.tags, not ns.nodepool_tags, so I re-write a new function in our owned subfolder.

Copy link
Member Author

@zqingqing1 zqingqing1 Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myronfanqiu please help to merge if no other concerns or questions :)

Copy link
Member Author

@zqingqing1 zqingqing1 Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myronfanqiu When I run test, it failed with error, can you shed light on how to fix it?

AssertionError: No match for the request (<Request (PUT) https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/clitest000001/providers/Microsoft.ContainerService/managedClusters/cliakstest000001?api-version=2020-01-01>) was found. Can't overwrite existing cassette ('/Users/qizhe/azure-cli/azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/recordings/test_aks_nodepool_create_scale_delete.yaml') in your current record mode ('once').

Copy link
Contributor

@mmyyrroonn mmyyrroonn Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zqingqing1 Hi. You should run it with --live like azdev test acs --live. Could you please a single test for this new feature?

Copy link
Member Author

@zqingqing1 zqingqing1 Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely, will add it.

Copy link
Member Author

@zqingqing1 zqingqing1 Feb 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when I test it locally, it passed, but https://dev.azure.com/azure-sdk/public/_build/results?buildId=261822&view=logs&j=ef7e37da-9721-5d17-b890-4759c548b596&t=ce7f2a4a-7f31-5156-db1b-13049c09b684&l=7799 test failed with "Can't overwrite existing cassette". How gonna we fix it?

Copy link
Contributor

@mmyyrroonn mmyyrroonn Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API version is not consistent. Do you need to bump api-version? Looks like you install the package locally.

Copy link
Member Author

@zqingqing1 zqingqing1 Feb 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, aks-preview are using new api-version. I was testing with this cli extension, after removal, test should pass.

Copy link
Member Author

@zqingqing1 zqingqing1 Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test passed, please help to merge this PR. Appreciated.


with self.argument_context('aks update') as c:
c.argument('attach_acr', acr_arg_type, validator=validate_acr)
Expand Down Expand Up @@ -285,6 +286,7 @@ def load_arguments(self, _):
c.argument('os_type', type=str)
c.argument('enable_cluster_autoscaler', options_list=["--enable-cluster-autoscaler", "-e"], action='store_true')
c.argument('node_taints', type=str, validator=validate_taints)
c.argument('tags', tags_type)

for scope in ['aks nodepool show', 'aks nodepool delete', 'aks nodepool scale', 'aks nodepool upgrade', 'aks nodepool update']:
with self.argument_context(scope) as c:
Expand All @@ -294,6 +296,7 @@ def load_arguments(self, _):
c.argument('enable_cluster_autoscaler', options_list=["--enable-cluster-autoscaler", "-e"], action='store_true')
c.argument('disable_cluster_autoscaler', options_list=["--disable-cluster-autoscaler", "-d"], action='store_true')
c.argument('update_cluster_autoscaler', options_list=["--update-cluster-autoscaler", "-u"], action='store_true')
c.argument('tags', tags_type)

with self.argument_context('aks upgrade-connector') as c:
c.argument('aci_resource_group')
Expand Down
10 changes: 10 additions & 0 deletions src/azure-cli/azure/cli/command_modules/acs/_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# pylint: disable=no-name-in-module,import-error
from knack.log import get_logger

from azure.cli.core.commands.validators import validate_tag
from azure.cli.core.util import CLIError
import azure.cli.core.keys as keys

Expand Down Expand Up @@ -242,6 +243,15 @@ def validate_acr(namespace):
raise CLIError('Cannot specify "--attach-acr" and "--detach-acr" at the same time.')


def validate_nodepool_tags(ns):
""" Extracts multiple space-separated tags in key[=value] format """
if isinstance(ns.nodepool_tags, list):
tags_dict = {}
for item in ns.nodepool_tags:
tags_dict.update(validate_tag(item))
ns.nodepool_tags = tags_dict


def validate_vnet_subnet_id(namespace):
if namespace.vnet_subnet_id is not None:
if namespace.vnet_subnet_id == '':
Expand Down
14 changes: 11 additions & 3 deletions src/azure-cli/azure/cli/command_modules/acs/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1605,6 +1605,7 @@ def aks_create(cmd, client, resource_group_name, name, ssh_key_value, # pylint:
node_osdisk_size=0,
node_count=3,
nodepool_name="nodepool1",
nodepool_tags=None,
service_principal=None, client_secret=None,
no_ssh_key=False,
disable_rbac=None,
Expand Down Expand Up @@ -1658,6 +1659,7 @@ def aks_create(cmd, client, resource_group_name, name, ssh_key_value, # pylint:

agent_pool_profile = ManagedClusterAgentPoolProfile(
name=_trim_nodepoolname(nodepool_name), # Must be 12 chars or less before ACS RP adds to it
tags=nodepool_tags,
count=int(node_count),
vm_size=node_vm_size,
os_type="Linux",
Expand Down Expand Up @@ -2658,6 +2660,7 @@ def aks_agentpool_add(cmd, client, resource_group_name, cluster_name, nodepool_n
max_count=None,
enable_cluster_autoscaler=False,
node_taints=None,
tags=None,
no_wait=False):
instances = client.list(resource_group_name, cluster_name)
for agentpool_profile in instances:
Expand All @@ -2682,6 +2685,7 @@ def aks_agentpool_add(cmd, client, resource_group_name, cluster_name, nodepool_n

agent_pool = AgentPool(
name=nodepool_name,
tags=tags,
count=int(node_count),
vm_size=node_vm_size,
os_type=os_type,
Expand Down Expand Up @@ -2731,13 +2735,15 @@ def aks_agentpool_update(cmd, client, resource_group_name, cluster_name, nodepoo
disable_cluster_autoscaler=False,
update_cluster_autoscaler=False,
min_count=None, max_count=None,
tags=None,
no_wait=False):

update_flags = enable_cluster_autoscaler + disable_cluster_autoscaler + update_cluster_autoscaler
if update_flags != 1:
raise CLIError('Please specify "--enable-cluster-autoscaler" or '
'"--disable-cluster-autoscaler" or '
'"--update-cluster-autoscaler"')
if update_flags != 0 or tags is None:
raise CLIError('Please specify "--enable-cluster-autoscaler" or '
'"--disable-cluster-autoscaler" or '
'"--update-cluster-autoscaler"')

instance = client.get(resource_group_name, cluster_name, nodepool_name)
node_count = instance.count
Expand Down Expand Up @@ -2771,6 +2777,8 @@ def aks_agentpool_update(cmd, client, resource_group_name, cluster_name, nodepoo
instance.min_count = None
instance.max_count = None

instance.tags = tags

return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, cluster_name, nodepool_name, instance)


Expand Down