Protects ensureNamespace() Against Race with Contour #19

danehans · 2020-09-02T22:03:56Z

Previously, ensureNamespace() was returning an error during the 2nd pass of contour object reconciliation. This is due to a race between the Contour object being updated by ensureFinalizer() and the namespace create operation. This PR updates the control flow of the function to fix the error case for create and delete operations.

Before:

2020-09-02T12:33:22.826-0700  INFO  controllers.Contour reconciling {"request": "default/contour-sample"}
2020-09-02T12:33:22.838-0700  INFO  controllers.Contour added finalizer to contour  {"finalizer": "contour.operator.projectcontour.io/finalizer", "namespace": "default", "name": "contour-sample"}
2020-09-02T12:33:22.948-0700  INFO  controllers.Contour created namespace {"name": "projectcontour"}
2020-09-02T12:33:22.948-0700  DEBUG controller  Successfully Reconciled {"reconcilerGroup": "operator.projectcontour.io", "reconcilerKind": "Contour", "controller": "contour", "name": "contour-sample", "namespace": "default"}
2020-09-02T12:33:22.948-0700  INFO  controllers.Contour reconciling {"request": "default/contour-sample"}
2020-09-02T12:33:22.974-0700  ERROR controller  Reconciler error  {"reconcilerGroup": "operator.projectcontour.io", "reconcilerKind": "Contour", "controller": "contour", "name": "contour-sample", "namespace": "default", "error": "failed to ensure namespace projectcontour: failed to create namespace projectcontour: namespaces \"projectcontour\" already exists"}
github.com/go-logr/zapr.(*zapLogger).Error
  /Users/daneyonhansen/code/go/pkg/mod/github.com/go-logr/[email protected]/zapr.go:128
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
  /Users/daneyonhansen/code/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:237
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
  /Users/daneyonhansen/code/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:209
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).worker
  /Users/daneyonhansen/code/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:188
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1
  /Users/daneyonhansen/code/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/wait.go:155
k8s.io/apimachinery/pkg/util/wait.BackoffUntil
  /Users/daneyonhansen/code/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/wait.go:156
k8s.io/apimachinery/pkg/util/wait.JitterUntil
  /Users/daneyonhansen/code/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/wait.go:133
k8s.io/apimachinery/pkg/util/wait.Until
  /Users/daneyonhansen/code/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/wait.go:90
2020-09-02T12:33:23.974-0700  INFO  controllers.Contour reconciling {"request": "default/contour-sample"}
2020-09-02T12:33:23.975-0700  INFO  controllers.Contour namespace exists; skipped adding  {"name": "projectcontour"}
2020-09-02T12:33:23.975-0700  DEBUG controller  Successfully Reconciled {"reconcilerGroup": "operator.projectcontour.io", "reconcilerKind": "Contour", "controller": "contour", "name": "contour-sample", "namespace": "default"}

With this PR:

2020-09-02T12:47:00.753-0700  INFO  controllers.Contour reconciling {"request": "default/contour-sample"}
2020-09-02T12:47:00.763-0700  INFO  controllers.Contour added finalizer to contour  {"finalizer": "contour.operator.projectcontour.io/finalizer", "namespace": "default", "name": "contour-sample"}
2020-09-02T12:47:00.770-0700  INFO  controllers.Contour created namespace {"name": "projectcontour"}
2020-09-02T12:47:00.770-0700  DEBUG controller  Successfully Reconciled {"reconcilerGroup": "operator.projectcontour.io", "reconcilerKind": "Contour", "controller": "contour", "name": "contour-sample", "namespace": "default"}
2020-09-02T12:47:00.770-0700  INFO  controllers.Contour reconciling {"request": "default/contour-sample"}
2020-09-02T12:47:00.770-0700  DEBUG controller  Successfully Reconciled {"reconcilerGroup": "operator.projectcontour.io", "reconcilerKind": "Contour", "controller": "contour", "name": "contour-sample", "namespace": "default"}

/cc @jpeach @Miciah

jpeach

Can you please also add the commentary from the PR into the body of the commit message? This makes life easier for our future selves; we can see the rationale for the change directly without having to follow breadcrumbs back to the PR.

controller/contour/namespace.go

Miciah · 2020-09-14T22:22:21Z

/lgtm

Miciah · 2020-09-14T22:38:49Z

On further thought... Were you consistently seeing the failure? It looks like a race condition, and I don't see how the code change avoids it (although the refactored code does look cleaner). I would be inclined to ignore "already exists" errors (just let the reconciler rerun, and the get should succeed the next time and result in a no-op), but you could check with errors.IsAlreadyExists(err) and ignore such errors.

stevesloka · 2020-09-15T00:13:17Z

controller/contour/namespace.go

- r.Log.Info("namespace exists; skipped adding", "name", ns.Name)
- return nil
- case errors.IsNotFound(err):
+ if err := r.Client.Get(ctx, types.NamespacedName{Name: ns.Name}, ns); err != nil {


Is there still a remaining case where there's an error getting the namespace, but it's different than the IsNotFound(...) method case?

@stevesloka I reorganized the conditions for both ensure namespace functions, PTAL.

danehans · 2020-09-15T18:59:14Z

@Miciah thanks for the feedback in #19 (comment). You are correct, I was hitting a race condition between the Contour object being updated by ensureFinalizer() and the namespace create operation. Commit 0c041d4 updates ensureNamespace() and ensureNamespaceRemoved() to address this race condition. PTAL and let me know if you have other ideas for addressing the race condition. cc: @stevesloka

controller/contour/namespace.go

danehans · 2020-09-21T17:48:10Z

@Miciah thanks for the review. Commit 9377428 addressees your #19 (comment).

skriss

I had a couple small suggestions for simplification but I don't consider them blocking and this can be merged as-is.

controller/contour/namespace.go

stevesloka

/lgtm

We can merge as-is like @skriss mentioned, or clean up the few nits in a new PR.

Signed-off-by: Daneyon Hansen <[email protected]>

danehans · 2020-09-23T16:06:48Z

@skriss commit 66a9e56 addresses your review feedback, PTAL.

danehans force-pushed the ns_create_fix branch from 39188a9 to e616e7c Compare September 2, 2020 22:04

danehans changed the title ~~Updates the Control Flow of ensureNamespace() Funciton~~ Improves ensureNamespace() Control Flow Sep 2, 2020

danehans force-pushed the ns_create_fix branch from e616e7c to ff6a443 Compare September 3, 2020 04:00

jpeach suggested changes Sep 4, 2020

View reviewed changes

controller/contour/namespace.go Outdated Show resolved Hide resolved

danehans force-pushed the ns_create_fix branch from ff6a443 to d70ed28 Compare September 14, 2020 21:37

stevesloka reviewed Sep 15, 2020

View reviewed changes

danehans force-pushed the ns_create_fix branch from d70ed28 to 0c041d4 Compare September 15, 2020 18:54

danehans changed the title ~~Improves ensureNamespace() Control Flow~~ Protects ensureNamespace() Against Race with Contour Sep 15, 2020

Miciah reviewed Sep 15, 2020

View reviewed changes

controller/contour/namespace.go Outdated Show resolved Hide resolved

danehans force-pushed the ns_create_fix branch from 0c041d4 to 9377428 Compare September 21, 2020 17:46

danehans requested review from jpeach and stevesloka September 21, 2020 17:51

skriss approved these changes Sep 22, 2020

View reviewed changes

controller/contour/namespace.go Outdated Show resolved Hide resolved

controller/contour/namespace.go Outdated Show resolved Hide resolved

stevesloka approved these changes Sep 23, 2020

View reviewed changes

Protects ensureNamespace() Against Race with Contour

66a9e56

Signed-off-by: Daneyon Hansen <[email protected]>

danehans force-pushed the ns_create_fix branch from 9377428 to 66a9e56 Compare September 23, 2020 16:04

danehans requested a review from stevesloka September 23, 2020 16:07

skriss approved these changes Sep 23, 2020

View reviewed changes

skriss merged commit 1585b7f into projectcontour:main Sep 23, 2020

skriss mentioned this pull request Sep 23, 2020

Implements Contour Namespace API #25

Merged

danehans deleted the ns_create_fix branch December 2, 2020 02:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protects ensureNamespace() Against Race with Contour #19

Protects ensureNamespace() Against Race with Contour #19

danehans commented Sep 2, 2020 •

edited

Loading

jpeach left a comment

Miciah commented Sep 14, 2020

Miciah commented Sep 14, 2020

stevesloka Sep 15, 2020

danehans Sep 21, 2020

danehans commented Sep 15, 2020

danehans commented Sep 21, 2020

skriss left a comment

stevesloka left a comment

danehans commented Sep 23, 2020

Protects ensureNamespace() Against Race with Contour #19

Protects ensureNamespace() Against Race with Contour #19

Conversation

danehans commented Sep 2, 2020 • edited Loading

jpeach left a comment

Choose a reason for hiding this comment

Miciah commented Sep 14, 2020

Miciah commented Sep 14, 2020

stevesloka Sep 15, 2020

Choose a reason for hiding this comment

danehans Sep 21, 2020

Choose a reason for hiding this comment

danehans commented Sep 15, 2020

danehans commented Sep 21, 2020

skriss left a comment

Choose a reason for hiding this comment

stevesloka left a comment

Choose a reason for hiding this comment

danehans commented Sep 23, 2020

danehans commented Sep 2, 2020 •

edited

Loading