Support restart task for statefulsets and daemonsets #836
Conversation
timothysmith0609
changed the title
| containers: containers.map do |container| | ||
| { | ||
| name: container.name, | ||
| env: [{ name: "RESTARTED_AT", value: Time.now.to_i.to_s }], |
There was a problem hiding this comment.
I found an internal reference to this variable here.
There was a problem hiding this comment.
This looks like perhaps it was cargo culted from here since they're using it to simply trigger a restart in this case (and they don't particularly care about reading its value). I'm tempted to change the implementation in the buddy to mimic this one (changing annotations on the template metadata). IMO having these processes/controllers mounting variables directly into containers doesn't strike me as the wisest idea.
| @@ -451,7 +451,7 @@ resource to deploy. | |||
|
|
|||
| # krane restart | |||
|
|
|||
| `krane restart` is a tool for restarting all of the pods in one or more deployments. It triggers the restart by touching the `RESTARTED_AT` environment variable in the deployment's podSpec. The rollout strategy defined for each deployment will be respected by the restart. | |||
| `krane restart` is a tool for restarting all of the pods in one or more deployments, statefuls sets, and/or daemon sets. It triggers the restart by patching template metadata with the `kubectl.kubernetes.io/restartedAt` annotation (with the value being an RFC 3339 representation of the current time). Note this is the manner in which `kubectl rollout restart` itself triggers restarts. | |||
There was a problem hiding this comment.
👍 Indeed RFC 3339 is used by kubectl too.
| @logger.info("Configured to restart all deployments with the `#{ANNOTATION}` annotation") | ||
| apps_v1_kubeclient.get_deployments(namespace: @namespace) | ||
| def identify_target_workloads(deployment_names, statefulset_names, daemonset_names, selector: nil) | ||
| if deployment_names.blank? && statefulset_names.blank? && daemonset_names.blank? |
There was a problem hiding this comment.
Curious why blank? vs nil? Are we passing empty strings in one of the paths? Looks like the default param in run! is an empty list now instead of nil?
There was a problem hiding this comment.
While the default arg is the empty list, I can't guarantee that someone won't explicitly pass nil at some point. Using blank? seems preferable over empty? or nil? since it will catch either case.
| @@ -212,7 +242,7 @@ def test_restart_failure | |||
| "command" => [ | |||
| "/bin/sh", | |||
| "-c", | |||
| "test $(env | grep -s RESTARTED_AT -c) -eq 0", | |||
| "test $(cat /etc/podinfo/annotations | grep -s kubectl.kubernetes.io/restartedAt -c) -eq 0", | |||
| @@ -22,6 +22,8 @@ def initialize(deployment_name, response) | |||
| HTTP_OK_RANGE = 200..299 | |||
| ANNOTATION = "shipit.shopify.io/restart" | |||
|
|
|||
| RESTART_TRIGGER_ANNOTATION = "kubectl.kubernetes.io/restartedAt" | |||
There was a problem hiding this comment.
Is there any specific benefit to reusing the same annotation name as kubectl or is this done just to reduce annotation clutter?
There was a problem hiding this comment.
I'm fairly ambivalent about which one to use; this one cuts down noise, but can also be construed as somewhat misleading. I'm tempted to leave it as-is and change it if we get some complaints
timothysmith0609
requested review from
peiranliushop
and removed request for
RyanBrushett
timothysmith0609
deleted the
support_deploys_and_statefulesets_and_daemonsets_for_restart_task
branch
Closes #835
Closes #610
Adds support to the restart task for StatefulSets and DaemonSets. To maintain backwards compatibility, and simplify some implementation, this adds separate flags for
statefulsetsanddaemonsets.This ended up being a bigger PR than I thought since the assumption that only deploys would be restart targets was fairly embedded in the code. Because of this, it was more straightforward to duplicate some parts of the code (e.g.
#fetch_deployments|statefulsets|daemonsets, etc.). In addition, the old code directly altered an ENV var (RESTARTED_AT) of a pod's container to trigger a restart. I've switched this to use the convention used bykubectl, which is to add an annotation inspec.template.metadataof the target resource.I still need to add some tests to ensure StatefulSets and DaemonSets are safe against regressions, but I've tophatted it enough to open this up for review, now.
List of changes: