Skip to content
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.

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

[Compute] (1) vm create: vm create: Add --vmss to specify an existing virtual machine scale set that the virtual machine should be assigned to. (2) vmss create: Add --orchestrator to specify how virtual machines are managed by the scale set. #10646

Merged
merged 16 commits into from
Oct 28, 2019

Conversation

qwordy
Copy link
Member

@qwordy qwordy commented Sep 26, 2019

  • vm create: Add --vmss to specify an existing virtual machine scale set that the virtual machine should be assigned to.
  • vmss create: Add --orchestrator to specify how virtual machines are managed by the scale set.

Implement #10156


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

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

…that the virtual machine should be assigned to
@qwordy
Copy link
Member Author

qwordy commented Sep 26, 2019

Testing has some problems now.

@yonzhan
Copy link
Collaborator

yonzhan commented Oct 13, 2019

@qwordy what is the status of this PR right now ?

@yonzhan yonzhan added this to the S160 - For Ignite milestone Oct 13, 2019
@qwordy
Copy link
Member Author

qwordy commented Oct 14, 2019

@qwordy what is the status of this PR right now ?

Use case problem.
And after many PR were merged, conflicts sprang up.
I need to fix both of them.

@qwordy qwordy requested review from yonzhan and mmyyrroonn October 21, 2019 09:32
@qwordy
Copy link
Member Author

qwordy commented Oct 21, 2019

@shpa-microsoft, please help review. Thanks.

@qwordy
Copy link
Member Author

qwordy commented Oct 21, 2019

@qwordy what is the status of this PR right now ?

Use case problem.
And after many PR were merged, conflicts sprang up.
I need to fix both of them.

Solved.

@@ -25,6 +25,8 @@ Release History
**Compute**

* vm create: Add warning when specifying accelerated networking and an existing NIC together.
* vm create: Add --vmss to specify virtual machine scale set that the virtual machine should be assigned to.
* vmss create: Add --empty to create a VMSS without VM profile, which can be then referenced in `vm create`.

Choose a reason for hiding this comment

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

Empty keyword will give a wrong indication IMO cause one can create an empty VMSS even when VM profile exists. We have started to use orchestrationMode to specify this. The 2 modes are: VMSS and VM. I'd also seek feedback from Jun and Varun.

Choose a reason for hiding this comment

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

Also vmss create should happen before vm create in this case. Not sure ordering matters in the doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also vmss create should happen before vm create in this case. Not sure ordering matters in the doc.

This doesn't matter. It's just change log.

Copy link
Member Author

Choose a reason for hiding this comment

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

Empty keyword will give a wrong indication IMO cause one can create an empty VMSS even when VM profile exists. We have started to use orchestrationMode to specify this. The 2 modes are: VMSS and VM. I'd also seek feedback from Jun and Varun.

I think "empty" is ambiguous, too. Do you have a suggested name?
Is there a change of vm and vmss creation ways in the future? You mentioned orchestrationMode.

src/azure-cli/azure/cli/command_modules/vm/_params.py Outdated Show resolved Hide resolved
@@ -496,6 +497,7 @@ def load_arguments(self, _):
help="The eviction policy for virtual machines in a low priority scale set.", is_preview=True)
c.argument('application_security_groups', resource_type=ResourceType.MGMT_COMPUTE, min_api='2018-06-01', nargs='+', options_list=['--asgs'], help='Space-separated list of existing application security groups to associate with the VM.', arg_group='Network', validator=validate_asg_names_or_ids)
c.argument('computer_name_prefix', help='Computer name prefix for all of the virtual machines in the scale set. Computer name prefixes must be 1 to 15 characters long')
c.argument('empty', help='Create a VMSS without VM profile, which can be then referenced in `vm create`.', arg_type=get_three_state_flag())

Choose a reason for hiding this comment

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

Nit: Creates
Additional: If we change the term to orchestration mode, then all this description will change.

Copy link
Member Author

@qwordy qwordy Oct 22, 2019

Choose a reason for hiding this comment

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

When will you change the term to orchestration mode?

Choose a reason for hiding this comment

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

We have that term on portal already. Let's have a call today and clarify all the gaps?

Choose a reason for hiding this comment

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

Decided on enum "orchestration" for this purpose. Keeping it in sync with Portal

