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.

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

23 informers deliverable #314

Merged
merged 9 commits into from
Nov 10, 2021
Merged

23 informers deliverable #314

merged 9 commits into from
Nov 10, 2021

Conversation

emmjohnson
Copy link
Contributor

No description provided.

@netlify
Copy link

netlify bot commented Nov 9, 2021

✔️ Deploy Preview for elated-stonebraker-105904 canceled.

🔨 Explore the source changes: 7f1aa7f

🔍 Inspect the deploy log: https://app.netlify.com/sites/elated-stonebraker-105904/deploys/618c21c08b98b00007d2cdb0

@emmjohnson emmjohnson requested a review from squeedee November 9, 2021 02:44
Copy link
Member

@squeedee squeedee left a comment

Choose a reason for hiding this comment

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

We shouldn't review this until #304 lands

Also we've got a flake that should have died by now, will investigate:
Screen Shot 2021-11-08 at 11 50 09 PM

@squeedee squeedee force-pushed the 23-informers-deliverable branch from 723c1f8 to 7482de3 Compare November 9, 2021 17:13
emmjohnson and others added 8 commits November 9, 2021 15:18
- Backofftests made sense when we used errors and timeouts to ensure
  reconcile was called again.
- Now when a reconcile error occurs, backoff will be shortcircuited
  by informers. Testing informers work correctly is all we need.

[#23]

Co-authored-by: Emily Johnson <emjohnson@vmware.com>
- We're dependent on APIServer + our countroller resolving this stamp
  in under a second, which just doesn't always happen.

Co-authored-by: Emily Johnson <emjohnson@vmware.com>
[#23]

Co-authored-by: Emily Johnson <emjohnson@vmware.com>
@squeedee squeedee force-pushed the 23-informers-deliverable branch from 0ffe53c to 7ccd223 Compare November 9, 2021 20:22
@martyspiewak martyspiewak self-requested a review November 10, 2021 15:49
})
if err != nil {
return fmt.Errorf("controller new: %w", err)
}

reconciler.AddTracking(&external.ObjectTracker{
Copy link
Contributor

Choose a reason for hiding this comment

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

one implicit detail of using external.ObjectTracker is that one can actually "pause the watching" by making use of clusterapi's paused annotation:

          // PausedAnnotation is an annotation that can be applied to any Cluster API
          // object to prevent a controller from processing a resource.
          //
          // Controllers working with Cluster API objects must check the existence of this annotation
          // on the reconciled object.
          PausedAnnotation = "cluster.x-k8s.io/paused"

used as a predicate in

          log.Info("Adding watcher on external object", "GroupVersionKind", gvk.String())
          err := o.Controller.Watch(
                  &source.Kind{Type: u},
                  handler,
                  predicates.ResourceNotPaused(log),
          )

(see https://github.com/kubernetes-sigs/cluster-api/blob/6abfc64139ade5a225ba62d675117c510885f1e2/controllers/external/tracker.go#L56-L60)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting...

@emmjohnson emmjohnson force-pushed the 23-informers-deliverable branch from d935c75 to 7f1aa7f Compare November 10, 2021 19:47
@emmjohnson emmjohnson merged commit 5df90e6 into main Nov 10, 2021
@emmjohnson emmjohnson deleted the 23-informers-deliverable branch November 10, 2021 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants