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.

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

[Backup] BREAKING CHANGE: az backup item set-policy: Add warning prompt for migration from Standard to Enhanced Policy #28317

Merged
merged 8 commits into from
Feb 28, 2024
Merged

[Backup] BREAKING CHANGE: az backup item set-policy: Add warning prompt for migration from Standard to Enhanced Policy #28317

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c3dfa82
Added warning message for standard -> enhanced policy migration
Feb 7, 2024
62a0665
Cleaned and Fixed update policy type check + yes/no prompt
Feb 22, 2024
7892123
fix for none check
Feb 22, 2024
c16d993
All tests good
Feb 23, 2024
b5f9167
fixed linter issues and tests
Feb 26, 2024
167507f
Adding a --yes skip option
Feb 26, 2024
9e734d0
fixing error
Feb 26, 2024
4c75a06
fixing issue with parameters
Feb 26, 2024
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
33 changes: 28 additions & 5 deletions src/azure-cli/azure/cli/command_modules/backup/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from datetime import datetime, timedelta, timezone
# pylint: disable=too-many-lines
from knack.log import get_logger
from knack.prompting import prompt_y_n
from azure.mgmt.core.tools import is_valid_resource_id

from azure.mgmt.recoveryservicesbackup.activestamp import RecoveryServicesBackupClient
Expand Down Expand Up @@ -126,6 +127,12 @@
os_linux = 'Linux'
password_offset = 33
password_length = 15
vm_policy_type_map = {
'v2': 'enhanced',
'v1': 'standard'
}
enhanced_policy_type = "v2"
standard_policy_type = "v1"
# pylint: disable=too-many-function-args


Expand Down Expand Up @@ -173,9 +180,9 @@ def create_vault(client, vault_name, resource_group_name, location, tags=None,
"to their default values. It is recommended to use az backup vault update instead.")

# If the vault exists, we move to the update flow instead
update_vault(client, vault_name, resource_group_name, tags, public_network_access,
immutability_state, cross_subscription_restore_state, classic_alerts,
azure_monitor_alerts_for_job_failures)
return update_vault(client, vault_name, resource_group_name, tags, public_network_access,
immutability_state, cross_subscription_restore_state, classic_alerts,
azure_monitor_alerts_for_job_failures)
except CoreResourceNotFoundError:
vault_properties = VaultProperties()

Expand Down Expand Up @@ -1004,16 +1011,32 @@ def update_policy_for_item(cmd, client, resource_group_name, vault_name, item, p
vm_item_properties.policy_id = policy.id
vm_item_properties.source_resource_id = item.properties.source_resource_id
vm_item = ProtectedItemResource(properties=vm_item_properties)
existing_policy = common.show_policy(protection_policies_cf(cmd.cli_ctx), resource_group_name, vault_name,
item.properties.policy_name)

if is_critical_operation:
existing_policy = common.show_policy(protection_policies_cf(cmd.cli_ctx), resource_group_name, vault_name,
item.properties.policy_name)
if cust_help.is_retention_duration_decreased(existing_policy, policy, "AzureIaasVM"):
# update the payload with critical operation and add auxiliary header for cross tenant case
if tenant_id is not None:
client = get_mgmt_service_client(cmd.cli_ctx, RecoveryServicesBackupClient,
aux_tenants=[tenant_id]).protected_items
vm_item.properties.resource_guard_operation_requests = [cust_help.get_resource_guard_operation_request(
cmd.cli_ctx, resource_group_name, vault_name, "updateProtection")]

# Raise warning for standard->enhanced policy
try:
existing_policy_type = existing_policy.properties.policy_type.lower()
new_policy_type = policy.properties.policy_type.lower()
if (new_policy_type in vm_policy_type_map and vm_policy_type_map[new_policy_type] == 'enhanced' and
existing_policy_type in vm_policy_type_map and vm_policy_type_map[existing_policy_type] == 'standard'):
warning_prompt = ('Upgrading to enhanced policy can incur additional charges. Once upgraded to the enhanced '
'policy, it is not possible to revert back to the standard policy. Do you want to continue?')
if not prompt_y_n(warning_prompt):
logger.warning('Cancelling policy update operation')
return None
Comment on lines +1033 to +1037
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to confirm if this prompt will cause a breaking change to the customer's automation script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will. This scenario is only triggered in very rare situations where a trusted launch VM was protected by a standard policy (which is not permitted anymore).

should I add a --yes/-y flag that skips this warning prompt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zhoxing-ms I've pushed this change, please review it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you evaluated how many customers will be affected by this breaking change? If the impact is a little big, I suggest releasing this feature in the next breaking change window (Build Sprint)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have you evaluated how many customers will be affected by this breaking change? If the impact is a little big, I suggest releasing this feature in the next breaking change window (Build Sprint)

The impact would be on any customers who already have automation scripts setup to migrate from standard to enhanced backup policy. This is a specialized scenario, so it should not be a very major impact, but I'll check with our PMs for their recommendation as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

but I'll check with our PMs for their recommendation as well.

May I ask if this change has been confirmed? If not, since we'll have a code freeze at 02/27/2024 10:00 UTC, this PR may has to be postponed to next sprint (04-02).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I'll check with our PMs for their recommendation as well.

May I ask if this change has been confirmed? If not, since we'll have a code freeze at 02/27/2024 10:00 UTC, this PR may has to be postponed to next sprint (04-02).

Sorry for the delay, would it be possible to push it through in this sprint still? The PM had asked for customer usage of the command in the last 3 months. I just checked, and for the general case (not just this specific scenario), the command has been run < 500 times in the last 3 months, and only one day had > 30 calls, indicating existing automation. I think the customer impact for this change is negligible, and we should be able to proceed with this.

@yanzhudd

except (AttributeError):
logger.warning("Unable to fetch policy type for either existing or new policy. Proceeding with update.")

# Update policy
result = client.create_or_update(vault_name, resource_group_name, fabric_name,
container_uri, item_uri, vm_item, cls=cust_help.get_pipeline_response)
Expand Down
Loading
Loading