Update Kubernetes and Client-Go for 1.11.0 / 8.0.0

[marpaia]

This PR contains my run at updating the revs for the Kubernetes dependencies. I had to do some refactoring to use the new dynamic client interface, which is the most noteworthy change to the Ark code in my opinion.

[ncdc]

I don't think this is required. We ran into this problem when we were updating the generated code when we didn't have k8s.io/api available on the GOPATH. How did you regenerate things? The kubernetes-1.11.x tags for k8s.io/code-generator now allow us to run the generate groups shell script from anywhere (i.e. within ark), so we can remove the k8s.io/api bind mount (see https://github.com/heptio/ark/blob/32907931e13f38ef4e055652245eeb78d20ac76e/Makefile#L101-L102).

[marpaia]

Yeah! So, hack/update-generated-crd-code.sh calls $GOPATH/src/k8s.io/code-generator/generate-groups.sh which also requires k8s.io/apimachinery to be in the GOPATH as well. This is described in this issue: kubernetes/code-generator#21

I have some other projects that use code generation and I add a required stanza to my Gopkg.toml for the k8s.io/code-generator repo so that we're not relying on out-of-tree code. I tried to set this up for Ark but that would be complicated for y'all because you prune non-go files, which prunes out the scripts.

When I did run the generator, however, both code-generator and apimachinery were checked out to the release-1.11 branch.

[marpaia]

I had to go to dinner last night before I could get this to work, but I will definitely circle back soon.

[ncdc]

If you run ‘make update’ it updates everything in a docker container with the right versions of everything.

…

On Sat, Jun 30, 2018 at 1:39 PM Mike Arpaia ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/apis/ark/v1/pod_volume_backup.go <#634 (comment)>: > @@ -27,7 +27,7 @@ type PodVolumeBackupSpec struct { Node string `json:"node"` // Pod is a reference to the pod containing the volume to be backed up. - Pod corev1api.ObjectReference `json:"pod"` + Pod *corev1api.ObjectReference `json:"pod"` Yeah! So, hack/update-generated-crd-code.sh calls $GOPATH/src/ k8s.io/code-generator/generate-groups.sh which also requires k8s.io/apimachinery to be in the GOPATH as well. This is described in this issue: kubernetes/code-generator#21 <kubernetes/code-generator#21> I have some other projects that use code generation and I add a required stanza to my Gopkg.toml for the k8s.io/code-generator repo so that we're not relying on out-of-tree code. I tried to set this up for Ark but that would be complicated for y'all because you prune non-go files, which prunes out the scripts. When I did run the generator, however, both code-generator and apimachinery were checked out to the release-1.11 branch. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#634 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABYvaP5Yq2-0iXVftBqjBc3GljrzZJks5uB7fIgaJpZM4U9sHm> .

[ncdc]

@marpaia thanks for doing this! We're working to close out 0.9.0, then we'll pick up reviewing this. When you get a chance (no rush!), please split out changes to Gopkg.* and vendor to a separate commit from changes to the Ark code. Thanks!

[skriss]

Just one minor comment, otherwise LGTM.

[skriss]

we already have a [[constraint]] for this package under Cloud provider packages - can you just update the constraint to specify this revision rather than adding an override?

[marpaia]

I removed the constraint below. I believe this needs to be an override instead of a constraint because we need to override the version used in k8s.io/client-go.

[skriss]

We may also be able to simplify pkg/client/dynamic.go now - IIRC we created a factory/client wrapper in order to be able to fake for unit testing, which may be easier with the updated client. That can be addressed separately, though - I'll add an issue for it.

[ncdc]

We created it because the upstream client had some developer UX issues. The new client is much better and we shouldn't have to provider a helper any more, hopefully.

[skriss]

@nrb can you take a look at this too? I'm fine merging as-is and can follow up to address my comments.

[skriss]

Thanks @marpaia. Could you add DCO signoff to that last commit? Or squash it into a previous one.

[marpaia]

Woops, sorry about that :)

[nrb]

Testing this branch out, I'm seeing backup failures with a simple example that I don't see with master.

I run ark backup create nginx --include-namespaces nginx-example.

I see this in the server logs:

INFO[0096] Backup completed                              backup=heptio-ark/nginx logSource="pkg/controller/backup_controller.go:404"
ERRO[0097] backup failed                                 error="the server could not find the requested resource" key=heptio-ark/nginx logSource="pkg/controller/backup_controller.go:280"

