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

Jump to bottom

Azure: add force delete option #6432

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Azure: add force delete option #6432

wants to merge 1 commit into from

Conversation

Jont828
Copy link
Contributor

@Jont828 Jont828 commented Jan 9, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it: Add force delete option in Azure to the Helm chart and Azure autoscaler.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Azure: add force delete option

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 9, 2024
@k8s-ci-robot k8s-ci-robot added the area/provider/azure Issues or PRs related to azure provider label Jan 9, 2024
@k8s-ci-robot k8s-ci-robot requested a review from nilo19 January 9, 2024 22:34
@jackfrancis
Copy link
Contributor

You'll want to cut a new chart version (Chart.yaml) w/ these changes. I'd say that this warrants a minor bump.

@jackfrancis
Copy link
Contributor

/assign @gandhipr

@gandhipr
Copy link
Contributor

gandhipr commented Jan 9, 2024

Can we add some testing details performed?

@Jont828 Jont828 changed the title Azure: add force delete option [WIP] Azure: add force delete option Jan 9, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 9, 2024
@Jont828
Copy link
Contributor Author

Jont828 commented Jan 9, 2024

Sure, also forgot to mention this is still WIP

@Jont828 Jont828 changed the title [WIP] Azure: add force delete option Azure: add force delete option Jan 10, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2024
@Jont828 Jont828 force-pushed the azure-force-delete branch 2 times, most recently from 3af1015 to 7212628 Compare January 10, 2024 23:34
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 16, 2024
@Jont828
Copy link
Contributor Author

Jont828 commented Jan 16, 2024

@gandhipr @jackfrancis Fixed unit tests, ready for final review pass before I squash.

gandhipr
gandhipr suggested changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@gandhipr gandhipr left a comment

Choose a reason for hiding this comment

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

Everything else looks good to me. Just had comment related to initializing the toggle.

charts/cluster-autoscaler/templates/deployment.yaml Outdated
@@ -166,6 +166,8 @@ spec:
secretKeyRef:
key: VMType
name: {{ default (include "cluster-autoscaler.fullname" .) .Values.secretKeyRefNameOverride }}
- name: AZURE_ENABLE_FORCE_DELETE
value: "{{ .Values.azureEnableForceDelete }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just have it in the same way as enableDynamicInstanceList ? Do we have some added advantage here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we want to be putting all the env vars in the Helm chart based on what @jackfrancis said a while back.

cluster-autoscaler/cloudprovider/azure/azure_config.go Show resolved Hide resolved
@gandhipr
Copy link
Contributor

gandhipr commented Mar 12, 2024 •
edited
Loading

/lgtm

@Jont828 Jont828 force-pushed the azure-force-delete branch from 7b26fc3 to e8ad029 Compare March 12, 2024 22:13
@jackfrancis
Copy link
Contributor

@Jont828 you'll need to bump the chart version (which will mean whichever of your PRs merges first will conflict with the other one that also needs a chart version bump, fyi)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2024
@jackfrancis
Copy link
Contributor

/test pull-cluster-autoscaler-e2e-azure

@jackfrancis
Copy link
Contributor

/test ?

@k8s-ci-robot
Copy link
Contributor

@jackfrancis: The following commands are available to trigger optional jobs:

  • /test pull-cluster-autoscaler-e2e-azure

Use /test all to run all jobs.

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Jont828 Jont828 force-pushed the azure-force-delete branch from e8ad029 to 8891d48 Compare March 19, 2024 21:56
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Jont828
Once this PR has been reviewed and has the lgtm label, please ask for approval from gandhipr. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 19, 2024
@jackfrancis
Copy link
Contributor

/test pull-cluster-autoscaler-e2e-azure

@Jont828 Jont828 force-pushed the azure-force-delete branch from 8891d48 to c5c4611 Compare April 24, 2024 22:31
@Jont828
Copy link
Contributor Author

Jont828 commented Apr 29, 2024

@jackfrancis @gandhipr Are we good to merge this?

@jackfrancis
Copy link
Contributor

Seems like something got lost here? I only see a fraction of the original changes.

@Jont828
Copy link
Contributor Author

Jont828 commented May 20, 2024

@jackfrancis We forgot to remove the commit containing force delete in #6447 so force delete actually got merged already. This is just a follow up to address comments and for consistency.

@Shubham82
Copy link
Contributor

/remove-area helm-charts
removing the /area helm-chart label as there are no changes/modifications by this PR (now) in the Charts directory.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2024
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandhipr gandhipr gandhipr requested changes

@gjtempleton gjtempleton Awaiting requested review from gjtempleton

@nilo19 nilo19 Awaiting requested review from nilo19

Assignees

@gandhipr gandhipr

Labels
area/cluster-autoscaler area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Jont828 @jackfrancis @gandhipr @k8s-ci-robot @Shubham82 @k8s-triage-robot