-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Optimze detecting if calico exists #9873
Optimze detecting if calico exists #9873
Conversation
Hi @hangscer8. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
@hangscer8 Please check #9861 This part has changed a lot recently ;) |
check version fails because calico's crd(clusterinformations.crd.projectcalico.org) is not yet installed, which causes But is there a better way to resolve it? There are some thoughts:
/cc @zhan9san Any thoughts? |
/ok-to-test |
Thanks for making me involved. I don't think ignoring error is a good option. Instead I prefer to catch the expected error. |
IMO, first time we introduce the calico check.yml is to work around some compatibility issues when we perform the calico upgrade. Can we check whether calico's crd(clusterinformations.crd.projectcalico.org) is installed like below? kubespray/roles/network_plugin/calico/tasks/check.yml Lines 46 to 51 in fff4005
|
I'm curious why the crds doesn't exist when calico is upgraded. check_version is only happen in calico upgrade? |
Yeah, it's weird that crds doesn't exist when calico is upgraded.
It always run in Except the fresh install, as if it is a fresh install, Line 34 in fff4005
kubespray/roles/kubernetes/preinstall/tasks/main.yml Lines 126 to 132 in fff4005
I'll try to reproduce this issue. |
It is possible that i sometimes kill the e2e ci job which runs |
So the issue becomes how to detect the calico is installed correctly. How about checking both |
I think it is a good idea. I will fix that. |
7375217
to
5efa8a4
Compare
09beeb0
to
e55d1a4
Compare
3c66930
to
6539fe3
Compare
Signed-off-by: hang.jiang <hang.jiang@daocloud.io>
6539fe3
to
0fa8412
Compare
Thanks @hangscer8 LGTM |
Thanks! /lgtm |
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.
@hangscer8 Thank you
And thanks guys for the discussion and review
/kind Network
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: floryut, hangscer8 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 |
Signed-off-by: hang.jiang <hang.jiang@daocloud.io>
Signed-off-by: hang.jiang <hang.jiang@daocloud.io>
What type of PR is this?
/kind bug
What this PR does / why we need it:
calicoctl.sh
exists ,but not the related crds, such asclusterinformations
.ClusterInformation
of calico.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: