[Compute] `az vmss reimage`: Fix the bug that all instances will be reimaged after using `--instance-id` by zhoxing-ms · Pull Request #25477 · Azure/azure-cli

zhoxing-ms · 2023-02-16T03:54:50Z

Related command

az vmss reimage

Description

The loop logic of reimaging instance from #25131 is wrong. We should use the batch operation provided by the Python SDK virtual_machine_scale_sets.begin_reimage_all. Otherwise, if the process is not returned in time after the end of the loop logic, and all instances will be re-imaged unexpectedly

In addition, in order to make the design style of parameter conform to the specification, add parameter --instance-ids to replace original parameter --instance-id

Issue Close: #25476

Testing Guide

History Notes

[Compute] az vmss reimage: Fix the bug that all instances will be reimaged after using --instance-id and add new parameter --instance-ids to replace --instance-id

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.

yonzhan · 2023-02-16T03:56:44Z

Compute

jiasli · 2023-02-16T04:18:51Z

src/azure-cli/azure/cli/command_modules/vm/_params.py

-        c.argument('instance_id', nargs='+', help='Space-separated list of VM instance ID. If missing, reimage all instances.')
+        c.argument('instance_id', nargs='+', deprecate_info=c.deprecate(target='--instance-id', redirect='--instance-ids', hide=True),
+                   help='Space-separated list of VM instance ID. If missing, reimage all instances.')
+        c.argument('instance_ids', nargs='+', help='Space-separated list of VM instance ID. If missing, reimage all instances.')


Consider making --instance-id an alias of --instance--ids and deprecate it?

Good suggestion! Updated

jiasli · 2023-02-16T04:19:27Z

src/azure-cli/azure/cli/command_modules/vm/custom.py



-def reimage_vmss(cmd, resource_group_name, vm_scale_set_name, instance_id=None, no_wait=False):
+def reimage_vmss(cmd, resource_group_name, vm_scale_set_name, instance_id=None, instance_ids=None, no_wait=False):


Retaining both instance_id and instance_ids makes the code hard to maintain.

jiasli · 2023-02-16T04:20:30Z

src/azure-cli/azure/cli/command_modules/vm/custom.py

+
+    if instance_ids:
+        VirtualMachineScaleSetVMInstanceIDs = cmd.get_models('VirtualMachineScaleSetVMInstanceIDs')
+        instance_ids = VirtualMachineScaleSetVMInstanceIDs(instance_ids=instance_ids)


Better to change the name if its type changes, in order to make the code easier to read.

Use instance_ids as variable name just to keep consistent with the generally existing code style, such as

azure-cli/src/azure-cli/azure/cli/command_modules/vm/custom.py

Lines 3541 to 3558 in 917fba1

def deallocate_vmss(cmd, resource_group_name, vm_scale_set_name, instance_ids=None, no_wait=False):

client = _compute_client_factory(cmd.cli_ctx)

if instance_ids and len(instance_ids) == 1:

return sdk_no_wait(no_wait, client.virtual_machine_scale_set_vms.begin_deallocate,

resource_group_name, vm_scale_set_name, instance_ids[0])

VirtualMachineScaleSetVMInstanceIDs = cmd.get_models('VirtualMachineScaleSetVMInstanceIDs')

vm_instance_i_ds = VirtualMachineScaleSetVMInstanceIDs(instance_ids=instance_ids)

return sdk_no_wait(no_wait, client.virtual_machine_scale_sets.begin_deallocate,

resource_group_name, vm_scale_set_name, vm_instance_i_ds)

def delete_vmss_instances(cmd, resource_group_name, vm_scale_set_name, instance_ids, no_wait=False):

client = _compute_client_factory(cmd.cli_ctx)

VirtualMachineScaleSetVMInstanceRequiredIDs = cmd.get_models('VirtualMachineScaleSetVMInstanceRequiredIDs')

instance_ids = VirtualMachineScaleSetVMInstanceRequiredIDs(instance_ids=instance_ids)

return sdk_no_wait(no_wait, client.virtual_machine_scale_sets.begin_delete_instances,

resource_group_name, vm_scale_set_name, instance_ids)

Ugh, vm_instance_i_ds is certainly ugly.

jiasli · 2023-02-16T04:24:54Z

src/azure-cli/azure/cli/command_modules/vm/tests/latest/test_vm_commands.py

-        self.cmd('vmss create -g {rg} -n {vmss} --image ubuntults --instance-count 2')
-        self.cmd('vmss reimage -g {rg} -n {vmss} --instance-id 0 1')


I am a little bit curious how the test passed previously.

At present, this is caused by the unstable behavior of REST service.

After the request virtual-machine-scale-sets/create-or-update is completed, if the request virtual-machine-scale-sets/reimage will be sent immediately, and the following error will occasionally occur

(InvalidParameter) The provided instanceId 0 is not an active Virtual Machine Scale Set VM instanceId. Code: InvalidParameter Message: The provided instanceId 0 is not an active Virtual Machine Scale Set VM instanceId. Target: instanceIds

I guess this problem may be caused by the VM instance's state is not completely ready after the request virtual-machine-scale-sets/create-or-update is completed, because this problem can be avoided if the time interval between the reimage operations is longer

