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

Remove ImageStreamReferenceIndex from BuildInformer #14635

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented Jun 14, 2017

The ImageStreamReferenceIndex doesn't apply to builds. Removes that index from the builds informer.

@csrwng
Copy link
Contributor Author

csrwng commented Jun 14, 2017

@bparees this was the issue with the index. I copied this code from the BuildConfig informer and included an index that would always return an error for builds. Because this index function returned an error, if it was the first index to be processed for a particular build, the other index for the namespace wasn't getting processed either. In upstream, maybe the right behavior would be to log the error and continue instead of returning:
https://github.com/openshift/origin/blob/master/vendor/k8s.io/kubernetes/staging/src/k8s.io/client-go/tools/cache/thread_safe_store.go#L252
/cc @deads2k
The result is that we were getting some entries in the cache that were not in the namespace index

@csrwng
Copy link
Contributor Author

csrwng commented Jun 14, 2017

[test]

@deads2k
Copy link
Contributor

deads2k commented Jun 14, 2017

In upstream, maybe the right behavior would be to log the error and continue instead of returning:

Since there is already a lack of compensation for partially inserted keys, aggregating all the errors together seems reasonable. I'd probably suggest logging it using utilruntime.HandleError higher up the stack.

@deads2k
Copy link
Contributor

deads2k commented Jun 14, 2017

Oh, also, you should switch to the generated informers.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to e65b77b

@bparees
Copy link
Contributor

bparees commented Jun 14, 2017

Oh, also, you should switch to the generated informers.

yeah my understanding is @smarterclayton is planning to do that wholesale for all our controllers shortly.

@bparees
Copy link
Contributor

bparees commented Jun 14, 2017

[testextended][extended:core(builds)]

@bparees
Copy link
Contributor

bparees commented Jun 14, 2017

@csrwng do you want to include re-enabling the cache for pruning w/ this PR then?

@bparees bparees self-assigned this Jun 14, 2017
@csrwng
Copy link
Contributor Author

csrwng commented Jun 14, 2017

@csrwng do you want to include re-enabling the cache for pruning w/ this PR then?

I'd rather not have too many dependencies. This PR can go in without requiring the other ones.
I'll create a separate one for re-enabling the cache.

@bparees
Copy link
Contributor

bparees commented Jun 14, 2017

I'd rather not have too many dependencies. This PR can go in without requiring the other ones.
I'll create a separate one for re-enabling the cache.

k

@bparees
Copy link
Contributor

bparees commented Jun 14, 2017

lgtm pending passing extended tests

@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to e65b77b

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2208/) (Base Commit: 653b218)

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/625/) (Base Commit: 653b218) (Extended Tests: core(builds))

@csrwng
Copy link
Contributor Author

csrwng commented Jun 14, 2017

extended test failures are known flakes
[merge][severity:bug]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 14, 2017

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 6

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to e65b77b

@smarterclayton smarterclayton merged commit c323526 into openshift:master Jun 15, 2017
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.

5 participants