From 60ff351269c27cfc8b1a29c5a4d6d5f02b144b2a Mon Sep 17 00:00:00 2001 From: Piper Dougherty Date: Wed, 21 Oct 2020 18:15:03 -0400 Subject: [PATCH] Adding fix for restic init container index on restores. (#3011) * Adding handling of restic-wait init container at any order with warning. Signed-off-by: Piper Dougherty * Adding newline at end of files to match convention. Signed-off-by: Piper Dougherty * Formatting. Signed-off-by: Piper Dougherty * Update copyright year on modified files. Signed-off-by: Piper Dougherty --- changelogs/unreleased/3011-doughepi | 2 + .../pod_volume_restore_controller.go | 37 ++++-- .../pod_volume_restore_controller_test.go | 108 +++++++++++++++++- 3 files changed, 133 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/3011-doughepi diff --git a/changelogs/unreleased/3011-doughepi b/changelogs/unreleased/3011-doughepi new file mode 100644 index 000000000..1ab34fc0a --- /dev/null +++ b/changelogs/unreleased/3011-doughepi @@ -0,0 +1,2 @@ +Allows the restic-wait container to exist in any order in the pod being restored. +Prints a warning message in the case where the restic-wait container isn't the first container in the list of initialization containers. diff --git a/pkg/controller/pod_volume_restore_controller.go b/pkg/controller/pod_volume_restore_controller.go index 3146f487e..09dd22efa 100644 --- a/pkg/controller/pod_volume_restore_controller.go +++ b/pkg/controller/pod_volume_restore_controller.go @@ -1,5 +1,5 @@ /* -Copyright 2018 the Velero contributors. +Copyright 2020 the Velero contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -155,6 +155,12 @@ func (c *podVolumeRestoreController) pvrHandler(obj interface{}) { return } + resticInitContainerIndex := getResticInitContainerIndex(pod) + if resticInitContainerIndex > 0 { + log.Warnf(`Init containers before the %s container may cause issues + if they interfere with volumes being restored: %s index %d`, restic.InitContainer, restic.InitContainer, resticInitContainerIndex) + } + log.Debug("Enqueueing") c.enqueue(obj) } @@ -174,6 +180,12 @@ func (c *podVolumeRestoreController) podHandler(obj interface{}) { return } + resticInitContainerIndex := getResticInitContainerIndex(pod) + if resticInitContainerIndex > 0 { + log.Warnf(`Init containers before the %s container may cause issues + if they interfere with volumes being restored: %s index %d`, restic.InitContainer, restic.InitContainer, resticInitContainerIndex) + } + selector := labels.Set(map[string]string{ velerov1api.PodUIDLabel: string(pod.UID), }).AsSelector() @@ -208,18 +220,21 @@ func isPodOnNode(pod *corev1api.Pod, node string) bool { } func isResticInitContainerRunning(pod *corev1api.Pod) bool { - // no init containers, or the first one is not the velero restic one: return false - if len(pod.Spec.InitContainers) == 0 || pod.Spec.InitContainers[0].Name != restic.InitContainer { - return false + + // Restic wait container can be anywhere in the list of init containers, but must be running. + i := getResticInitContainerIndex(pod) + return i >= 0 && pod.Status.InitContainerStatuses[i].State.Running != nil +} + +func getResticInitContainerIndex(pod *corev1api.Pod) int { + // Restic wait container can be anywhere in the list of init containers so locate it. + for i, initContainer := range pod.Spec.InitContainers { + if initContainer.Name == restic.InitContainer { + return i + } } - // status hasn't been created yet, or the first one is not yet running: return false - if len(pod.Status.InitContainerStatuses) == 0 || pod.Status.InitContainerStatuses[0].State.Running == nil { - return false - } - - // else, it's running - return true + return -1 } func (c *podVolumeRestoreController) processQueueItem(key string) error { diff --git a/pkg/controller/pod_volume_restore_controller_test.go b/pkg/controller/pod_volume_restore_controller_test.go index b50f8c2a7..cf4045cd0 100644 --- a/pkg/controller/pod_volume_restore_controller_test.go +++ b/pkg/controller/pod_volume_restore_controller_test.go @@ -1,5 +1,5 @@ /* -Copyright 2018 the Velero contributors. +Copyright 2020 the Velero contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -491,7 +491,7 @@ func TestIsResticContainerRunning(t *testing.T) { expected: false, }, { - name: "pod with running restic init container that's not first should return false", + name: "pod with running restic init container that's not first should still work", pod: &corev1api.Pod{ ObjectMeta: metav1.ObjectMeta{ Namespace: "ns-1", @@ -522,7 +522,7 @@ func TestIsResticContainerRunning(t *testing.T) { }, }, }, - expected: false, + expected: true, }, { name: "pod with restic init container as first initContainer that's not running should return false", @@ -598,3 +598,105 @@ func TestIsResticContainerRunning(t *testing.T) { }) } } + +func TestGetResticInitContainerIndex(t *testing.T) { + tests := []struct { + name string + pod *corev1api.Pod + expected int + }{ + { + name: "init container is not present return -1", + pod: &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns-1", + Name: "pod-1", + }, + }, + expected: -1, + }, + { + name: "pod with no restic init container return -1", + pod: &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns-1", + Name: "pod-1", + }, + Spec: corev1api.PodSpec{ + InitContainers: []corev1api.Container{ + { + Name: "non-restic-init", + }, + }, + }, + }, + expected: -1, + }, + { + name: "pod with restic container as second initContainern should return 1", + pod: &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns-1", + Name: "pod-1", + }, + Spec: corev1api.PodSpec{ + InitContainers: []corev1api.Container{ + { + Name: "non-restic-init", + }, + { + Name: restic.InitContainer, + }, + }, + }, + }, + expected: 1, + }, + { + name: "pod with restic init container as first initContainer should return 0", + pod: &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns-1", + Name: "pod-1", + }, + Spec: corev1api.PodSpec{ + InitContainers: []corev1api.Container{ + { + Name: restic.InitContainer, + }, + { + Name: "non-restic-init", + }, + }, + }, + }, + expected: 0, + }, + { + name: "pod with restic init container as first initContainer should return 0", + pod: &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns-1", + Name: "pod-1", + }, + Spec: corev1api.PodSpec{ + InitContainers: []corev1api.Container{ + { + Name: restic.InitContainer, + }, + { + Name: "non-restic-init", + }, + }, + }, + }, + expected: 0, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.expected, getResticInitContainerIndex(test.pod)) + }) + } +}