-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Add service events for azure cloud provider #68212
Conversation
/sig azure |
/assign @andyzhangx |
9f28277
to
9179b28
Compare
@feiskyer It's conflict now |
@andyzhangx Solved now. PTAL |
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.
/lgtm
@@ -383,7 +391,12 @@ func parseConfig(configReader io.Reader) (*Config, error) { | |||
} | |||
|
|||
// Initialize passes a Kubernetes clientBuilder interface to the cloud provider | |||
func (az *Cloud) Initialize(clientBuilder controller.ControllerClientBuilder) {} | |||
func (az *Cloud) Initialize(clientBuilder controller.ControllerClientBuilder) { | |||
az.kubeClient = clientBuilder.ClientOrDie("azure-cloud-provider") |
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.
cc @tallclair @deads2k
for more cloud provider clients
I know we don't want roles for these getting added to bootstrap policy. what client should they use to speak to the API?
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.
@liggitt Didn't get the response yet from @tallclair @deads2k. Any suggestion of candidate role should be used for such cases?
cloud-specific roles should not be part of the bootstrap policy installed on all clusters (xref #66628 for clearing out the one that slipped in accidentally) I'm actually out of the office, redirecting to @tallclair / @mikedanese / @deads2k for guidance on what client to use for reporting events instead (or alternately how to set up the role to give the cloudprovider-specific client event permissions) |
@liggitt Thanks for the comments.
@tallclair @deads2k @mikedanese Could you please take a look at this? |
/retest |
kindly ping @tallclair @deads2k @mikedanese |
@@ -489,6 +489,13 @@ func ClusterRoles() []rbacv1.ClusterRole { | |||
rbacv1helpers.NewRule("create").Groups(certificatesGroup).Resources("certificatesigningrequests/selfnodeclient").RuleOrDie(), | |||
}, | |||
}, | |||
{ | |||
// a role making azure cloud provider to send events | |||
ObjectMeta: metav1.ObjectMeta{Name: "system:azure-cloud-provider"}, |
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.
This is the role that @liggitt noted shouldn't go into core.
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.
@deads2k Thanks. Then let me remove this and add a release note on it. This is a user-fencing change.
While there is still discussion over exactly which org cloud providers should be in, they all seem to agree that k/k isn't the right spot. Given that view, I don't think it makes sense to use a service account from the |
When errors occur on azure cloud provider, events now are added so that users could easily find the underground errors on Azure API.
@brendandburns @khenidak do you have any opinion about this PR? We are going to make this PR into v1.12, it's very useful for debugging, while clusterrole and rolebinding needs to be added into acs-engine and AKS otherwise there will be error event in service description. Update: |
/retest |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx, feiskyer 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 |
What this PR does / why we need it:
azure-cloud-provider events are now added for easily identify the underground errors of Azure API.
Before this PR, only
timed out waiting for the condition
on the service events. Users couldn't know what's wrong unless login to master and check kube-controller-manager logs:After this PR, the details errors would also show in service's events:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #63155
Special notes for your reviewer:
Release note: