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

Route controller ignoring config updates due to missing TypeMeta from informer #2372

Closed
jonjohnsonjr opened this issue Oct 31, 2018 · 3 comments
Assignees
Labels
area/API API objects and controllers area/networking kind/bug Categorizes issue or PR as related to a bug.

Comments

@jonjohnsonjr
Copy link
Contributor

Expected Behavior

The Route controller should be reconciling on changes to Configurations.

Actual Behavior

The Route controller does not reconcile some Configuration changes because the change has a different ObjectReference representation in the tracker.

Steps to Reproduce the Problem

  1. Create a RunLatest service
  2. Upgrade knative/serving
  3. Update the service to point to a new image

Additional Info

We use pkg/tracker to Track Configurations and Revisions that are referred to by Routes. The tracker ignores any updates for objects that we aren't tracking.

I believe the root cause here is that the configInformer's UpdateFunc and AddFunc are being called with different values, one missing TypeMeta. Instrumenting the tracker to print what's coming into OnChanged yields (at different times) both:

  • OnChanged: {Configuration serving-tests pizzaplanet-upgrade-service serving.knative.dev/v1alpha1 }
  • OnChanged: { serving-tests pizzaplanet-upgrade-service }

Note that the second value is missing APIVersion and Kind, thus they are different keys in the tracker's map, thus we miss updates from one.

I added some logging to when we track Configurations, and when the controller first comes up, I see:

{
  "level": "info",
  "ts": "2018-10-31T22:04:10.204Z",
  "logger": "controller.route-controller",
  "caller": "route/route.go:268",
  "msg": "Tracked: {Configuration serving-tests pizzaplanet-upgrade-service  serving.knative.dev/v1alpha1  }",
  "knative.dev/controller": "route-controller",
  "knative.dev/key": "serving-tests/pizzaplanet-upgrade-service"
}

So we will ignore any updates that are missing TypeMeta.

7 minutes later (triggered by a config-gc change it seems??), we see:

{
  "level": "info",
  "ts": "2018-10-31T22:11:21.012Z",
  "logger": "controller.configuration-controller.config-store",
  "caller": "configmap/store.go:166",
  "msg": "configuration config \"config-gc\" config was added or updated: &{24h0m0s 15h0m0s 1 5h0m0s}",
  "knative.dev/controller": "configuration-controller"
}
...
{
  "level": "info",
  "ts": "2018-10-31T22:11:21.018Z",
  "logger": "controller.route-controller",
  "caller": "route/route.go:268",
  "msg": "Tracked: { serving-tests pizzaplanet-upgrade-service    }",
  "knative.dev/controller": "route-controller",
  "knative.dev/key": "serving-tests/pizzaplanet-upgrade-service"
}

So we start finally tracking the un-TypeMeta'd version, which then causes the reconcile, and the route starts working again.

@knative-prow-robot knative-prow-robot added area/API API objects and controllers kind/bug Categorizes issue or PR as related to a bug. labels Oct 31, 2018
@mattmoor
Copy link
Member

mattmoor commented Nov 1, 2018

/area networking

@mattmoor
Copy link
Member

mattmoor commented Nov 1, 2018

/assign

@mattmoor
Copy link
Member

mattmoor commented Nov 1, 2018

@jonjohnsonjr also pointed me at this issue offline: kubernetes/apiextensions-apiserver#29

mattmoor added a commit to mattmoor/serving that referenced this issue Nov 1, 2018
In the Route controller, we were relying on accessors on TypeMeta to tell us the
GroupVersionKind of Configurations and Revisions so that we could construct an
ObjectReference to them with slightly less boilerplate.  It turns out, however,
that the presence of TypeMeta is
[unreliable](kubernetes/apiextensions-apiserver#29) and
that we should instead simply synthesize these directly.

Fixes: knative#2372
mattmoor added a commit to mattmoor/serving that referenced this issue Nov 1, 2018
In the Route controller, we were relying on accessors on TypeMeta to tell us the
GroupVersionKind of Configurations and Revisions so that we could construct an
ObjectReference to them with slightly less boilerplate.  It turns out, however,
that the presence of TypeMeta is
[unreliable](kubernetes/apiextensions-apiserver#29) and
that we should instead simply synthesize these directly.

Fixes: knative#2372
mattmoor added a commit to mattmoor/serving that referenced this issue Nov 1, 2018
In the Route controller, we were relying on accessors on TypeMeta to tell us the
GroupVersionKind of Configurations and Revisions so that we could construct an
ObjectReference to them with slightly less boilerplate.  It turns out, however,
that the presence of TypeMeta is
[unreliable](kubernetes/apiextensions-apiserver#29) and
that we should instead simply synthesize these directly.

Fixes: knative#2372
knative-prow-robot pushed a commit that referenced this issue Nov 1, 2018
In the Route controller, we were relying on accessors on TypeMeta to tell us the
GroupVersionKind of Configurations and Revisions so that we could construct an
ObjectReference to them with slightly less boilerplate.  It turns out, however,
that the presence of TypeMeta is
[unreliable](kubernetes/apiextensions-apiserver#29) and
that we should instead simply synthesize these directly.

Fixes: #2372
mattmoor added a commit to mattmoor/serving that referenced this issue Nov 1, 2018
In the Route controller, we were relying on accessors on TypeMeta to tell us the
GroupVersionKind of Configurations and Revisions so that we could construct an
ObjectReference to them with slightly less boilerplate.  It turns out, however,
that the presence of TypeMeta is
[unreliable](kubernetes/apiextensions-apiserver#29) and
that we should instead simply synthesize these directly.

Fixes: knative#2372
mchmarny pushed a commit that referenced this issue Nov 2, 2018
* Don't rely on TypeMeta to be properly populated. (#2373)

In the Route controller, we were relying on accessors on TypeMeta to tell us the
GroupVersionKind of Configurations and Revisions so that we could construct an
ObjectReference to them with slightly less boilerplate.  It turns out, however,
that the presence of TypeMeta is
[unreliable](kubernetes/apiextensions-apiserver#29) and
that we should instead simply synthesize these directly.

Fixes: #2372

* Return error on lastPinned update failures (#2333)

As is we only log an error in this case. Fail the reconciler to ensure
we retry on failures as revisions will otherwise be GCd incorrectly.

* Guard every connection access with a non-nil check. (#2361)
@mattmoor mattmoor moved this from Proposed to 0.2.1 in Serving 0.2 Cherrypicks Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers area/networking kind/bug Categorizes issue or PR as related to a bug.
Projects
No open projects
Development

No branches or pull requests

3 participants