Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install crictl in Agent containers #418

Merged
merged 9 commits into from
Nov 23, 2021

Conversation

kate-goldenring
Copy link
Contributor

What this PR does / why we need it:
Instead of requiring users to install crictl locally, this adds crictl directly to the Agent containers, removing the need to mount it from the host in the charts/templates.

Many users can forget to specify the path of crictl. fixes #6

Special notes for your reviewer:
The size of crictl locally is 28.5 MBs:

kagold@nuc-ubtunu1:~$ stat /usr/local/bin/crictl
  File: /usr/local/bin/crictl
  Size: 28488749        Blocks: 55648      IO Block: 4096   regular file
Device: fd00h/64768d    Inode: 9440263     Links: 1
Access: (0755/-rwxr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2021-11-12 00:47:30.575087778 +0000
Modify: 2019-12-16 03:11:36.000000000 +0000
Change: 2021-10-04 21:26:57.523199337 +0000
 Birth: -

Similarly, adding crictl to the agent increases its size from 128MB to 161MB.

Associated documentation should be updated to match this PR.

We could also remove the need to set the container runtime path agent.host.dockerShimSock and instead have users specify their distro with something like kubernetesDistro.microk8s=true. Then the appropriate socket path will be set.

We also may want to consider using a more recent version of crictl and add updating it to our release cycle.

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • this PR contains unit tests
  • added code adheres to standard Rust formatting (cargo fmt)
  • code builds properly (cargo build)
  • code is free of common mistakes (cargo clippy)
  • all Akri tests succeed (cargo test)
  • inline documentation builds (cargo doc)
  • version has been updated appropriately (./version.sh)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

@bfjelds
Copy link
Collaborator

bfjelds commented Nov 15, 2021

is this a patch-level version change or a minor-level change?

@kate-goldenring
Copy link
Contributor Author

@bfjelds what do you think about:

We could also remove the need to set the container runtime path agent.host.dockerShimSock and instead have users specify their distro with something like kubernetesDistro.microk8s=true. Then the appropriate socket path will be set.

I was thinking that it would make sense to do this in a subsequent PR so we only have to change our docs once. With just this PR, AKRI_HELM_CRICTL_CONFIGURATION should be changed to something like CRI_SOCKET but if we enable setting a K8s distro, it could be an added line of --set kubernetesDistro=<microk8s|k3s|k8s> or kubernetesDistro.<distro>=true

RUN apt-get update && apt-get install -y --no-install-recommends libssl-dev openssl curl ca-certificates && apt-get clean

# Install crictl
RUN curl -L https://github.com/kubernetes-sigs/cri-tools/releases/download/v1.17.0/crictl-v1.17.0-linux-amd64.tar.gz --output crictl-v1.17.0-linux-amd64.tar.gz &&\
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use same VERSION=... && curl $VERSION style here too?

Copy link
Contributor Author

@kate-goldenring kate-goldenring Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch thanks! fixed dcb45dd

@bfjelds
Copy link
Collaborator

bfjelds commented Nov 16, 2021

@bfjelds what do you think about:

We could also remove the need to set the container runtime path agent.host.dockerShimSock and instead have users specify their distro with something like kubernetesDistro.microk8s=true. Then the appropriate socket path will be set.

I was thinking that it would make sense to do this in a subsequent PR so we only have to change our docs once. With just this PR, AKRI_HELM_CRICTL_CONFIGURATION should be changed to something like CRI_SOCKET but if we enable setting a K8s distro, it could be an added line of --set kubernetesDistro=<microk8s|k3s|k8s> or kubernetesDistro.<distro>=true

how confident are we that we can get the right path based on distro? if we are really sure, then it sounds like a good idea. i suppose if someone was using some nonstandard-path, they wouldn't be able to use the helm charts directly anymore, but maybe that never happens?

@kate-goldenring
Copy link
Contributor Author

@bfjelds what do you think about:

We could also remove the need to set the container runtime path agent.host.dockerShimSock and instead have users specify their distro with something like kubernetesDistro.microk8s=true. Then the appropriate socket path will be set.

I was thinking that it would make sense to do this in a subsequent PR so we only have to change our docs once. With just this PR, AKRI_HELM_CRICTL_CONFIGURATION should be changed to something like CRI_SOCKET but if we enable setting a K8s distro, it could be an added line of --set kubernetesDistro=<microk8s|k3s|k8s> or kubernetesDistro.<distro>=true

how confident are we that we can get the right path based on distro? if we are really sure, then it sounds like a good idea. i suppose if someone was using some nonstandard-path, they wouldn't be able to use the helm charts directly anymore, but maybe that never happens?

I think we could still enable someone to override the default. I'll put in a PR of what im thinking after this

COPY ./target/${CROSS_BUILD_TARGET}/${BUILD_TYPE}/agent /agent

# Install crictl
RUN VERSION="v1.17.0" && \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you pick 1.17? We had problems using older crictl against newer clusters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the version we have been using in our e2e tests and documentation.

We also may want to consider using a more recent version of crictl and add updating it to our release cycle.

I think our next release we can consider moving to a newer version

curl -L https://github.com/kubernetes-sigs/cri-tools/releases/download/$VERSION/crictl-$VERSION-linux-amd64.tar.gz --output crictl-$VERSION-linux-amd64.tar.gz && \
tar zxvf crictl-$VERSION-linux-amd64.tar.gz -C /usr/local/bin && \
rm -f crictl-$VERSION-linux-amd64.tar.gz && \
apt-get remove -y curl ca-certificates && apt-get clean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge the two RUNs? Otherwise curl/ca-certs will be part of the akri layers and contribute to the overall size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I condensed them in 75f8467 but it didn't decrease the size at all interestingly. Still 161 MB

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Do we know ca-certs and curl were not in the initial image to begin with or installed as dependencies of the other packages we are installing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure they werent there because it would not install until i added them

Signed-off-by: Kate Goldenring <[email protected]>
Signed-off-by: Kate Goldenring <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle crictl query error in slot reconciliation
3 participants