-
Notifications
You must be signed in to change notification settings - Fork 189
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
OCPBUGS-4573: Target metrics port by name in internal service #864
OCPBUGS-4573: Target metrics port by name in internal service #864
Conversation
Reformat ensureInternalIngressControllerService to follow the standard pattern for "ensure" functions. * pkg/operator/controller/ingress/controller.go (ensureIngressController): Expect a Boolean return value from ensureInternalIngressControllerService, indicating whether the service exists. * pkg/operator/controller/ingress/internal_service.go (ensureInternalIngressControllerService): Add a Boolean return value. Use a switch statement, and add a to-do comment for handling updates. (currentInternalIngressControllerService): Add a Boolean return value.
Add logic to ensureInternalIngressControllerService to update the service when an update is required. * pkg/operator/controller/ingress/internal_service.go (ensureInternalIngressControllerService): Use the new updateInternalService method to update the service as needed. (updateInternalService): Check whether the given ClusterIP service needs to be updated, and update it if so, using the new internalServiceChanged function. (managedInternalServiceAnnotations): New variable with the set of annotation keys for annotations that the operator manages for its internal router services. (internalServiceChanged): New function. Check whether the current internal service needs to be updated, and update it if it does, using the new managedInternalServiceAnnotations variable. * pkg/operator/controller/ingress/internal_service_test.go: New file. (Test_desiredInternalIngressControllerService): New test. Verify that desiredInternalIngressControllerService returns the expected service. (Test_internalServiceChanged): New test. Verify that internalServiceChanged correctly detects changes and performs updates.
@Miciah: This pull request references Jira Issue OCPBUGS-4573, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
/jira refresh |
@Miciah: This pull request references Jira Issue OCPBUGS-4573, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
/retest-required |
/assign @frobware |
/retest |
34d3af4
to
c44b72f
Compare
/retest |
/test e2e-gcp-ovn-serial |
/test e2e-gcp-ovn-serial |
Use the "metrics" port name instead of port number 1936 for the router internal service's metrics port's target. Before this commit, the router's internal service's metrics port always targeted port 1936 on the router pod. However, the router pod's metrics port can be customized, and so it is not necessarily port 1936. As a consequence, the service's metrics port didn't work when the router pod's metrics port was customized. The router pod's metrics port always has the name "metrics", so this commit changes the service to reference the port by name to avoid breaking when the port number changes. This commit fixes OCPBUGS-4573. https://issues.redhat.com/browse/OCPBUGS-4573 Follow-up to commit af653f9. * assets/router/service-internal.yaml: Use the "metrics" port name instead of port 1936 for the metrics port's target. * pkg/manifests/bindata.go: Regenerate. * pkg/operator/controller/ingress/internal_service_test.go (Test_desiredInternalIngressControllerService): Expect the metrics port to reference the "metrics" target port by name. (Test_internalServiceChanged): Add a test case for changing the "metrics" port's target port from an integer to a string.
c44b72f
to
9c6e368
Compare
/retest-required |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware 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 |
/test ? |
@candita: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use 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. |
/test e2e-gcp-ovn |
/test e2e-aws-ovn-serial |
/hold Revision 9c6e368 was retested 3 times: holding |
@Miciah: The following tests failed, say
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. |
/hold cancel |
@Miciah: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-4573 has been moved to the MODIFIED state. 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. |
/cherry-pick release-4.12 |
@Miciah: new pull request created: #910 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. |
ensureInternalIngressControllerService
: ReformatReformat
ensureInternalIngressControllerService
to follow the standard pattern for "ensure" functions.pkg/operator/controller/ingress/controller.go
(ensureIngressController
): Expect a Boolean return value fromensureInternalIngressControllerService
, indicating whether the service exists.pkg/operator/controller/ingress/internal_service.go
(ensureInternalIngressControllerService
): Add a Boolean return value. Use aswitch
statement, and add a to-do comment for handling updates.(
currentInternalIngressControllerService
): Add a Boolean return value.ensureInternalIngressControllerService
: UpdatesAdd logic to
ensureInternalIngressControllerService
to update the service when an update is required.pkg/operator/controller/ingress/internal_service.go
(ensureInternalIngressControllerService
): Use the newupdateInternalService
method to update the service as needed.(
updateInternalService
): Check whether the given ClusterIP service needs to be updated, and update it if so, using the newinternalServiceChanged
function.(
managedInternalServiceAnnotations
): New variable with the set of annotation keys for annotations that the operator manages for its internal router services.(
internalServiceChanged
): New function. Check whether the current internal service needs to be updated, and update it if it does, using the newmanagedInternalServiceAnnotations
variable.pkg/operator/controller/ingress/internal_service_test.go
: New file.(
Test_desiredInternalIngressControllerService
): New test. Verify thatdesiredInternalIngressControllerService
returns the expected service.(
Test_internalServiceChanged
): New test. Verify thatinternalServiceChanged
correctly detects changes and performs updates.service-internal.yaml
: Target metrics port by nameUse the "metrics" port name instead of port number 1936 for the router internal service's metrics port's target.
Before this change, the router's internal service's metrics port always targeted port 1936 on the router pod. However, the router pod's metrics port can be customized, and so it is not necessarily port 1936. As a consequence, the service's metrics port didn't work when the router pod's metrics port was customized. The router pod's metrics port always has the name "metrics", so this PR changes the service to reference the port by name to avoid breaking when the port number changes.
Follow-up to #694..
assets/router/service-internal.yaml
: Use the "metrics" port name instead of port 1936 for the metrics port's target.pkg/manifests/bindata.go
: Regenerate.pkg/operator/controller/ingress/internal_service_test.go
(Test_desiredInternalIngressControllerService
): Expect the metrics port to reference the "metrics" target port by name.(
Test_internalServiceChanged
): Add a test case for changing the "metrics" port's target port from an integer to a string.