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

Change k8s 1.6 calico manifest's ordering of service account creation #2641

Merged
merged 1 commit into from
May 27, 2017

Conversation

ottoyiu
Copy link
Contributor

@ottoyiu ottoyiu commented May 26, 2017

This fixes the race-condition behaviour described in #2529 which was fixed by #2590, by
avoiding the configure-calico job all together.


This change is Reviewable

…nt first

This fixes the behaviour described in kubernetes#2529 which was fixed by kubernetes#2590, by
avoiding the configure-calico job all together.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 26, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @ottoyiu. 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 @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 26, 2017
@ottoyiu ottoyiu changed the title Change k8s1.6 calico manifest ordering of service account creation Change k8s 1.6 calico manifest's ordering of service account creation May 26, 2017
@chrislovecnm
Copy link
Contributor

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 26, 2017
@chrislovecnm
Copy link
Contributor

You tested by hand on which 1.6 versions?

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Thanks - questions about testing

@chrislovecnm
Copy link
Contributor

Waiting on e2e and questions about how it was tested

@ottoyiu
Copy link
Contributor Author

ottoyiu commented May 26, 2017

I played around with it on 1.6.2 with the old kops prior to configure-calico was removed and couldn't seem to replicate the problem anymore after this fix. @blakebarnett mentioned service accounts, and I quickly check and found out that the manifests were put in the wrong order. By having the service accounts be created after the Daemonset of calico, the Deployment/ReplicaSet for policy-manager and the configure-calico Job previously - you were in the mercy of the scheduler really.

I can test this change tomorrow morning on k8s 1.6.3, and 1.6.4 to play it safe. If you can hold off the merge after e2e testing, I'll double and triple check this change.

@chrislovecnm
Copy link
Contributor

Will mark this WIP

@ottoyiu
Copy link
Contributor Author

ottoyiu commented May 26, 2017

Tested this on k8s 1.6.3 and 1.6.4 with no issues.

oyiu@oyiu-melt [~/.gvm/pkgsets/go1.7/global/src/k8s.io/kops][kops_srcdst*] kubectl get serviceaccounts calico --namespace kube-system
NAME      SECRETS   AGE
calico    1         16m

Edit: while testing this change, figured out a related issue:
#2647

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Thanks

@chrislovecnm chrislovecnm merged commit 3d845f4 into kubernetes:master May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants