Restic controller improvements #570
Merged
Conversation
This file contains 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
Contributor
skriss
commented
Jun 21, 2018
•
skriss
commented
- various logging improvements - helpful fields, adjusting levels, removing noise
- for the PVB/PVR controllers, filter out items in the informer event handlers before adding them to the queue, to reduce noise
- refactor/simplify the code in the PVR controller that determines which PVRs are ready to process (based on the status of them and their associated pod)
skriss
force-pushed
the
restic-controller-improvements
branch
2 times, most recently
from
771d82c to
e105210
Compare
ncdc
reviewed
Jun 26, 2018
| "namespace": req.Namespace, | ||
| "name": req.Name, | ||
| }) | ||
|
|
||
| if len(req.OwnerReferences) == 1 { | ||
| log = log.WithField("arkBackup", req.OwnerReferences[0].Name) |
There was a problem hiding this comment.
We've used backup as the key with namespace/name as the value before - let's be consistent?
| for _, pvr := range pvrsToEnqueueForPod(pod, c.podVolumeRestoreLister, c.nodeName, log) { | ||
| c.enqueue(pvr) | ||
| pod, err := c.podLister.Pods(pvr.Spec.Pod.Namespace).Get(pvr.Spec.Pod.Name) | ||
| if err != nil { |
There was a problem hiding this comment.
I think we should log not-found errors at debug level, and all other errors at error level. WDYT?
| return nil | ||
| // the pod should always be for this node since the podInformer is filtered | ||
| // based on node, so this is just a failsafe. | ||
| if pod.Spec.NodeName != c.nodeName { |
| func shouldEnqueuePVR(pvr *arkv1api.PodVolumeRestore, podLister corev1listers.PodLister, nodeName string, log logrus.FieldLogger) bool { | ||
| if !shouldProcessPVR(pvr, log) { | ||
| return false | ||
| if !isResticInitContainerRunning(pod) { |
There was a problem hiding this comment.
Move this check up before the lister call (it's less expensive)?
| "namespace": req.Namespace, | ||
| "name": req.Name, | ||
| }) | ||
|
|
||
| if len(req.OwnerReferences) == 1 { | ||
| log = log.WithField("arkRestore", req.OwnerReferences[0].Name) |
|
addressed comments & added a buncha unit tests. |
|
Will squash once you get a chance to look at updates. |
ncdc
reviewed
Jun 27, 2018
| var ( | ||
| client = kubefake.NewSimpleClientset() | ||
| informers = kubeinformers.NewSharedInformerFactory(client, 0) | ||
| podInformer = informers.Core().V1().Pods() |
There was a problem hiding this comment.
You could construct the podInformer by hand instead of having the factory do it. Then you don't need the fake kube clientset.
| }, | ||
| }, | ||
| expected: true, | ||
| }, |
There was a problem hiding this comment.
Need a test case for first initContainer is our restic, but container state != running?
skriss
force-pushed
the
restic-controller-improvements
branch
from
bd54321 to
4c74b20
Compare
|
lgtm |
Signed-off-by: Steve Kriss <steve@heptio.com>
skriss
force-pushed
the
restic-controller-improvements
branch
from
4c74b20 to
a697ad1
Compare
|
squashed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.