Add resource limits to restic init container

[nrb]

Fixes #1201

• Add tests for the plugin's Execute method.

Signed-off-by: Nolan Brubaker brubakern@vmware.com

[skriss]

We should consider making these optionally configurable via the existing ConfigMap for this plugin.

[nrb]

I was thinking about that, but seeing how valid quantities in the wrong fields can break with #1678 gives me pause on the failure mode.

For example, if we allow the user to specify the limits and/or requests and they put cpu: 128Mi, it's a valid quantity string, but not for CPU. MustParse and ParseQuantity only seem to care about the string. So far I don't know that there's a way to find out about that until the pod fails to get scheduled, which I think would cause the Velero Restore to get stuck InProgress.

I'll do some more digging in the Kubernetes code to see if there's a helper function that we may have missed.

[nrb]

Added 2 test cases and the necessary helpers for creating initContainers. Let me know if this approach makes sense.

[skriss]

Approach LGTM.

Looking back at the original issue, looks like we need to add requests and limits (see the error message).

Is the 1Mi mem limit sufficient? Let's just make sure it's big enough that we won't run into issues.

[nrb]

Is the 1Mi mem limit sufficient? Let's just make sure it's big enough that we won't run into issues.

I need to actually run this against a cluster to be sure, was mostly making sure I got compilation working first.

Do you have any more thoughts about the failure mode when allowing users to configure the values?

[skriss]

Do you have any more thoughts about the failure mode when allowing users to configure the values?

Let me think about this a little more.

[skriss]

Per other PR, I think if we (a) validate that the request <= the limit for both CPU and mem, and (b) put some generous upper bounds on each (e.g. 1 CPU, 128Mi mem?), then we can allow user-configuration. If their values don't conform to these validations, we can log an error and proceed with our own defaults

[nrb]

#1577 and #1678 should be merged before this.

[carlisia]

lgtm so far 👍

[prydonius]

@nrb should I wait for this before adding flags to install for restic requests/limits (#94), since I think you wanted to move around the parseResourceRequests helper? https://github.com/heptio/velero/blob/master/pkg/cmd/cli/install/install.go#L262

[nrb]

@prydonius That'll work - I'll get this out of draft today.

[skriss]

one nit but otherwise LGTM. Have you been able to test manually?

[nrb]

@skriss So I'm trying to test it, but I think master might be in a broken state with restic right now.

We're not adding the snapshot annotations to the pod anymore, but the restore action code still aborts if it's not present. (https://github.com/heptio/velero/blob/master/pkg/restore/restic_restore_action.go#L71-L72). The pod comes back unaltered.

[skriss]

Ah, you're right, we should've continued to write the annotation in the first PR. @carlisia how's progress on #1691? Can we do anything to help?

[skriss]

We should probably document the additional ConfigMap fields here.

[nrb]

EDIT: sorry, my restic daemonset wasn't running. Running it fixes this.

I think this is currently still broken; the init container never finishes, as it doesn't find the Velero directories.

% k logs deployment/nginx-deployment -n nginx-example --container restic-wait
Not found: /restores/nginx-logs/.velero/1bdff6db-b553-11e9-9cae-42010a96001e
Not found: /restores/nginx-logs/.velero/1bdff6db-b553-11e9-9cae-42010a96001e
Not found: /restores/nginx-logs/.velero/1bdff6db-b553-11e9-9cae-42010a96001e
Not found: /restores/nginx-logs/.velero/1bdff6db-b553-11e9-9cae-42010a96001e
Not found: /restores/nginx-logs/.velero/1bdff6db-b553-11e9-9cae-42010a96001e
Not found: /restores/nginx-logs/.velero/1bdff6db-b553-11e9-9cae-42010a96001e
Not found: /restores/nginx-logs/.velero/1bdff6db-b553-11e9-9cae-42010a96001e
[....repeated...]

% k exec nginx-deployment-757ddf98bb-7xj4b  -n nginx-example --container restic-wait -it -- bash
nobody@nginx-deployment-757ddf98bb-7xj4b:/$ ls
bin  boot  dev  etc  home  lib  lib64  media  mnt  opt  proc  restores  root  run  sbin  srv  sys  tmp  usr  var  velero-restic-restore-helper
nobody@nginx-deployment-757ddf98bb-7xj4b:/$ cd restores
nobody@nginx-deployment-757ddf98bb-7xj4b:/restores$ ls
nginx-logs
nobody@nginx-deployment-757ddf98bb-7xj4b:/restores$ cd nginx-logs/
nobody@nginx-deployment-757ddf98bb-7xj4b:/restores/nginx-logs$ ls
lost+found
nobody@nginx-deployment-757ddf98bb-7xj4b:/restores/nginx-logs$ ls -al
total 24
drwxr-xr-x 3 root root  4096 Aug  2 18:27 .
drwxr-xr-x 3 root root  4096 Aug  2 18:27 ..
drwx------ 2 root root 16384 Aug  2 18:27 lost+found

I don't believe my changes caused this, but perhaps the change to the builders may have left out some info.

[nrb]

With the latest code, I'm able to restore into a namespace with a quota.

k describe quota -n nginx-example
Name:                       gke-resource-quotas
Namespace:                  nginx-example
Resource                    Used  Hard
--------                    ----  ----
count/ingresses.extensions  0     100
count/jobs.batch            0     5k
pods                        1     1500
services                    1     500


Name:          test
Namespace:     nginx-example
Resource       Used   Hard
--------       ----   ----
limits.cpu     100m   2
limits.memory  128Mi  2Gi

[nrb]

@skriss Ok, all documentation should be updated now.

[prydonius]

lgtm

[prydonius]

@skriss @nrb is this ok to merge for the beta?

[skriss]

yep - merging