Address Invalid Address in GRPC Catalogs #2499
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awgreene 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
awgreene
changed the title
|
@awgreene: This pull request references Bugzilla bug 2026343, which is invalid:
Comment In response to this:
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
bot
added
bugzilla/severity-high
timflannagan
changed the title
openshift-ci
bot
removed
the
bugzilla/severity-high
|
@awgreene: No Bugzilla bug is referenced in the title of this pull request. In response to this:
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
bot
removed
the
bugzilla/invalid-bug
|
(removed bugzilla reference from the PR title) |
awgreene
force-pushed
the
empty-service-name
branch
from
3b0607d to
614bf4a
Compare
|
/retest |
Problem: Within the catalogSource resource, the RegistryServiceStatus stores service information that is used to generate an address that OLM relies on in order to establish a connection with the associated pod. If the RegistryStatusService is not nil and is missing the namespace, name, and port information for its service, OLM is unable to recover until the catalogService's associated pod has an invalid image or spec. Solution: When reconciling a CatalogSource, OLM will now ensure that the RegistryServiceStatus of the catalogSource is valid and will update the catalogSource's status to reflect the change. Additionally, this address is stored within the status of the catalogSource within the status.GRPCConnectionState.Address field. If the address changes, OLM will update this field to reflect the new address as well. Signed-off-by: Alexander Greene <[email protected]>
awgreene
force-pushed
the
empty-service-name
branch
from
614bf4a to
7b5d764
Compare
There was a problem hiding this comment.
Looks ready to go. Only thing I'll ask is if we should add an E2E test that validates that OLM is handling this correctly from a behavior stand-point. It may not be worth it considering a lot of that is being captured by these unit test, though it is worth asking.
/lgtm
| @@ -192,7 +192,7 @@ func (c *GrpcRegistryReconciler) EnsureRegistryServer(catalogSource *v1alpha1.Ca | |||
| source := grpcCatalogSourceDecorator{catalogSource} | |||
|
|
|||
| // if service status is nil, we force create every object to ensure they're created the first time | |||
There was a problem hiding this comment.
nit: should we update this comment?
| // if service status is nil, we force create every object to ensure they're created the first time | |
| // if service status is nil, or it is invalid, we force create every object to ensure they're created the first time |
Problem: Within the catalogSource resource, the RegistryServiceStatus
stores service information that is used to generate an address that OLM
relies on in order to establish a connection with the associated pod.
If the RegistryStatusService is not nil and is missing the namespace,
name, and port information for its service, OLM is unable to recover
until the catalogService's associated pod has an invalid image or
spec.
Solution: When reconciling a CatalogSource, OLM will now ensure that
the RegistryServiceStatus of the catalogSource is valid and will update
the catalogSource's status to reflect the change. Additionally, this
address is stored within the status of the catalogSource within the
status.GRPCConnectionState.Address field. If the address changes, OLM
will update this field to reflect the new address as well.
Description of the change:
Motivation for the change:
Reviewer Checklist
/doc