-
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
Template the calico flex volume based on the kubelet spec #7978
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @mtl-wgtwo! |
Hi @mtl-wgtwo. 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. |
Thanks for the contribution @mtl-wgtwo, we're sorry for the issue! Would you mind signing the CLA with the email used to commit your code? Once you do this, you'll be able to contribute to all our projects going forward. Once that is complete, we can kick off tests and get this rolling into a fix! Thanks so much! |
I think this PR makes the CoreOS/Flatcar support happening faster than my PR #7545 which is left behind since a while. Since seems that this is getting the right attraction so I would refactor my PR and keep the only few things missing for the complete Flatcar support and rely on this to customize the Calico Flex volume plugins. |
@mtl-wgtwo keep in mind please that also the controller-manager set as a flag the volume plugin dir. I'm not sure if we change it in kubelet, we need to change it also in here https://github.com/kubernetes/kops/blob/master/nodeup/pkg/model/kube_controller_manager.go#L167 WDYT? |
I've checked better the code and I've found out that setting the kubelet volume plugin dir in the spec, it sets it in the controller manager too. So all good here. |
/assign @KashifSaadat |
Thanks for the feedback @mazzy89 and @mikesplain. I’m currently working in my company to get the CLA sorted out. It will be a one-time pain and I’ll get this moving as quickly as possible. |
@mtl-wgtwo which is the status here? May I help somehow? Perhaps moving this change into my already opened PR would help to move faster and have CoreOS/Flatcar support fixed for the kops 1.15.x release. Let me know if is fine for you. |
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.
/approve
/lgtm
ah missing CLA, its not going through |
/ok-to-test /approve cancel |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mtl-wgtwo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@mtl-wgtwo: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
This was included as part of #8062. |
@mtl-wgtwo: PR needs rebase. 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. |
Niceee :-) so this can be closed |
Apologies on being slow on this. We've signed the CCLA, but for some reason it isn't registering properly so I can't sign my CLA. I'll just close this then and deal with the other issue later. |
The calico spec uses a flex dir that doesn't work on CoreOS
and probably other platforms. This PR templates the spec
dir with the value from the kubelet spec that is set here:
https://github.com/kubernetes/kops/blob/master/docs/cluster_spec.md#configure-a-flex-volume-plugin-directory
Note that without this, kops 1.15 fails to provision on CoreOS nodes.