@grizzlytheodore Could you please take a look at this issue? Or you can ask the right person to look at it?

jiasli · 2023-02-16T04:28:13Z

src/azure-cli/azure/cli/command_modules/vm/_help.py

+  - name: Reimage a VM instance within a VMSS.
    text: |
-        az vmss reimage --instance-id 1 --name MyScaleSet --resource-group MyResourceGroup --subscription MySubscription
-    crafted: true
+        az vmss reimage --instance-ids 1 --name MyScaleSet --resource-group MyResourceGroup --subscription MySubscription
+  - name: Reimage a batch of VM instances within a VMSS.
+    text: |
+        az vmss reimage --instance-ids 1 2 3 --name MyScaleSet --resource-group MyResourceGroup --subscription MySubscription


Let's provide an example for reimaging all instances.

jiasli · 2023-02-16T06:25:38Z

src/azure-cli/azure/cli/command_modules/vm/tests/latest/test_vm_commands.py

+        instances = self.cmd('vmss list-instances -g {rg} -n {vmss}').get_output_in_json()
+        self.kwargs['instance_id1'] = instances[0]['instanceId']
+        self.kwargs['instance_id2'] = instances[1]['instanceId']


az vmss list-instances can still return 0, 1. If we pass 0, 1 to az vmss reimage, we may still see the failure (#25476).

The correct solution is for the computer service to guarantee all instances are ready to be reimaged after az vmss create.

jiasli · 2023-02-21T02:49:18Z

CI randomly fails due to this threading issue (#25472). Let's merge this PR ASAP.

…eimaged after using `--instance-id` (Azure#25477)

Fix vmss reimage issue

d0788b0

zhoxing-ms requested review from jsntcy, wangzelin007 and yanzhudd as code owners February 16, 2023 03:54

ghost requested a review from yonzhan February 16, 2023 03:54

ghost added the Auto-Assign Auto assign by bot label Feb 16, 2023

ghost assigned zhoxing-ms Feb 16, 2023

ghost added this to the Feb 2023 (2023-03-07) milestone Feb 16, 2023

ghost added the Compute az vm/vmss/image/disk/snapshot label Feb 16, 2023

Fix linter issue

917fba1

zhoxing-ms mentioned this pull request Feb 16, 2023

{Packaging} Bump paramiko to 3.0.0 #25472

Merged

3 tasks

yanzhudd previously approved these changes Feb 16, 2023

View reviewed changes

jiasli reviewed Feb 16, 2023

View reviewed changes

Address comments

fd9c7dc

zhoxing-ms dismissed yanzhudd’s stale review via fd9c7dc February 16, 2023 09:26

jiasli approved these changes Feb 21, 2023

View reviewed changes

yanzhudd approved these changes Feb 21, 2023

View reviewed changes

zhoxing-ms merged commit 682a4dc into Azure:dev Feb 21, 2023

avgale pushed a commit to avgale/azure-cli that referenced this pull request Aug 24, 2023

[Compute] az vmss reimage: Fix the bug that all instances will be r…

0426c83

…eimaged after using `--instance-id` (Azure#25477)



		def reimage_vmss(cmd, resource_group_name, vm_scale_set_name, instance_id=None, no_wait=False):
		def reimage_vmss(cmd, resource_group_name, vm_scale_set_name, instance_id=None, instance_ids=None, no_wait=False):

	def deallocate_vmss(cmd, resource_group_name, vm_scale_set_name, instance_ids=None, no_wait=False):
	client = _compute_client_factory(cmd.cli_ctx)
	if instance_ids and len(instance_ids) == 1:
	return sdk_no_wait(no_wait, client.virtual_machine_scale_set_vms.begin_deallocate,
	resource_group_name, vm_scale_set_name, instance_ids[0])

	VirtualMachineScaleSetVMInstanceIDs = cmd.get_models('VirtualMachineScaleSetVMInstanceIDs')
	vm_instance_i_ds = VirtualMachineScaleSetVMInstanceIDs(instance_ids=instance_ids)
	return sdk_no_wait(no_wait, client.virtual_machine_scale_sets.begin_deallocate,
	resource_group_name, vm_scale_set_name, vm_instance_i_ds)


	def delete_vmss_instances(cmd, resource_group_name, vm_scale_set_name, instance_ids, no_wait=False):
	client = _compute_client_factory(cmd.cli_ctx)
	VirtualMachineScaleSetVMInstanceRequiredIDs = cmd.get_models('VirtualMachineScaleSetVMInstanceRequiredIDs')
	instance_ids = VirtualMachineScaleSetVMInstanceRequiredIDs(instance_ids=instance_ids)
	return sdk_no_wait(no_wait, client.virtual_machine_scale_sets.begin_delete_instances,
	resource_group_name, vm_scale_set_name, instance_ids)

		self.cmd('vmss create -g {rg} -n {vmss} --image ubuntults --instance-count 2')
		self.cmd('vmss reimage -g {rg} -n {vmss} --instance-id 0 1')

Conversation

zhoxing-ms commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yonzhan commented Feb 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiasli Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiasli commented Feb 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhoxing-ms commented Feb 16, 2023 •

edited

Loading

jiasli Feb 16, 2023 •

edited

Loading