Repository: kubernetes-sigs/cloud-provider-azure
Pull request #2406 (2022-09-27 13:58:47)

Get latest service before UpdateLoadBalancer and serialize LB reconcile
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide/first-contribution.md#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/sig-release/release.md#issuepr-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/sig-testing/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

#### What type of PR is this?
/kind bug
<!--
Add one of the following kinds:
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind design

Optionally add one or more of the following kinds if applicable:
/kind api-change
/kind deprecation
/kind failing-test
/kind flake
/kind regression
-->

#### What this PR does / why we need it:
Avoid race condition between node-sync-loop routine and service reconcile routine.
Avoid UpdateLoadBalancer() using stale service spec to update Load Balancers.

#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->
```release-note
Avoid race condition between node-sync-loop routine and service reconcile routine.
Avoid UpdateLoadBalancer() using stale service spec to update Load Balancers.
```

#### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

<!--
This section can be blank if this pull request does not require a release note.

When adding links which point to resources within git repositories, like
KEPs or supporting documentation, please reference a specific commit and avoid
linking directly to the master branch. This ensures that links reference a
specific point in time, rather than a document that may change over time.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [KEP]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```


netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *kubernetes-sigs-cloud-provide-azure* processing.


|  Name | Link |
|---------------------------------|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | f0a7df28cc219938cd6f4ef1d03ee380262444bf |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/kubernetes-sigs-cloud-provide-azure/deploys/6333019aab1feb000863890e |
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/cloud-provider-azure/pull/2406#" title="Author self-approved">jwtty</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcloud-provider-azure).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/cloud-provider-azure/blob/master/OWNERS)~~ [jwtty]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
jwtty: /block still testing
jwtty: /hold
coveralls: 
[![Coverage Status](https://coveralls.io/builds/52796123/badge)](https://coveralls.io/builds/52796123)

Coverage decreased (-0.07%) to 80.274% when pulling **f0a7df28cc219938cd6f4ef1d03ee380262444bf on jwtty:race** into **628624958d351069e464d5cf3fc135fc4100c1cd on kubernetes-sigs:master**.

k8s-ci-robot: @jwtty: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-cloud-provider-azure-e2e-capz | e6133c2b6b95064222cbd88d277d2289000f7328 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cloud-provider-azure/2406/pull-cloud-provider-azure-e2e-capz/1574944414449537024) | true | `/test pull-cloud-provider-azure-e2e-capz`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cloud-provider-azure&pr=2406). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Ajwtty). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cloud-provider-azure/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
jwtty: /test pull-cloud-provider-azure-e2e-capz

Patch:
diff --git a/pkg/provider/azure.go b/pkg/provider/azure.go
index 2cd8e5ab2..dd28ee28a 100644
--- a/pkg/provider/azure.go
+++ b/pkg/provider/azure.go
@@ -38,6 +38,7 @@ import (
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
 	v1core "k8s.io/client-go/kubernetes/typed/core/v1"
+	corelisters "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/flowcontrol"
@@ -143,8 +144,8 @@ type Config struct {
 	// The name of the scale set that should be used as the load balancer backend.
 	// If this is set, the Azure cloudprovider will only add nodes from that scale set to the load
 	// balancer backend pool. If this is not set, and multiple agent pools (scale sets) are used, then
-	// the cloudprovider will try to add all nodes to a single backend pool which is forbidden in the basic sku.
-	// In other words, if you use multiple agent pools (scale sets), and loadBalancerSku is set to basic, you MUST set this field.
+	// the cloudprovider will try to add all nodes to a single backend pool which is forbidden.
+	// In other words, if you use multiple agent pools (scale sets), you MUST set this field.
 	PrimaryScaleSetName string `json:"primaryScaleSetName,omitempty" yaml:"primaryScaleSetName,omitempty"`
 	// Tags determines what tags shall be applied to the shared resources managed by controller manager, which
 	// includes load balancer, security group and route table. The supported format is `a=b,c=d,...`. After updated
@@ -359,6 +360,11 @@ type Cloud struct {
 	// use LB frontEndIpConfiguration ID as the key and search for PLS attached to the frontEnd
 	plsCache *azcache.TimedCache
 
+	// Add service lister to always get latest service
+	serviceLister corelisters.ServiceLister
+	// node-sync-loop routine and service-reconcile routine should not update LoadBalancer at the same time
+	serviceReconcileLock sync.Mutex
+
 	*ManagedDiskController
 	*controllerCommon
 }
@@ -1061,6 +1067,8 @@ func (az *Cloud) SetInformers(informerFactory informers.SharedInformerFactory) {
 		},
 	})
 	az.nodeInformerSynced = nodeInformer.HasSynced
+
+	az.serviceLister = informerFactory.Core().V1().Services().Lister()
 }
 
 // updateNodeCaches updates local cache for node's zones and external resource groups.
diff --git a/pkg/provider/azure_loadbalancer.go b/pkg/provider/azure_loadbalancer.go
index 706cf31df..4c55c90e6 100644
--- a/pkg/provider/azure_loadbalancer.go
+++ b/pkg/provider/azure_loadbalancer.go
@@ -31,6 +31,7 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 
 	v1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 	cloudprovider "k8s.io/cloud-provider"
@@ -144,6 +145,11 @@ func (az *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, ser
 	// When a client updates the internal load balancer annotation,
 	// the service may be switched from an internal LB to a public one, or vise versa.
 	// Here we'll firstly ensure service do not lie in the opposite LB.
+
+	// Serialize service reconcile process
+	az.serviceReconcileLock.Lock()
+	defer az.serviceReconcileLock.Unlock()
+
 	var err error
 	serviceName := getServiceName(service)
 	mc := metrics.NewMetricContext("services", "ensure_loadbalancer", az.ResourceGroup, az.getNetworkResourceSubscriptionID(), serviceName)
@@ -164,8 +170,25 @@ func (az *Cloud) EnsureLoadBalancer(ctx context.Context, clusterName string, ser
 	return lbStatus, nil
 }
 
+func (az *Cloud) getLatestService(service *v1.Service) (*v1.Service, bool, error) {
+	latestService, err := az.serviceLister.Services(service.Namespace).Get(service.Name)
+	switch {
+	case apierrors.IsNotFound(err):
+		// service absence in store means the service deletion is caught by watcher
+		return nil, false, nil
+	case err != nil:
+		return nil, false, err
+	default:
+		return latestService.DeepCopy(), true, nil
+	}
+}
+
 // UpdateLoadBalancer updates hosts under the specified load balancer.
 func (az *Cloud) UpdateLoadBalancer(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node) error {
+	// Serialize service reconcile process
+	az.serviceReconcileLock.Lock()
+	defer az.serviceReconcileLock.Unlock()
+
 	var err error
 	serviceName := getServiceName(service)
 	mc := metrics.NewMetricContext("services", "update_loadbalancer", az.ResourceGroup, az.getNetworkResourceSubscriptionID(), serviceName)
@@ -176,6 +199,17 @@ func (az *Cloud) UpdateLoadBalancer(ctx context.Context, clusterName string, ser
 		klog.V(5).InfoS("UpdateLoadBalancer Finish", "service", serviceName, "cluster", clusterName, "service_spec", service, "error", err)
 	}()
 
+	// In case UpdateLoadBalancer gets stale service spec, retrieve the latest from lister
+	service, serviceExists, err := az.getLatestService(service)
+	if err != nil {
+		return fmt.Errorf("UpdateLoadBalancer: failed to get latest service %s: %w", service.Name, err)
+	}
+	if !serviceExists {
+		isOperationSucceeded = true
+		klog.V(2).Infof("UpdateLoadBalancer: skipping service %s because service is going to be deleted", service.Name)
+		return nil
+	}
+
 	shouldUpdateLB, err := az.shouldUpdateLoadBalancer(clusterName, service, nodes)
 	if err != nil {
 		return err
@@ -203,6 +237,10 @@ func (az *Cloud) UpdateLoadBalancer(ctx context.Context, clusterName string, ser
 // have multiple underlying components, meaning a Get could say that the LB
 // doesn't exist even if some part of it is still laying around.
 func (az *Cloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName string, service *v1.Service) error {
+	// Serialize service reconcile process
+	az.serviceReconcileLock.Lock()
+	defer az.serviceReconcileLock.Unlock()
+
 	var err error
 	isInternal := requiresInternalLoadBalancer(service)
 	serviceName := getServiceName(service)
@@ -2599,7 +2637,7 @@ func (az *Cloud) shouldUpdateLoadBalancer(clusterName string, service *v1.Servic
 	}
 
 	_, _, existsLb, _ := az.getServiceLoadBalancer(service, clusterName, nodes, false, existingManagedLBs)
-	return existsLb && service.ObjectMeta.DeletionTimestamp == nil, nil
+	return existsLb && service.ObjectMeta.DeletionTimestamp == nil && service.Spec.Type == v1.ServiceTypeLoadBalancer, nil
 }
 
 func logSafe(s *string) string {


Test patch:
diff --git a/pkg/provider/azure_loadbalancer_test.go b/pkg/provider/azure_loadbalancer_test.go
index 6817543b7..9cb3418c2 100644
--- a/pkg/provider/azure_loadbalancer_test.go
+++ b/pkg/provider/azure_loadbalancer_test.go
@@ -920,7 +920,7 @@ func TestShouldReleaseExistingOwnedPublicIP(t *testing.T) {
 			expectedShouldRelease: false,
 		},
 		{
-			desc:           "existing public ip with no format properties (unit test only?), tags required by annotation, expect release",
+			desc:           "existing public ip with no format properties (unit test only?), tags required by annotation, no release",
 			existingPip:    existingPipWithNoPublicIPAddressFormatProperties,
 			lbShouldExist:  true,
 			lbIsInternal:   false,
@@ -2028,7 +2028,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules shall return corresponding probe and lbRule (slb with IPv6)",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.ServiceAnnotationLoadBalancerInternal: "true",
+				"service.beta.kubernetes.io/azure-load-balancer-internal": "true",
 			}, true, 80),
 			loadBalancerSku: "standard",
 			expectedProbes:  getDefaultTestProbes("Tcp", ""),
@@ -2037,8 +2037,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules shall return corresponding probe and lbRule (slb with HA enabled)",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.ServiceAnnotationLoadBalancerEnableHighAvailabilityPorts: "true",
-				consts.ServiceAnnotationLoadBalancerInternal:                    "true",
+				"service.beta.kubernetes.io/azure-load-balancer-enable-high-availability-ports": "true",
+				"service.beta.kubernetes.io/azure-load-balancer-internal":                       "true",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			expectedProbes:  getDefaultTestProbes("Tcp", ""),
@@ -2047,8 +2047,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules shall return corresponding probe and lbRule (slb with HA mode and SCTP)",
 			service: getTestService("test1", v1.ProtocolSCTP, map[string]string{
-				consts.ServiceAnnotationLoadBalancerEnableHighAvailabilityPorts: "true",
-				consts.ServiceAnnotationLoadBalancerInternal:                    "true",
+				"service.beta.kubernetes.io/azure-load-balancer-enable-high-availability-ports": "true",
+				"service.beta.kubernetes.io/azure-load-balancer-internal":                       "true",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			expectedRules:   getHATestRules(true, false, v1.ProtocolSCTP),
@@ -2056,8 +2056,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules shall return corresponding probe and lbRule (slb with HA enabled multi-ports services)",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.ServiceAnnotationLoadBalancerEnableHighAvailabilityPorts: "true",
-				consts.ServiceAnnotationLoadBalancerInternal:                    "true",
+				"service.beta.kubernetes.io/azure-load-balancer-enable-high-availability-ports": "true",
+				"service.beta.kubernetes.io/azure-load-balancer-internal":                       "true",
 			}, false, 80, 8080),
 			loadBalancerSku: "standard",
 			expectedProbes:  getDefaultTestProbes("Tcp", ""),
@@ -2074,7 +2074,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return tcp probe when invalid protocol is defined",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsRequestPath): "/healthy1",
+				"service.beta.kubernetes.io/port_80_health-probe_request-path": "/healthy1",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "TCP1",
@@ -2084,7 +2084,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return tcp probe when invalid protocol is defined",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsRequestPath): "/healthy1",
+				"service.beta.kubernetes.io/port_80_health-probe_request-path": "/healthy1",
 			}, false, 80),
 			loadBalancerSku: "basic",
 			probeProtocol:   "TCP1",
@@ -2094,8 +2094,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return correct rule when deprecated annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsRequestPath): "/healthy1",
-				consts.ServiceAnnotationLoadBalancerHealthProbeProtocol:                              "https",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path": "/healthy1",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-protocol":     "https",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			expectedProbes:  getDefaultTestProbes("Https", "/healthy1"),
@@ -2104,8 +2104,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return correct rule when deprecated annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsRequestPath): "/healthy1",
-				consts.ServiceAnnotationLoadBalancerHealthProbeProtocol:                              "http",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path": "/healthy1",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-protocol":     "http",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			expectedProbes:  getDefaultTestProbes("Http", "/healthy1"),
@@ -2114,8 +2114,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return correct rule when deprecated annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsRequestPath): "/healthy1",
-				consts.ServiceAnnotationLoadBalancerHealthProbeProtocol:                              "tcp",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path": "/healthy1",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-protocol":     "tcp",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			expectedProbes:  getDefaultTestProbes("Tcp", ""),
@@ -2124,7 +2124,7 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return correct rule when deprecated annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsRequestPath): "/healthy1",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path": "/healthy1",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Https",
@@ -2134,8 +2134,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should overwrite value defined in deprecated annotation when deprecated annotations and probe path are defined",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.ServiceAnnotationLoadBalancerHealthProbeRequestPath:                           "/healthy1",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsRequestPath): "/healthy2",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path": "/healthy1",
+				"service.beta.kubernetes.io/port_80_health-probe_request-path":             "/healthy2",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Https",
@@ -2145,9 +2145,9 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return error when deprecated tcp health probe annotations and protocols are added and config is not valid",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "10",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "20",
-				consts.ServiceAnnotationLoadBalancerHealthProbeProtocol:                                "https",
+				"service.beta.kubernetes.io/port_80_health-probe_interval":             "10",
+				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe":         "20",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-protocol": "https",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Tcp",
@@ -2156,9 +2156,9 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return error when deprecated tcp health probe annotations and protocols are added and config is not valid",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "10",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "20",
-				consts.ServiceAnnotationLoadBalancerHealthProbeProtocol:                                "tcp",
+				"service.beta.kubernetes.io/port_80_health-probe_interval":             "10",
+				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe":         "20",
+				"service.beta.kubernetes.io/azure-load-balancer-health-probe-protocol": "tcp",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Tcp",
@@ -2167,8 +2167,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return correct rule when health probe annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "20",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "5",
+				"service.beta.kubernetes.io/port_80_health-probe_interval":     "20",
+				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "5",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Https",
@@ -2179,8 +2179,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return correct rule when health probe annotations are added,default path should be /healthy",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "20",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "5",
+				"service.beta.kubernetes.io/port_80_health-probe_interval":     "20",
+				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "5",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Http",
@@ -2190,8 +2190,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return correct rule when tcp health probe annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "20",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "5",
+				"service.beta.kubernetes.io/port_80_health-probe_interval":     "20",
+				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "5",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Tcp",
@@ -2201,8 +2201,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return error when invalid tcp health probe annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "20",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "5a",
+				"service.beta.kubernetes.io/port_80_health-probe_interval":     "20",
+				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "5a",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Tcp",
@@ -2211,8 +2211,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return error when invalid tcp health probe annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "1",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "5",
+				"service.beta.kubernetes.io/port_80_health-probe_interval":     "1",
+				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "5",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Tcp",
@@ -2221,8 +2221,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return error when invalid tcp health probe annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "10",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "1",
+				"service.beta.kubernetes.io/port_80_health-probe_interval":     "10",
+				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "1",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Tcp",
@@ -2231,8 +2231,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 		{
 			desc: "getExpectedLBRules should return error when invalid tcp health probe annotations are added",
 			service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "10",
-				consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "20",
+				"service.beta.kubernetes.io/port_80_health-probe_interval":     "10",
+				"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "20",
 			}, false, 80),
 			loadBalancerSku: "standard",
 			probeProtocol:   "Tcp",
@@ -2262,9 +2262,9 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 	}{
 		desc: "getExpectedLBRules should expected rules when timeout are added",
 		service: getTestService("test1", v1.ProtocolTCP, map[string]string{
-			consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "10",
-			consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "10",
-			consts.ServiceAnnotationLoadBalancerIdleTimeout:                                        "5",
+			"service.beta.kubernetes.io/port_80_health-probe_interval":        "10",
+			"service.beta.kubernetes.io/port_80_health-probe_num-of-probe":    "10",
+			"service.beta.kubernetes.io/azure-load-balancer-tcp-idle-timeout": "5",
 		}, false, 80),
 		loadBalancerSku: "standard",
 		probeProtocol:   "Tcp",
@@ -2280,8 +2280,8 @@ func TestReconcileLoadBalancerRule(t *testing.T) {
 	rules1[1].Probe.ID = to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lbname/probes/atest1-TCP-34567")
 	rules1[2].Probe.ID = to.StringPtr("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Network/loadBalancers/lbname/probes/atest1-TCP-34567")
 	svc := getTestService("test1", v1.ProtocolTCP, map[string]string{
-		consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsProbeInterval): "10",
-		consts.BuildHealthProbeAnnotationKeyForPort(80, consts.HealthProbeParamsNumOfProbe):    "10",
+		"service.beta.kubernetes.io/port_80_health-probe_interval":     "10",
+		"service.beta.kubernetes.io/port_80_health-probe_num-of-probe": "10",
 	}, false, 80, 443, 421)
 	svc.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeLocal
 	svc.Spec.HealthCheckNodePort = 34567
@@ -4221,30 +4221,42 @@ func TestShouldUpdateLoadBalancer(t *testing.T) {
 	testCases := []struct {
 		desc                   string
 		lbHasDeletionTimestamp bool
+		serviceType            v1.ServiceType
 		existsLb               bool
 		expectedOutput         bool
 	}{
 		{
 			desc:                   "should update a load balancer that does not have a deletion timestamp and exists in Azure",
 			lbHasDeletionTimestamp: false,
+			serviceType:            v1.ServiceTypeLoadBalancer,
 			existsLb:               true,
 			expectedOutput:         true,
 		},
 		{
 			desc:                   "should not update a load balancer that is being deleted / already deleted in K8s",
 			lbHasDeletionTimestamp: true,
+			serviceType:            v1.ServiceTypeLoadBalancer,
+			existsLb:               true,
+			expectedOutput:         false,
+		},
+		{
+			desc:                   "should not update a load balancer that is no longer LoadBalancer type in K8s",
+			lbHasDeletionTimestamp: false,
+			serviceType:            v1.ServiceTypeClusterIP,
 			existsLb:               true,
 			expectedOutput:         false,
 		},
 		{
 			desc:                   "should not update a load balancer that does not exist in Azure",
 			lbHasDeletionTimestamp: false,
+			serviceType:            v1.ServiceTypeLoadBalancer,
 			existsLb:               false,
 			expectedOutput:         false,
 		},
 		{
 			desc:                   "should not update a load balancer that has a deletion timestamp and does not exist in Azure",
 			lbHasDeletionTimestamp: true,
+			serviceType:            v1.ServiceTypeLoadBalancer,
 			existsLb:               false,
 			expectedOutput:         false,
 		},
@@ -4253,6 +4265,7 @@ func TestShouldUpdateLoadBalancer(t *testing.T) {
 	for i, test := range testCases {
 		az := GetTestCloud(ctrl)
 		service := getTestService("test1", v1.ProtocolTCP, nil, false, 80)
+		service.Spec.Type = test.serviceType
 		setMockPublicIPs(az, ctrl, 1)
 		mockLBsClient := mockloadbalancerclient.NewMockInterface(ctrl)
 		az.LoadBalancerClient = mockLBsClient
@@ -4486,8 +4499,8 @@ func TestIsFrontendIPConfigIsUnsafeToDelete(t *testing.T) {
 			unsafe: true,
 		},
 		{
-			desc: "isFrontendIPConfigUnsafeToDelete should return true if there is a " +
-				"outbound rule referencing the frontend IP config",
+			desc: "isFrontendIPConfigUnsafeToDelete should return false if there is a " +
+				"loadBalancing rule from this service referencing the frontend IP config",
 			existingLB: &network.LoadBalancer{
 				Name: to.StringPtr("lb"),
 				LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{
@@ -4507,7 +4520,7 @@ func TestIsFrontendIPConfigIsUnsafeToDelete(t *testing.T) {
 		},
 		{
 			desc: "isFrontendIPConfigUnsafeToDelete should return false if there is a " +
-				"loadBalancing rule from this service referencing the frontend IP config",
+				"outbound rule referencing the frontend IP config",
 			existingLB: &network.LoadBalancer{
 				Name: to.StringPtr("lb"),
 				LoadBalancerPropertiesFormat: &network.LoadBalancerPropertiesFormat{