Bug 1709958: Use graceful shutdown for router#240
Closed
Miciah wants to merge 1 commit intoopenshift:masterfrom
Closed
Bug 1709958: Use graceful shutdown for router#240Miciah wants to merge 1 commit intoopenshift:masterfrom
Miciah wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
size/L
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
danehans
reviewed
May 20, 2019
| for _, tc := range testCases { | ||
| nineteen := int32(19) | ||
| fourTwenty := int32(420) // = 0644 octal. | ||
| thirty := int64(30) |
Contributor
There was a problem hiding this comment.
Is thirty expected to match the termination grace period of openshift/router#28? If so, should this be documented here?
Contributor
Author
There was a problem hiding this comment.
I only chose it because it's the default value, which means that it is the value that the operator will see on an existing ingress controller's deployment when the operator is upgraded. It really could be any value (including nil), as long as the "if the deployment terminationGracePeriodSeconds is changed" test case sets a distinct value.
Miciah
force-pushed
the
BZ1709958-use-graceful-shutdown-for-router
branch
5 times, most recently
from
2d6fcbf to
c6b5a68
Compare
openshift-ci-robot
added
size/XL
Miciah
force-pushed
the
BZ1709958-use-graceful-shutdown-for-router
branch
from
c6b5a68 to
c751959
Compare
openshift-ci-robot
added
the
needs-rebase
Miciah
force-pushed
the
BZ1709958-use-graceful-shutdown-for-router
branch
from
c751959 to
034d05c
Compare
openshift-ci-robot
removed
the
needs-rebase
Miciah
force-pushed
the
BZ1709958-use-graceful-shutdown-for-router
branch
from
034d05c to
f7ed23c
Compare
Configure the router deployment to ensure graceful shutdown: * Set a lifecycle pre-stop handler that runs the haproxy-router image's shutdown-haproxy script. * Set a 120-second termination grace period. * Create a pod disruption budget with a maximum of 25% unavailable pods. * Create a NodePort service for the router pod's healthz endpoint, and use that NodePort service for the LoadBalancer service's health-check node-port. This commit is related to bug 1709958. https://bugzilla.redhat.com/show_bug.cgi?id=1709958 * assets/router/deployment.yaml: Use /healthz/live for the liveness probe and /healthz/ready for the readiness probe. * manifests/00-cluster-role.yaml: Allow the operator to manage PodDisruptionBudget resources. * pkg/manifests/bindata.go: Regenerate. * pkg/operator/controller/controller.go (ensureIngressController): Call ensureHealthzService. Pass healthz service to ensureLoadBalancerService. Call ensureRouterPodDisruptionBudget. * pkg/operator/controller/controller_healthz_service.go: New file. (ensureHealthzService): New method. Ensure a healthz NodePort service exists for the given ingress controller if needed. Get the current one using currentHealthzService. If one needs to be created, create one using the desiredHealthzService function and createHealthzService method. If one already exists and none is desired, delete it using deleteHealthzService. (currentHealthzService): New method. (desiredHealthzService): New function. (createHealthzService): New method. (deleteHealthzService): New method. * pkg/operator/controller/controller_internal_service.go: (ensureInternalIngressControllerService): Add a return value indicating whether the service exists. Use the new createInternalService and deleteInternalService methods in order to handle creation and deletion. (currentInternalIngressControllerService): Rename... (currentInternalService): ...to this. Add a return value indicating whether a service exists. (desiredInternalIngressControllerService): Add a return value indicating whether a service is desired. (createInternalService): New method. (deleteInternalService): New method. * pkg/operator/controller/controller_lb.go (ensureLoadBalancerService): Add parameters for specifying whether a NodePort healthz service should be used for the LoadBalancer service. Add a return value indicating whether the service exists. Really return the current service, even in error cases, in order to be consistent with the godoc. Return an error if a healthz service should be used but the provided one has no port. Pass the healthz service node port to desiredLoadBalancerService. Use the new createLoadBalancerService, deleteLoadBalancerService, and updateLoadBalancerService methods in order to handle creation, updating (in particular for .spec.healthCheckNodePort), and deletion. (loadBalancerServiceName): Rename, and move to names.go. (desiredLoadBalancerService): Add a parameter for healthz port. If it is provided, use the parameter value for the LoadBalancer service's HealthCheckNodePort. Add a return value indicating whether a service is desired. (currentLoadBalancerService): Add a return value indicating whether a service exists. (createLoadBalancerService): New method. (deleteLoadBalancerService): New method. (updateLoadBalancerService): New method. Use loadBalancerServicesEqual to determine whether an update is needed. (loadBalancerServicesEqual): New function. For now, only check .spec.healthCheckNodePort. * pkg/operator/controller/controller_router_deployment.go (desiredRouterDeployment): Set a pre-stop handler. Set a grace period of 120 seconds. (deploymentConfigChanged): Compare the current and expected container lifecycle handlers, pod termination grace period, and liveness and readiness probes, and set them in the updated deployment. * pkg/operator/controller/controller_router_deployment_test.go (TestDeploymentConfigChanged): Add test cases for container lifecycle handlers, pod termination grace period, and liveness and readiness probes. * pkg/operator/controller/names.go (RouterPodDisruptionBudgetName): (HealthzServiceName): New functions. (LoadBalancerServiceName): Moved from controller_controller_lb.go. * pkg/operator/controller/router_poddisruptionbudget.go: New file. (ensureRouterPodDisruptionBudget): New method. Ensure the appropriate pod disruption budget exists for the given ingress controller. Get the current one using currentRouterPodDisruptionBudget. If none exists, create one using the desiredRouterPodDisruptionBudget function and createRouterPodDisruptionBudget method. If one already exists, update it if necessary using updateRouterPodDisruptionBudget, or delete it using deleteRouterPodDisruptionBudget if none is desired. (desiredRouterPodDisruptionBudget): New function. (currentRouterPodDisruptionBudget): New method. (createRouterPodDisruptionBudget): New method. (deleteRouterPodDisruptionBudget): New method. (updateRouterPodDisruptionBudget): New method. Use podDisruptionBudgetChanged to determine whether an update is needed. (podDisruptionBudgetChanged): New function.
Miciah
force-pushed
the
BZ1709958-use-graceful-shutdown-for-router
branch
from
f7ed23c to
863a6c4
Compare
Contributor
|
@Miciah: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. |
Contributor
|
@Miciah: PR needs rebase. DetailsInstructions 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
added
the
needs-rebase
Contributor
Author
|
/close |
Contributor
|
@Miciah: Closed this PR. DetailsIn 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Configure the router deployment to ensure graceful shutdown:
Set a lifecycle pre-stop handler that runs the haproxy-router image's
shutdown-haproxyscript.Set a 90-second termination grace period.
Create a pod disruption budget with a maximum of 25% unavailable pods.
manifests/00-cluster-role.yaml: Allow the operator to managePodDisruptionBudgetresources.pkg/operator/controller/controller.go(ensureIngressController): CallensureRouterPodDisruptionBudget.pkg/operator/controller/controller_router_deployment.go(desiredRouterDeployment): Set a pre-stop handler. Set a grace period of 90 seconds.(
deploymentConfigChanged): Compare the current and expected container lifecycle handlers and pod termination grace period, and set them in the updated deployment.pkg/operator/controller/controller_router_deployment_test.go(TestDeploymentConfigChanged): Add test cases for container lifecycle handlers and pod termination grace period.pkg/operator/controller/names.go(RouterPodDisruptionBudgetName): New function.pkg/operator/controller/router_poddisruptionbudget.go: New file.(
ensureRouterPodDisruptionBudget): New method. Ensure the appropriate pod disruption budget exists for the given ingress controller. Get the current one usingcurrentRouterPodDisruptionBudget. If none exists, create one using thedesiredRouterPodDisruptionBudgetfunction andcreateRouterPodDisruptionBudgetmethod. If one already exists, update it if necessary usingupdateRouterPodDisruptionBudget.(
desiredRouterPodDisruptionBudget): New function.(
currentRouterPodDisruptionBudget): New method.(
createRouterPodDisruptionBudget): New method.(
updateRouterPodDisruptionBudget): New method. UsepodDisruptionBudgetChangedto determine whether an update is needed.(
podDisruptionBudgetChanged): New function.Depends on openshift/router#28.