Conversation
Fix getting the sandbox id on docker as well.
mogren
force-pushed
the
fix-sandbox-delete
branch
from
3fbfad9 to
c0afbd0
Compare
jaypipes
left a comment
jaypipes
left a comment
There was a problem hiding this comment.
Please update the commit message and/or the PR summary message to explain why this commit is fixing a problem. Currently, the PR summary and commit message is a bit thread-bare and doesn't indicate why falling back to the dockershim socket actually worked...
| value: eni | ||
| - name: AWS_VPC_K8S_CNI_MTU | ||
| value: 9001 | ||
| value: "9001" |
There was a problem hiding this comment.
interesting. this was a problem?
There was a problem hiding this comment.
Yeah, for some reason I got an error applying the config without the quotes.
| golang.org/x/net v0.0.0-20191004110552-13f9640d40b9 | ||
| golang.org/x/sys v0.0.0-20190826190057-c7b8b68b1456 | ||
| golang.org/x/time v0.0.0-20180412165947-fbb02b2291d2 // indirect | ||
| google.golang.org/appengine v1.4.0 // indirect |
There was a problem hiding this comment.
hmm. this is likely due to different Golang versions in our local development environments trampling on each other... My local go version is 1.12.7. I suspect you're on 1.13 and beyond?
| // TODO need to add node health stats here | ||
| return errors.Wrap(err, "failed to get running pods!") | ||
| } | ||
| log.Debugf("getLocalPodsWithRetry() found %d local pods", len(localPods)) |
| socketPath = criSocketPath | ||
| } | ||
| log.Debugf("Getting running pod sandboxes from %q", socketPath) | ||
| conn, err := grpc.Dial(socketPath, grpc.WithInsecure()) |
There was a problem hiding this comment.
I suppose we can turn this into a blocking call (WithBlock() with a backoff timeout) in a future commit...
| AGENT_LOG_PATH=${AGENT_LOG_PATH:-aws-k8s-agent.log} | ||
| HOST_CNI_BIN_PATH=${HOST_CNI_BIN_PATH:-/host/opt/cni/bin} | ||
| HOST_CNI_CONFDIR_PATH=${HOST_CNI_CONFDIR_PATH:-/host/opt/cni/net.d} | ||
| HOST_CNI_CONFDIR_PATH=${HOST_CNI_CONFDIR_PATH:-/host/etc/cni/net.d} |
| # Best practice states we should send the container's CMD output to stdout, so | ||
| # let's tee back up the log file into stdout | ||
| cat $AGENT_LOG_PATH | ||
| cat "$AGENT_LOG_PATH" |
There was a problem hiding this comment.
interesting that quote-encapping the variable in the entrypoint.sh script fixed anything here and above. I know it's safer to quote-encap vars but I'm curious whether this was an actual fix for a problem in the script.
There was a problem hiding this comment.
It didn't, but we were mixing quoted and unquoted, so I just made it consistent.
Fix getting the sandbox id on docker as well.
Fix getting the sandbox id on docker as well.
|
@mogren any reason not to just mount I don't think there's anything broken by doing this instead, but it's the only place in the codebase where there's a special case for Docker, and it appears unnecessary 😛 |
|
Heh, thanks @drakedevel I think you are right. I did try to use the |
Fix getting the sandbox id on docker as well.
Fix getting the sandbox id on docker as well.
3 participants
Issue #714 related
Description of changes:
In the original commit, we only checked
/var/run/cri.sockto get the Sandbox ID. This works fine for CRI-O or containerd with the CRI plugin, as long as the socket is mounted correctly, but when using docker we need the/var/run/dockershim.sockcreated by Kubelet, since it talks gRPC with the CRI API, while/var/run/docker.sockis http only.dockershim.sockprovided by kubelet to get a gRPC CRI to call. (Follow up in Fall back to using http docker client #752)Fix delete check of pod that failed to start(Merged Treating ErrUnknownPod from ipamd to be a noop #750 first)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.