Add kubelet CA to no-reboot action list/Do not drain for non-reboot actions #2398
Conversation
openshift-ci-robot
requested review from
kikisdeliveryservice and
sinnykumari
openshift-ci-robot
added
the
approved
openshift-ci-robot
added
the
do-not-merge/hold
yuqi-zhang
force-pushed
the
suppress-cert-rotation-drains
branch
from
53f1598
to
5c35983
Compare
|
After some discussion and consideration, it is likely better for us to still drain on registries.conf changes (crio reload), to surface errors quicker if the file is bad. See commit message for details. /hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
| @@ -132,8 +132,7 @@ func (dn *Daemon) performPostConfigChangeAction(postConfigChangeActions []string | |||
| if dn.recorder != nil { | |||
| dn.recorder.Eventf(getNodeRef(dn.node), corev1.EventTypeWarning, "FailedServiceReload", fmt.Sprintf("Reloading %s service failed. Error: %v", serviceName, err)) | |||
| } | |||
| dn.logSystem("Reloading %s configuration failed, node will reboot instead. Error: %v", serviceName, err) | |||
| dn.reboot(fmt.Sprintf("Node will reboot into config %s", configName)) | |||
| return fmt.Errorf("Could not apply update: reloading %s configuration failed. Error: %v", serviceName, err) | |||
There was a problem hiding this comment.
I am not convinced why we shouldn't reboot to try to auto fix the problem. These errors may not be user initiated, user will be anyway clueless and will end up creating bug report or rebooting node explicitly to fix. Since MCO has already drained the node, attempting to auto-recover by reboot makes sense and if problem still remains we will at least have logs that rebooting doesn't help and we can investigate further.
There was a problem hiding this comment.
So I have two considerations here although I do not feel super strongly either way:
- In terms of failure conditions of the reload, it could be due to
1a) a bad config provided by the user causing the reload to fail, in which case rebooting does not help anyways
1b) a transient issue with e.g. systemd, but since our logic keeps on retrying through a new sync, it will resolve itself eventually if its just a transient issue
1c) an issue that isn't transient but resolvable by a reboot. This would be covered by a reboot but I'm not sure what an example would be - Since we are saying this change is non-reboot, a customer may be confused if it rebooted (due to an error) and not necessarily have insight to what the error was. Since we make calls to systemctl elsewhere in the MCD anyways, I thought it would be consistent to display the same failure mode (returns error, logs it, but will retry upon a later sync)
There was a problem hiding this comment.
thanks Jerry for providing the reasoning, I am convinced now 🙂
This file should contain the 10-year root CA, but currently it also syncs the 1-year kube-apiserver-to-kubelet-signer cert. Remove the kube-apiserver-to-kubelet-signer cert, since it is not needed in this file.
This file syncs contents of kube-apiserver-to-kubelet-signer cert manged by the openshift-kubeapi operator. This is a 1 year cert that is auto-rotated, which normally would cause an a non user-triggered update + reboot in live clusters, which is non-desirable in many customer cases. Since the kubelet has the ability to live reload the cert when the file is changed, add this file to "None" action post config change, thus avoiding an automatic reboot every time the cert is rotated.
In 4.7 we introduced non-reboot updates for select cases, but still drained nodes for safety. This is still a major workload disruption, and non-reboot updates do not require draining for correctness. This commit will instead not drain at all for postConfigChangeActionNone updates, and in case of error, return the error directly instead of rebooting in an attempt to fix it. Note that a change to /etc/containers/registries.conf, i.e. postConfigChangeActionReloadCrio triggering a crio reload, will still cause a drain. This is to reflect registries.conf errors immediately, since if we did not perform the drain, existing containers will not attempt any new image pulls. A "bad" registries.conf then won't cause any errors until a future update, and the source of the error will be harder to find. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
yuqi-zhang
force-pushed
the
suppress-cert-rotation-drains
branch
from
0068b68
to
22da53e
Compare
|
/approve |
|
/retest did some manual testing, the behaviour is as expected |
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: darkmuggle, rphillips, sinnykumari, yuqi-zhang 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@yuqi-zhang: The following test 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. |
Incorporates #2294 and #2290, such that a kubelet cert signer rotation causes a "none" action.
In 4.7 we introduced non-reboot updates for select cases, but still drained nodes for safety. This is still a major workload disruption, and non-reboot updates do not require draining for correctness. This PR will also introduce the ability to not drain at all for non-reboot updates, and in case of error, return the error directly instead of rebooting in an attempt to fix it.
This will cause updates to effectively to not cordon nodes at all, and on AWS a change to e.g. kubelet ca would talk ~20 seconds to apply to a 3-node pool.
One last thing to consider is what behaviour we want for changes to
/etc/containers/registries.conf, since not draining may cause a mismatch of image reference between existing and new containers.