Fixing namespace reconciler bugs by shashwathi · Pull Request #1128 · knative/eventing

shashwathi · 2019-04-30T23:19:25Z

Addressed few issues mentioned in #1080

Proposed Changes

produces reconciliation successs/failure events
unit test for cleaning up resources and default broker in namespace

Harwayne · 2019-04-30T23:25:20Z

I'm not sure we want to clean up the resources if the annotation is removed. That will instantaneously stop all Brokers in that namespace (by deleting a needed ServiceAccount and its RBAC permissions).

n3wscott

Awesome! And Nacho just added back the watchers on these dependents (but not the cleanup like this). Couple comments.

/cc @nachocano

n3wscott · 2019-04-30T23:27:03Z

+			FieldSelector: fmt.Sprintf("metadata.name=%s", resources.DefaultBrokerName),
+		})
+	if err != nil && !k8serrors.IsNotFound(err) {
+		logging.FromContext(ctx).Error("Error deleting broker in namespace", zap.String("brokername", resources.DefaultBrokerName), zap.Error(err))


it is an error? perhaps the user deleted it and disabled the namespace annotation?

n3wscott · 2019-04-30T23:28:09Z

+			FieldSelector: fmt.Sprintf("metadata.name=%s", resources.ServiceAccountName),
+		})
+	if serr != nil && !k8serrors.IsNotFound(serr) {
+		logging.FromContext(ctx).Error("Error deleting service account %s in namespace", zap.String("name", resources.ServiceAccountName), zap.Error(serr))


And here? perhaps not an error. Log it and try to delete the other things too.

If one resource is missing you will miss out of deleting the others.

Thats a good idea. I will update that

Harwayne · 2019-04-30T23:33:52Z

/hold

I've outlined my concerns in the linked bug (#1080).

shashwathi · 2019-05-01T23:07:12Z

@n3wscott @Harwayne I have removed the cleaning resources and updated the PR only for events and logging updates

Harwayne

/hold cancel

Harwayne · 2019-05-01T23:12:45Z

-	controllerAgentName = "knative-eventing-namespace-controller"
+	controllerAgentName       = "knative-eventing-namespace-controller"
+	namespaceReconciled       = "NamespaceReconciled"
+	namespaceReconcileFailure = "ReconcileReconcileFailure"


"ReconcileReconcileFailure" -> "NamespaceReconcileFailure"

Harwayne · 2019-05-01T23:14:06Z

 	if apierrs.IsNotFound(err) {
 		// The resource may no longer exist, in which case we stop processing.
-		logging.FromContext(ctx).Error("namespace key in work queue no longer exists")
+		logging.FromContext(ctx).Error("namespace key in work queue no longer exists", zap.String("key", key))


key is no longer needed, see #1135.

Harwayne · 2019-05-01T23:14:30Z

-		logging.FromContext(ctx).Error("Error reconciling Namespace", zap.Error(err))
-	} else {
-		logging.FromContext(ctx).Debug("Namespace reconciled")
+		logging.FromContext(ctx).Error("Error reconciling Namespace", zap.Error(err), zap.String("key", key))


key is no longer needed.

Harwayne · 2019-05-01T23:14:51Z


-	// Requeue if the resource is not ready:
-	return err
+	logging.FromContext(ctx).Debug("Namespace reconciled", zap.String("key", key))


key is no longer required.

Harwayne · 2019-05-01T23:15:42Z

 	InjectionLabelKey           = "knative-eventing-injection"
 	InjectionEnabledLabelValue  = "enabled"
 	InjectionDisabledLabelValue = "disabled"
+	ResourceInjectionLabel      = "eventing.knative.dev/namespaceInjected"


InjectedResourceLabel

shashwathi · 2019-05-01T23:21:29Z

@Harwayne : Addressed all your comments.

shashwathi · 2019-05-01T23:40:15Z

/test pull-knative-eventing-integration-tests

Harwayne · 2019-05-02T00:05:59Z

-		logging.FromContext(ctx).Error("Error reconciling Namespace", zap.Error(err))
-	} else {
-		logging.FromContext(ctx).Debug("Namespace reconciled")
+		logging.FromContext(ctx).Error("Error reconciling Namespace")


zap.Error(error) should still be here.

- produces reconciliation sucesss/failure events - unit test for default broker in namespace

knative-metrics-robot · 2019-05-02T15:43:48Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/namespace/namespace.go	Do not exist	75.0%

Harwayne

/lgtm

Harwayne

/approve

Harwayne · 2019-05-02T16:28:15Z

/test pull-knative-eventing-integration-tests

knative-prow-robot · 2019-05-02T16:28:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Harwayne, shashwathi

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Harwayne]

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

evankanderson · 2019-05-02T21:48:13Z

/retest

…

On Thu, May 2, 2019 at 9:47 AM Knative Prow Robot ***@***.***> wrote: @shashwathi <https://github.com/shashwathi>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-knative-eventing-integration-tests 61484a7 <61484a7> link <https://gubernator.knative.dev/build/knative-prow/pr-logs/pull/knative_eventing/1128/pull-knative-eventing-integration-tests/1123987606384676864/> /test pull-knative-eventing-integration-tests Full PR test history <https://gubernator.knative.dev/pr/knative_eventing/1128>. Your PR dashboard <https://gubernator.knative.dev/pr/shashwathi>. 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>. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4XEN6GICOEFUYP3BNULODPTMLKDANCNFSM4HJQ3O7Q> .

-- Evan Anderson <argent@google.com>

Harwayne · 2019-05-02T22:25:36Z

/test pull-knative-eventing-integration-tests

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 30, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 30, 2019

knative-prow-robot requested review from Harwayne and evankanderson April 30, 2019 23:19

n3wscott reviewed Apr 30, 2019

View reviewed changes

knative-prow-robot requested a review from nachocano April 30, 2019 23:30

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2019

shashwathi force-pushed the ns-recon branch 2 times, most recently from 6f69701 to 51c7dcd Compare May 1, 2019 23:05

Harwayne reviewed May 1, 2019

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2019

shashwathi force-pushed the ns-recon branch from 51c7dcd to 4781f88 Compare May 1, 2019 23:21

Harwayne reviewed May 2, 2019

View reviewed changes

Add more events and update logging in namespace reconciler

61484a7

- produces reconciliation sucesss/failure events - unit test for default broker in namespace

shashwathi force-pushed the ns-recon branch from 4781f88 to 61484a7 Compare May 2, 2019 15:31

Harwayne reviewed May 2, 2019

View reviewed changes

knative-prow-robot assigned Harwayne May 2, 2019

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

Harwayne reviewed May 2, 2019

View reviewed changes

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

knative-prow-robot merged commit 2f73f38 into knative:master May 3, 2019

shashwathi deleted the ns-recon branch May 3, 2019 00:07

lberk added a commit to lberk/eventing that referenced this pull request Mar 3, 2021

Fix typo in update-ci.sh script (knative#1128)

0bdb34c

Conversation

shashwathi commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

Harwayne commented Apr 30, 2019

Uh oh!

n3wscott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Harwayne commented Apr 30, 2019

Uh oh!

shashwathi commented May 1, 2019

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shashwathi commented May 1, 2019

Uh oh!

shashwathi commented May 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented May 2, 2019

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Harwayne commented May 2, 2019

Uh oh!

knative-prow-robot commented May 2, 2019

Uh oh!

evankanderson commented May 2, 2019 via email

Uh oh!

Harwayne commented May 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shashwathi commented Apr 30, 2019 •

edited

Loading