OCPBUGS-8271: external template and route Informer

[lance5890]

1. external template and route Informer, I think this is caused by commit ccd7b3e and some other similar commit

2. log and analysis show in this issue:
   # issue 99 cluster-policy-controller log err

[openshift-ci]

Hi @lance5890. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lance5890]

@deads2k @soltysh

[lance5890]

@atiratree @mfojtik @stlaz

[lance5890]

Anyone who has any ideas? I have modify the cluster-policy-controller code and rebuild the image, and stop the cvo，and change the release image, it does work

[lance5890]

@ingvagabund

[stlaz]

I am missing context here (even after looking into the issue). What's not working?

[lance5890]

I am missing context here (even after looking into the issue). What's not working?

1. The cluster-policy-controller log show "unable to use a shared informer for resource "template.openshift.io/v1, Resource=templates": no informer found for template.openshift.io/v1, Resource=templates" ;

2. but when I change the GenericInternalResourceInformerFunc to GenericResourceInformerFunc, and I replace the new image into the release, this err log will never show again

3. The quota.openshift.io and template.openshift.io are not internal resources, they are managed by openshift-apiserver and openshft-controller-manager, just like build.openshift.io and other openshift api resources

so we should change the GenericInternalResourceInformerFunc to GenericResourceInformerFunc, just like the build.openshift.io and other openshift api resources

[openshift-ci-robot]

@lance5890: This pull request references Jira Issue OCPBUGS-7917, which is invalid:

• expected the bug to target the "4.13.0" version, but it targets "4.11" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

1. external template and route Informer, I think this is caused by commit ccd7b3e and some other simialr commit

2. log and analysis show in this issue:
   cluster-policy-controller log err issue 99

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lance5890]

/jira refresh

[openshift-ci-robot]

@lance5890: This pull request references Jira Issue OCPBUGS-7917, which is invalid:

• expected the bug to target the "4.13.0" version, but it targets "4.11" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@lance5890: This pull request references Jira Issue OCPBUGS-7917, which is invalid:

• expected the bug to target the "4.13.0" version, but it targets "4.11" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

1. external template and route Informer, I think this is caused by commit ccd7b3e and some other simialr commit

2. log and analysis show in this issue:
   issues 99 cluster-policy-controller log err issue 99

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@lance5890: This pull request references Jira Issue OCPBUGS-7917, which is invalid:

• expected the bug to target the "4.13.0" version, but it targets "4.11" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

1. external template and route Informer, I think this is caused by commit ccd7b3e and some other simialr commit

2. log and analysis show in this issue:
   issues 99 cluster-policy-controller log err

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@lance5890: This pull request references Jira Issue OCPBUGS-7917, which is invalid:

• expected the bug to target the "4.13.0" version, but it targets "4.11" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

1. external template and route Informer, I think this is caused by commit ccd7b3e and some other simialr commit

2. log and analysis show in this issue:
   issue 99 cluster-policy-controller log err

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@lance5890: This pull request references Jira Issue OCPBUGS-7917, which is invalid:

• expected the bug to target the "4.13.0" version, but it targets "4.11" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

1. external template and route Informer, I think this is caused by commit ccd7b3e and some other simialr commit

2. log and analysis show in this issue:
   # issue 99 cluster-policy-controller log err

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ingvagabund]

/ok-to-test

[lance5890]

/retest-required

[openshift-ci-robot]

@lance5890: This pull request references Jira Issue OCPBUGS-7917, which is invalid:

• expected the bug to target the "4.13.0" version, but it targets "4.11" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

1. external template and route Informer, I think this is caused by commit ccd7b3e and some other similar commit

2. log and analysis show in this issue:
   # issue 99 cluster-policy-controller log err

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[openshift-ci-robot]

@lance5890: This pull request references Jira Issue OCPBUGS-8271, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.13.0) matches configured target version for branch (4.13.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

1. external template and route Informer, I think this is caused by commit ccd7b3e and some other similar commit

2. log and analysis show in this issue:
   # issue 99 cluster-policy-controller log err

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ingvagabund]

This looks like a reasonable fix without the knowledge why the internal version was used in the first place. Nevertheless, the fix needs at least a unit test to validate it. A test invoking NewControllerContext as the top level entry with a complete list of all resources that are watched. Checking the code this might be challenging.

@lance5890 thanks for the effort. I will need to check the code more to properly review your changes.

[lance5890]

This looks like a reasonable fix without the knowledge why the internal version was used in the first place. Nevertheless, the fix needs at least a unit test to validate it. A test invoking NewControllerContext as the top level entry with a complete list of all resources that are watched. Checking the code this might be challenging.

@lance5890 thanks for the effort. I will need to check the code more to properly review your changes.

@ingvagabund thanks for your review, that does make sense !

1. I will check if this fix should be checked by UT or by E2E
2. I have rebuilt the cluster-policy-controller image within my fix, and then stop the cvo and kube-controller-manager-operator, replace the new image into my ocp cluster, is does work

[knelasevero]

@lance5890 are you planning to follow up in this issue?

[lance5890]

@lance5890 are you planning to follow up in this issue?

yeah, could u review my PR

[knelasevero]

I mean taking care of the test that Jan mentioned before. The PR looks fine with the same reservations as Jan pointed out

[lance5890]

I mean taking care of the test that Jan mentioned before. The PR looks fine with the same reservations as Jan pointed out

i'd like to complete this issue, would u help me to finish this together?

[ingvagabund]

The code has been sitting here for a long time. For that it is not easily seen impact of renaming GenericInternalResourceInformerFunc -> GenericResourceInformerFunc. Also, CPC code could use some maintenance phase where we improve the test coverage.

[ingvagabund]

// GenericInternalResourceInformerFunc will return an internal informer for any resource matching
// its group resource, instead of the external version. Only valid for use where the type is accessed
// via generic interfaces, such as the garbage collector with ObjectMeta.
type GenericInternalResourceInformerFunc func(resource schema.GroupVersionResource) (informers.GenericInformer, error)

vs.

// genericResourceInformerFunc will handle a cast to a matching type
type GenericResourceInformerFunc func(resource schema.GroupVersionResource) (informers.GenericInformer, error)

The notable difference: GenericInternalResourceInformerFunc only valid for use where the type is accessed via generic interfaces. Only applies for Quotas and Templates. Also

// this is a temporary condition until we rewrite enough of generation to auto-conform to the required interface and no longer need the internal version shim
func (fn GenericInternalResourceInformerFunc) Start(stopCh <-chan struct{}) {}

[ingvagabund]

Comparing https://github.com/openshift/client-go/blob/master/quota/informers/externalversions/generic.go and https://github.com/openshift/client-go/blob/master/image/informers/externalversions/generic.go and other files on common both externalversions are generated using the same generators.

[ingvagabund]

openshift/origin@2778fab might help to deduce which condition was needed to GenericInternalResourceInformerFunc -> GenericResourceInformerFunc.

[ingvagabund]

Checking openshift/origin@555f87c#diff-9a553c47b0996d1d4fb15a1ea194e97c15abbe737b9fda6f05eee890ae7a4045L154 it seems GenericInternalResourceInformerFunc -> GenericResourceInformerFunc change was just forgotten :)

[ingvagabund]

@lance5890 the internal version was used long time ago before there were versioned clientset. Can you also remove definition of GenericInternalResourceInformerFunc as well? Both quota and template resources import externalversion informers already. The changes looks good.

[lance5890]

@lance5890 the internal version was used long time ago before there were versioned clientset. Can you also remove definition of GenericInternalResourceInformerFunc as well? Both quota and template resources import externalversion informers already. The changes looks good.

ok I will get a closer look at this, and repaire what u said

[openshift-ci]

@lance5890: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[ingvagabund]

/lgtm

[atiratree]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, ingvagabund, lance5890

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [atiratree]

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

[openshift-ci-robot]

@lance5890: Jira Issue OCPBUGS-8271: All pull requests linked via external trackers have merged:

• openshift/cluster-policy-controller#100

Jira Issue OCPBUGS-8271 has been moved to the MODIFIED state.

In response to this:

1. external template and route Informer, I think this is caused by commit ccd7b3e and some other similar commit

2. log and analysis show in this issue:
   # issue 99 cluster-policy-controller log err

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lance5890]

/approve

could we cherry-pick this to 4.12?

[atiratree]

+1, I think it makes to backport this
/cherry-pick release-4.13

[openshift-cherrypick-robot]

@atiratree: new pull request created: #112

In response to this:

+1, I think it makes to backport this
/cherry-pick release-4.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.