# vmss without vm profile
if empty:
if platform_fault_domain_count is None:
platform_fault_domain_count = 2

Choose a reason for hiding this comment

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

Why is it defaulted to 2? Can this not be passed down as null?

Copy link
Member Author

@qwordy qwordy Oct 22, 2019

Choose a reason for hiding this comment

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

When I use null,

Deployment failed. Correlation ID: e7f42bc9-2d4b-4310-90cd-8291da117c5d. {
  "error": {
    "code": "InvalidParameter",
    "message": "Required parameter 'platformFaultDomainCount' is missing (null).",
    "target": "platformFaultDomainCount"
  }
}

This is my template:

vmss = {
            'type': 'Microsoft.Compute/virtualMachineScaleSets',
            'name': name,
            'location': location,
            'tags': tags,
            'apiVersion': cmd.get_api_version(ResourceType.MGMT_COMPUTE, operation_group='virtual_machine_scale_sets'),
            'properties': {
                'singlePlacementGroup': True,
                'provisioningState': 0,
                'platformFaultDomainCount': platform_fault_domain_count
            }
        }

Did I make some mistakes in the template?

Choose a reason for hiding this comment

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

Oh My bad. We do need this one; but not sure defaulting is the right thing to do v/s making clients always enter that value.

@qwordy
Copy link
Member Author

qwordy commented Oct 22, 2019

Another question, after I create a vmss without vm profile, I can't run commands like list-instances, get-instance-view on it,

Operation 'VirtualMachineScaleSets.virtualMachines.GET' is not allowed on Virtual Machine Scale Set 'vmss1'
Operation 'VirtualMachineScaleSets.instanceView.GET' is not allowed on Virtual Machine Scale Set 'vmss1'.

Is it normal?

Copy link

@shpa-microsoft shpa-microsoft left a comment

Choose a reason for hiding this comment

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

Another question, after I create a vmss without vm profile, I can't run commands like list-instances, get-instance-view on it,

Operation 'VirtualMachineScaleSets.virtualMachines.GET' is not allowed on Virtual Machine Scale Set 'vmss1'
Operation 'VirtualMachineScaleSets.instanceView.GET' is not allowed on Virtual Machine Scale Set 'vmss1'.

Is it normal?

Yes this is expected. I specifically throw the "Not allowed" error for this. It is because the VM added to scaleset is not the ScalesetVM Instance. Rather a standalone type of instance that is added to VMSS. If that makes anything clear

@qwordy
Copy link
Member Author

qwordy commented Oct 23, 2019

Shalaka and I have discussed in a phone call.

shpa-microsoft
shpa-microsoft previously approved these changes Oct 24, 2019
@qwordy qwordy requested a review from yonzhan October 24, 2019 04:42
@qwordy qwordy changed the title [Compute] vm create: Add --vmss to specify virtual machine scale set that the virtual machine should be assigned to [Compute] (1) vm create: vm create: Add --vmss to specify an existing virtual machine scale set that the virtual machine should be assigned to. (2) vmss create: Add --orchestrator to specify how virtual machines are managed by the scale set. Oct 24, 2019
yonzhan
yonzhan previously approved these changes Oct 24, 2019
Copy link
Collaborator

@yonzhan yonzhan left a comment

Choose a reason for hiding this comment

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

LGTM

mmyyrroonn
mmyyrroonn previously approved these changes Oct 24, 2019
@qwordy qwordy dismissed stale reviews from mmyyrroonn, yonzhan, and shpa-microsoft via fe2273d October 24, 2019 06:24
Copy link
Collaborator

@yonzhan yonzhan left a comment

Choose a reason for hiding this comment

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

LGTM

@shpa-microsoft
Copy link

Can we name the param: OrchestrationMode instead? With options "VM" and "ScaleSetVM"

Copy link

@shpa-microsoft shpa-microsoft left a comment

Choose a reason for hiding this comment

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

OrchestrationMode comment

@qwordy qwordy merged commit 87b3b74 into Azure:dev Oct 28, 2019
@qwordy
Copy link
Member Author

qwordy commented Oct 28, 2019

Can we name the param: OrchestrationMode instead? With options "VM" and "ScaleSetVM"

I've merged it to enable related people to try it. I will open another PR for this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants