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

fix: Remove managed pip if switched to another pip in a different res… #8419

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

fix: Remove managed pip if switched to another pip in a different res… #8419

wants to merge 1 commit into from

Conversation

nilo19
Copy link
Contributor

@nilo19 nilo19 commented Feb 24, 2025

…ource group

What type of PR is this?

/kind bug

What this PR does / why we need it:

We should remove managed pip when the service switches to a different pip in another resource group.

Which issue(s) this PR fixes:

Fixes #8113

Special notes for your reviewer:

Does this PR introduce a user-facing change?

fix: Remove managed pip if switched to another pip in a different resource group

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Feb 24, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nilo19

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

The pull request process is described 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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2025
@coveralls
Copy link

coveralls commented Feb 24, 2025 •
edited
Loading

Coverage Status

coverage: 74.64% (+0.02%) from 74.618%
when pulling a31b322 on nilo19:fix/pip
into 01e8d0f on kubernetes-sigs:master.

@nilo19 nilo19 force-pushed the fix/pip branch 4 times, most recently from a31b322 to e2f5b3b Compare February 25, 2025 22:36
@nilo19
Copy link
Contributor Author

nilo19 commented Feb 26, 2025

/retest pull-cloud-provider-azure-e2e-ccm-capz\

@k8s-ci-robot
Copy link
Contributor

@nilo19: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-cloud-provider-azure-check

/test pull-cloud-provider-azure-e2e-capz

/test pull-cloud-provider-azure-e2e-ccm-capz

/test pull-cloud-provider-azure-e2e-ccm-dualstack-capz

/test pull-cloud-provider-azure-e2e-ccm-dualstack-vmss-capz

/test pull-cloud-provider-azure-e2e-ccm-ipv6-capz

/test pull-cloud-provider-azure-e2e-ccm-ipv6-vmss-capz

/test pull-cloud-provider-azure-e2e-ccm-vmss-capz

/test pull-cloud-provider-azure-e2e-ccm-vmss-ip-lb-capz

/test pull-cloud-provider-azure-e2e-ccm-vmss-multi-slb-capz

/test pull-cloud-provider-azure-e2e-ccm-vmss-shared-probe-capz

/test pull-cloud-provider-azure-e2e-ccm-vmssflex-capz

/test pull-cloud-provider-azure-unit

Use /test all to run the following jobs that were automatically triggered:

pull-cloud-provider-azure-check

pull-cloud-provider-azure-e2e-capz

pull-cloud-provider-azure-e2e-ccm-capz

pull-cloud-provider-azure-e2e-ccm-vmss-capz

pull-cloud-provider-azure-e2e-ccm-vmss-ip-lb-capz

pull-cloud-provider-azure-e2e-ccm-vmss-multi-slb-capz

pull-cloud-provider-azure-e2e-ccm-vmss-shared-probe-capz

pull-cloud-provider-azure-e2e-ccm-vmssflex-capz

pull-cloud-provider-azure-unit

In response to this:

/retest pull-cloud-provider-azure-e2e-ccm-capz\

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.

@nilo19
Copy link
Contributor Author

nilo19 commented Feb 26, 2025

/test pull-cloud-provider-azure-e2e-ccm-capz

@nilo19
Copy link
Contributor Author

nilo19 commented Feb 26, 2025

/test pull-cloud-provider-azure-e2e-ccm-vmss-ip-lb-capz

@nilo19
Copy link
Contributor Author

nilo19 commented Feb 26, 2025

/retest

feiskyer
feiskyer reviewed Feb 27, 2025
View reviewed changes
pkg/provider/azure_loadbalancer.go
pipID := strings.ToLower((ptr.Deref(pipCopy.ID, "")))
rg, err := getPIPRGFromID(pipID)
if err != nil {
logger.Error(err, "Failed to get resource group from PIP ID", "pip-id", pipID)
Copy link
Member

Choose a reason for hiding this comment

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

return err 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.

done

feiskyer
feiskyer reviewed Feb 27, 2025
View reviewed changes
pkg/provider/azure_loadbalancer.go Outdated
@@ -3267,13 +3267,29 @@ func (az *Cloud) ensurePIPTagged(service *v1.Service, pip *armnetwork.PublicIPAd

// reconcilePublicIPs reconciles the PublicIP resources similar to how the LB is reconciled.
func (az *Cloud) reconcilePublicIPs(ctx context.Context, clusterName string, service *v1.Service, lbName string, wantLb bool) ([]*armnetwork.PublicIPAddress, error) {
logger := klog.FromContext(ctx).WithName("reconcilePublicIPs").
WithValues("load-balancer", lbName)
Copy link
Member

@feiskyer feiskyer Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

how about using the same log format for those keys, e.g. loadBalancer, resourceGroup (in the following lines)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@nilo19
Copy link
Contributor Author

nilo19 commented Feb 28, 2025

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer left review comments

@andyzhangx andyzhangx Awaiting requested review from andyzhangx

@MartinForReal MartinForReal Awaiting requested review from MartinForReal

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Public IP Address orphaned when updating LoadBalancer type Service to a user-provided PIP from another RG
4 participants
@nilo19 @k8s-ci-robot @coveralls @feiskyer