-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[csi/aws] Bump templates + add support for warm pools #11304
Conversation
Hi @dntosas. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
a34cfb0
to
e8c8b92
Compare
qq: where IAM resources that described in here are handled? |
they are handled in It would be nice to collect all the privs needed for CSI to one function though, to make this more maintainable. That would also allow us to use IRSA for this addon further down the road. |
@olemarkus are not IAM permissions for attaching etc volumes should be present on regular nodes role also? without them how will the pods of node daemonSet can be allowed to do such operations? |
As I understand it, it is the ebs-csi-controller that does the AWS calls. The purpose of the DaemonSet is to extend kubelet with the driver specific functionality. |
y makes sense ^^ |
I don't see the code for calculating the default value of |
@johngmyers i thought something like this should be enough ^^ if there is a more proper way or place to implement this logic, can you give a hint on where to look? |
@dntosas Okay, that does work; I had missed that. Unfortunately it is error prone to have the version in so many places. If one gets missed on an update that is unlikely to be caught in code review. The other way is to define a template function, like kops/upup/pkg/fi/cloudup/template_functions.go Lines 588 to 608 in dbd2347
|
f9adcd7
to
7bf0805
Compare
7bf0805
to
c08a35d
Compare
@olemarkus @johngmyers thanks for your help :) pushed changes, i think we are better now ^^ |
pkg/apis/kops/componentconfig.go
Outdated
@@ -838,6 +838,16 @@ type CloudConfiguration struct { | |||
type AWSEBSCSIDriver struct { | |||
//Enabled enables the AWS EBS CSI driver | |||
Enabled *bool `json:"enabled,omitempty"` | |||
|
|||
// Version of the CSI controller container |
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.
Nit: go comments should have proper punctuation.
return nil | ||
} | ||
|
||
if c.Enabled == nil { |
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.
nit: this check is typically noops as we tend to use if fi.BoolValue()
which defaults to false.
And you have to use {{ if WithDefaultBool <defaultVaule> }}
in templates as templates will only check if the pointer is nil or not.
Just a few nits, otherwise looks good to go to me. |
bd7e329
to
12cd402
Compare
- Update manifests - Bump components version - Add API capability of setting Version + VolumeLimit - Remove snapshot-controller resources as it should be independent from any CSI driver Signed-off-by: dntosas <[email protected]>
12cd402
to
1a2e55b
Compare
1a2e55b
to
ef88814
Compare
Add pulling needed images as initial task for warming up instances for csi driver resources. Signed-off-by: dntosas <[email protected]>
ef88814
to
9481246
Compare
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
AWS EBS CSI Driver:
- Update manifests
- Bump components version
- Add API capability of setting Version + VolumeLimit
- Add support for warm pools
- Remove snapshot-controller resources as it should be independent from
any CSI driver, check here --> https://kubernetes.io/blog/2020/12/10/kubernetes-1.20-volume-snapshot-moves-to-ga/#how-to-deploy-volume-snapshots
Signed-off-by: dntosas [email protected]