End of backup logs:
ark backup logs nginx

time="2018-07-13T11:30:40-04:00" level=info msg="Backup completed with errors: the server could not find the requested resource" backup=heptio-ark/nginx logSource="pkg/backup/backup.go:302"

I'll do some more debugging on this, since Steve didn't see this behavior.

[skriss]

@nrb what version's your cluster? Do you not get this error if running v0.9.0 in this same cluster?

[nrb]

Kube versions:

Client Version: version.Info{Major:"1", Minor:"9", GitVersion:"v1.9.1", GitCommit:"3a1c9449a956b6026f075fa3134ff92f7d55f812", GitTreeState:"clean", BuildDate:"2018-01-04T11:52:23Z", GoVersion:"go1.9.2", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"9+", GitVersion:"v1.9.7-gke.3", GitCommit:"9b5b719c5f295c99de68ffb5b63101b0e0175376", GitTreeState:"clean", BuildDate:"2018-05-31T18:32:23Z", GoVersion:"go1.9.3b4", Compiler:"gc", Platform:"linux/amd64"}

Creating with a pod running v0.9.0:

pod log:

time="2018-07-13T15:50:54Z" level=info msg="Starting backup" backup=heptio-ark/nginx logSource="pkg/controller/backup_controller.go:339"
time="2018-07-13T15:50:57Z" level=info msg="Backup completed" backup=heptio-ark/nginx logSource="pkg/controller/backup_controller.go:401"

backup log:

time="2018-07-13T15:50:56Z" level=info msg="Backup completed successfully" backup=heptio-ark/nginx logSource="pkg/backup/backup.go:300"

I'm going to try with a clean cluster and this branch on top of master.

[nrb]

Narrowing this down some, it looks like the nginx pod isn't getting retrieved.

ark backup create all on a fresh cluster, I get the following in my backup logs:

x1c in /home/nrb/go/src/github.com/heptio/ark/docs (git) marpaia-k8s-1.11 U
% ark backup logs all | grep error
time="2018-07-13T12:30:12-04:00" level=error msg="Error executing item actions" backup=heptio-ark/all error="the server could not find the requested resource" group=v1 groupResource=pods logSource="pkg/backup/item_backupper.go:208" name=nginx-deployment-99997d74d-2xz8k namespace=nginx-example
time="2018-07-13T12:30:59-04:00" level=info msg="Backup completed with errors: the server could not find the requested resource" backup=heptio-ark/all logSource="pkg/backup/backup.go:307"

Printing some debug info, I see

