-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Adding ResiliencyProfile to VirtualMachines Properties #39385
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
base: release-feature/cplat-2025-11-01
Are you sure you want to change the base?
Adding ResiliencyProfile to VirtualMachines Properties #39385
Conversation
Next Steps to MergeNext steps that must be taken to merge this PR:
Comment generated by summarize-checks workflow run. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews
|
...xamples/2025-11-01/virtualMachineExamples/VirtualMachine_Create_WithZoneMovementEnabled.json
Show resolved
Hide resolved
...xamples/2025-11-01/virtualMachineExamples/VirtualMachine_Create_WithZoneMovementEnabled.json
Show resolved
Hide resolved
| /** | ||
| * Time gap between creation of two restore points for a VM, in hours. Default: 24. | ||
| */ | ||
| frequencyInHours?: int32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was decided to hide all attributes other than isEnabled from customers. So let's not publish these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is both for set and get right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Customers doing GET and then sending the same payload back is a very common pattern.
So, I would rather not have these attributes in the GET payload. It unnecessarily brings the risk of CRP throwing a "these are read-only attributes" client error. So, let's just keep them out of both read and write payloads.
Instead, we can keep these values document that periodicRestorePoints.isEnabled = true means you are getting daily restore point creation with 10 retained.
| /** | ||
| * Indicates if zone movement is enabled. | ||
| */ | ||
| isEnabled?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If zoneMovement: {} is provided, isEnabled = false, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking .. Zonemovement empty and then defaulting to false does not look good. Making isEnabled mandatory.
i.e If ZoneMovement is added , isEnabled should be explicitly required.
| }, | ||
| "ResiliencyProfile": { | ||
| "type": "object", | ||
| "description": "Describes resiliency profile for a VM.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check with Aarthi what she wants this description to be.
| @@ -0,0 +1,188 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why do we need to define example twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One file is auto generated file. Keeping comment active for API Spec Reviewer too in case I am doing anything wrong .
ac350cf to
85bf738
Compare
|
Please address the failing checks (LintDiff, Avocado) |
| }, | ||
| "periodicRestorePoints": { | ||
| "$ref": "#/definitions/PeriodicRestorePoints", | ||
| "description": "Periodic restore points configuration." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the new naming convention should we have this as Basic Data Protection or Automatic Restore Points @kirankumarp-azure ?
| @@ -0,0 +1,204 @@ | |||
| { | |||
| "parameters": { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need 2 copies of these example jsons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#39385 (comment), addressed here.
| "hardwareProfile": { | ||
| "vmSize": "Standard_D2s_v3" | ||
| }, | ||
| "resiliencyProfile": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirankumarp-azure, we do not need to add the other properties in periodic restore points - maxretained, frequency ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#39385 (comment), similar comment.

ARM (Control Plane) API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting helpsection at the bottom of this PR description.PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
Purpose of this PR
What's the purpose of this PR? Check the specific option that applies. This is mandatory!
Due diligence checklist
To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:
ARM resource provider contract and
REST guidelines (estimated time: 4 hours).
I understand this is required before I can proceed to the diagram Step 2, "ARM API changes review", for this PR.
Additional information
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiViewcomment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
suppressions guide to get approval.
Getting help
Purpose of this PRandDue diligence checklist.write accessper aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queuedstate, please add a comment with contents/azp run.This should result in a new comment denoting a
PR validation pipelinehas started and the checks should be updated after few minutes.