-
Notifications
You must be signed in to change notification settings - Fork 995
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
[aws-cloudwatch-metrics] Updated cw agent image to version 1.247350.0b251780 and introduced bottlerocket support #706
[aws-cloudwatch-metrics] Updated cw agent image to version 1.247350.0b251780 and introduced bottlerocket support #706
Conversation
…b251780 and introduced bottlerocket support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codeJack sorry for the delayed review. You mention in the PR summary that you tested manually with an EKS cluster. Did you test an EKS cluster using bottlerocket images for the worker nodes?
@jaypipes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codeJack small request inline...
@@ -69,6 +72,14 @@ spec: | |||
- name: varlibdocker | |||
hostPath: | |||
path: /var/lib/docker | |||
- name: containerdsock | |||
hostPath: | |||
{{- if .Values.bottlerocket }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this would solve the immediate problem at hand. However, this value name of bottlerocket
is a one-way door for a couple reasons:
- What happens if Bottlerocket decides to change their containerd sock path again?
- What happens if there is another host OS we want to target in the future that has a different sock path?
For this reason, I think it would be better to have a containerdSockPath
value name and make it a string, defaulted to /run/containerd/containerd.sock
with a comment informing the user to set this to /run/dockershim.sock
when using Bottlerrocket as a host OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bottlerocket rejoined the fold in 1.23 and is planning to backport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @codeJack!
Issue
N/A
Description of changes
The purpose of this PR is mainly to align the chart with the latest version of the aws samples manifest as well as providing bottlrocket support.
Checklist
README.md
for modified charts)version
inChart.yaml
for the modified chart(s)Testing
The chart rendering has been tested locally, also it has been installed against an EKS cluster to make sure
ContainerInsights
is working as expected.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.