Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress #3479

ZhiminXiang · 2019-03-21T06:00:38Z

Fixes #

Proposed Changes

Introduce Finalizer into ClusterIngress
Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress

ClusterIngress

knative-prow-robot

In response to this:

Fixes #

Proposed Changes

Introduce Finalizer into ClusterIngress

Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress

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.

mattmoor · 2019-03-21T13:39:26Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

+//    finalizers:
+//    - clusteringresses.networking.internal.knative.dev
+var (
+	//


Suggested change

//

mattmoor · 2019-03-21T13:52:54Z

This LGTM, but I'm concerned that the Gateway will become a bottleneck / SPOF with this API.

IIUC every ClusterIngress is going to be adding Server entries to a single resource. This is going to fail at some scale, we should press Istio for a better API (maybe I'm wrong?).

mattmoor · 2019-03-21T13:53:42Z

Note: I'm obviously not suggestion you fix istio in this PR, leaving open for the other comment 😅

ZhiminXiang · 2019-03-21T17:52:11Z

This LGTM, but I'm concerned that the Gateway will become a bottleneck / SPOF with this API.

IIUC every ClusterIngress is going to be adding Server entries to a single resource. This is going to fail at some scale, we should press Istio for a better API (maybe I'm wrong?).

Yes, the Gateway is the single resource here. I will follow up with Istio team about this API.

vagababov · 2019-03-21T18:28:45Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

+
+	mergePatch := map[string]interface{}{
+		"metadata": map[string]interface{}{
+			"finalizers":      finalizers.List(),


does the order of the finalizers matter?

No, the order does not matter.
An object is deleted when all of the finalizers are removed. The deletion order of finalizers does not matter.

Then if it doesn't matter why do we check that our finalizer is the "first one" below?

vagababov · 2019-03-21T18:28:56Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

+	}
+
+	gatewayNames := gatewayNamesFromContext(ctx, ci)
+	logger.Info("Cleaning up Gateway Servers")


log the key?

vagababov · 2019-03-21T18:29:48Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go

@@ -410,6 +438,14 @@ func gateway(name, namespace string, servers []v1alpha3.Server) *v1alpha3.Gatewa
 	}
 }

+func patchAddFinalizerAction(ingressName, finalizer string) clientgotesting.PatchActionImpl {
+	action := clientgotesting.PatchActionImpl{}


initialize during creation.

vagababov · 2019-03-24T06:18:00Z

/lgtm
I'll let Matt approve is his concern is alleviated.
/assign @mattmoor

mattmoor · 2019-03-25T14:24:33Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

 	// If our Finalizer is first, delete the `Servers` from Gateway for this ClusterIngress,
 	// and remove the finalizer.
-	if len(ci.Finalizers) == 0 || ci.Finalizers[0] != clusterIngressFinalizer {
+	if !finalizers.Has(clusterIngressFinalizer) {


This is now incorrect. We should ONLY process our finalizer when we are "next" (first).

So if the order matters, we can't do Sets(x).List() above either.

I see. Thanks!
Now I use append instead of doing Sets(x).List() in order to keep the order of finalizers.

We also need to do the similar thing for the finalizers of Route.

…lizer

knative-metrics-robot · 2019-03-25T17:12:23Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/clusteringress/clusteringress.go	78.6%	80.8%	2.2
pkg/reconciler/v1alpha1/clusteringress/resources/gateway.go	Do not exist	96.9%

vagababov

/lgtm

ZhiminXiang · 2019-03-26T00:09:35Z

/test pull-knative-serving-upgrade-tests

mattmoor · 2019-03-26T04:10:32Z

/lgtm
/approve

knative-prow-robot · 2019-03-26T04:10:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, ZhiminXiang

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:

~~pkg/reconciler/v1alpha1/clusteringress/OWNERS~~ [mattmoor]

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

Delete Istio Gateway Servers of a ClusterIngress when deleting the

56cd504

ClusterIngress

ZhiminXiang requested review from tcnghia, vagababov and mattmoor March 21, 2019 06:00

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/API API objects and controllers area/networking size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 21, 2019

knative-prow-robot requested a review from mdemirhan March 21, 2019 06:00

knative-prow-robot reviewed Mar 21, 2019

View reviewed changes

update comment

c5ebec3

ZhiminXiang changed the title ~~[WIP] Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress~~ Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress Mar 21, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 21, 2019

Zhimin Xiang added 2 commits March 20, 2019 23:23

fix comment

e0df3fd

fix typo

5e75d53

mattmoor reviewed Mar 21, 2019

View reviewed changes

address comment

9b52623

vagababov reviewed Mar 21, 2019

View reviewed changes

Zhimin Xiang added 2 commits March 23, 2019 21:42

address comments

a3fe2b9

address comment

da12b01

knative-prow-robot assigned vagababov and mattmoor Mar 24, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2019

mattmoor reviewed Mar 25, 2019

View reviewed changes

keep the order of finalizers when adding clusteringress specific fina…

47ca756

…lizer

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2019

fix the incorrect comment

b222996

vagababov reviewed Mar 25, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2019

knative-prow-robot merged commit b00c960 into knative:master Mar 26, 2019

ZhiminXiang mentioned this pull request May 7, 2019

ClusterIngress controller reconciles ClusterIngressTLS #3052

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress #3479

Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress #3479

ZhiminXiang commented Mar 21, 2019

knative-prow-robot left a comment

mattmoor Mar 21, 2019

ZhiminXiang Mar 21, 2019

mattmoor commented Mar 21, 2019

mattmoor commented Mar 21, 2019

ZhiminXiang commented Mar 21, 2019 •

edited

Loading

vagababov Mar 21, 2019

ZhiminXiang Mar 24, 2019

vagababov Mar 24, 2019

vagababov Mar 21, 2019

ZhiminXiang Mar 24, 2019

vagababov Mar 21, 2019

ZhiminXiang Mar 24, 2019

vagababov commented Mar 24, 2019

mattmoor Mar 25, 2019

vagababov Mar 25, 2019

ZhiminXiang Mar 25, 2019

knative-metrics-robot commented Mar 25, 2019

vagababov left a comment

ZhiminXiang commented Mar 26, 2019

mattmoor commented Mar 26, 2019

knative-prow-robot commented Mar 26, 2019

Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress #3479

Clean up Istio Servers of a ClusterIngress when deleting the ClusterIngress #3479

Conversation

ZhiminXiang commented Mar 21, 2019

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor commented Mar 21, 2019

mattmoor commented Mar 21, 2019

ZhiminXiang commented Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vagababov commented Mar 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Mar 25, 2019

vagababov left a comment

Choose a reason for hiding this comment

ZhiminXiang commented Mar 26, 2019

mattmoor commented Mar 26, 2019

knative-prow-robot commented Mar 26, 2019

ZhiminXiang commented Mar 21, 2019 •

edited

Loading