Error in resourceBackupper.backupResource with the itemBackupper.backupItem call
err = the server could not find the requested resource
unstructured = &{Object:map[kind:Pod apiVersion:v1 metadata:map[generateName:nginx-deployment-99997d74d- selfLink:/api/v1/namespaces/nginx-example/pods/nginx-deployment-99997d74d-2xz8k resourceVersion:679 creationTimestamp:2018-07-13T16:25:21Z labels:map[app:nginx pod-template-hash:555538308] ownerReferences:[map[kind:ReplicaSet name:nginx-deployment-99997d74d uid:56ce88c3-86b9-11e8-8ba2-42010a9600bf controller:true blockOwnerDeletion:true apiVersion:extensions/v1beta1]] name:nginx-deployment-99997d74d-2xz8k namespace:nginx-example uid:56d0fd4a-86b9-11e8-8ba2-42010a9600bf] spec:map[serviceAccountName:default serviceAccount:default securityContext:map[] volumes:[map[name:nginx-logs persistentVolumeClaim:map[claimName:nginx-logs]] map[name:default-token-9w69v secret:map[defaultMode:420 secretName:default-token-9w69v]]] containers:[map[terminationMessagePolicy:File imagePullPolicy:IfNotPresent name:nginx image:nginx:1.7.9 ports:[map[containerPort:80 protocol:TCP]] resources:map[] volumeMounts:[map[name:nginx-logs mountPath:/var/log/nginx] map[name:default-token-9w69v readOnly:true mountPath:/var/run/secrets/kubernetes.io/serviceaccount]] terminationMessagePath:/dev/termination-log]] dnsPolicy:ClusterFirst schedulerName:default-scheduler tolerations:[map[effect:NoExecute tolerationSeconds:300 key:node.kubernetes.io/not-ready operator:Exists] map[tolerationSeconds:300 key:node.kubernetes.io/unreachable operator:Exists effect:NoExecute]] restartPolicy:Always terminationGracePeriodSeconds:30 nodeName:gke-cluster-1-default-pool-4655bde0-17mn] status:map[containerStatuses:[map[image:nginx:1.7.9 imageID:docker-pullable://nginx@sha256:e3456c851a152494c3e4ff5fcc26f240206abac0c9d794affb40e0714846c451 containerID:docker://d41feea34000e758736929861085173ca2134406575410431583807797bd7e51 name:nginx state:map[running:map[startedAt:2018-07-13T16:25:52Z]] lastState:map[] ready:true restartCount:0]] qosClass:BestEffort phase:Running conditions:[map[type:Initialized status:True lastProbeTime:<nil> lastTransitionTime:2018-07-13T16:25:28Z] map[type:Ready status:True lastProbeTime:<nil> lastTransitionTime:2018-07-13T16:25:52Z] map[type:PodScheduled status:True lastProbeTime:<nil> lastTransitionTime:2018-07-13T16:25:28Z]] hostIP:10.150.0.3 podIP:10.28.2.7 startTime:2018-07-13T16:25:28Z]]}

And

Found an error in kbbackupper.Backup with the gb.backupGroup call
err = the server could not find the requested resource
group = &APIResourceList{GroupVersion:v1,APIResources:[{pods  true   Pod [create delete deletecollection get list patch proxy update watch] [po] [all]} {persistentvolumeclaims  true   PersistentVolumeClaim [create delete deletecollection get list patch update watch] [pvc] []} {persistentvolumes  false   PersistentVolume [create delete deletecollection get list patch update watch] [pv] []} {endpoints  true   Endpoints [create delete deletecollection get list patch update watch] [ep] []} {configmaps  true   ConfigMap [create delete deletecollection get list patch update watch] [cm] []} {nodes  false   Node [create delete deletecollection get list patch proxy update watch] [no] []} {resourcequotas  true   ResourceQuota [create delete deletecollection get list patch update watch] [quota] []} {secrets  true   Secret [create delete deletecollection get list patch update watch] [] []} {events  true   Event [create delete deletecollection get list patch update watch] [ev] []} {podtemplates  true   PodTemplate [create delete deletecollection get list patch update watch] [] []} {limitranges  true   LimitRange [create delete deletecollection get list patch update watch] [limits] []} {namespaces  false   Namespace [create delete get list patch update watch] [ns] []} {services  true   Service [create delete get list patch proxy update watch] [svc] [all]} {serviceaccounts  true   ServiceAccount [create delete deletecollection get list patch update watch] [sa] []} {replicationcontrollers  true   ReplicationController [create delete deletecollection get list patch update watch] [rc] [all]}],}

[skriss]

Hmm, okay, I'll see if I can reproduce this and dig into it some more

[skriss]

aha! good catch @nrb. This line needs to be:

f.dynamicClient.Resource(gv.WithResource(resource.Name)).Namespace(namespace),

[nrb]

We should probably have a test case to catch this kind of thing. Thoughts @skriss @marpaia ?

[marpaia]

🤦‍♂️ , sorry guys. Yeah, the whole instantiation of the dynamic client is pretty suspect as I'm not super familiar with the dynamic client or the context in which it's used in Ark. I definitely think that some more tests would be great. I often struggle with how to most effectively test API interactions with an API server.. y'all don't have some sort of pattern for this in Ark, do you?

[nrb]

No worries! I think a basic test that ensures the returned client has a namespace would be sufficient for now. I'm not super familiar with the new client so I'm not sure if there's a method or field on it that we can inspect.

[marpaia]

@nrb I dug around the dynamic interface and there is no good way as far as I can tell to interrogate the client to determine if it has a namespace set or not. I found that when a namespace isn't set, the error message for GETing an API object that doesn't exist is slightly different, which is what the test asserts now, but this is really brittle / not adding much value IMO. I'm happy to back it out if y'all think it's not worth it.

[skriss]

I'd say let's leave out that test for now, get this merged, and when one of us circles back to doing some refactoring/simplification of pkg/client/dynamic.go, we can see if there are any useful tests that can be added. @nrb WDYT?

@marpaia if @nrb is OK with that approach, then I'd drop the unit-test commit, squash each of the most recent two (removing constraint and namespace fix) into the first three, and we'll be good to go.

[nrb]

@skriss Works for me - it doesn't look like there's a great test directly on the client at this point in time.

[skriss]

LGTM.

[skriss]

Thanks @marpaia!