-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
📖 Docs for record events. #3375
Conversation
/test pull-kubebuilder-e2e-k8s-1-24-7 |
/test pull-kubebuilder-e2e-k8s-1-26-0 |
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.
Thank you for your contribution, but this PR requires changes.
- Ensure that you check your changes using the preview so that you can auto-verify if all is ok
- You need add a new section in the menu for the new doc (it should be under references)
- You also need to ensure that the content follows up the required styled
- By last, instead of trying to link the samples under testdata/, we must add the code snippet related to the specific examples.
I hope the detailed steps described in https://github.com/kubernetes-sigs/kubebuilder/pull/3375/files#r1184594890 can help you out.
Thank you so much @camilamacedo86 for these detailed explanation. I am lucky to have a mentor like you. |
I know understand how to link page to left menu. All thanks to you. about code snippet, I dont know which code snippet would be used or where to take it. from links one link was working so I just extract it out add it in docs. Let me know which code snippet would you like me to add. |
|
||
### how to implement the solution to raise an event: | ||
|
||
a) You need to pass the recorder when you set up the recorder for the controller when the event will be raised see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4-with-deploy-image/main.go#L103 |
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.
a) You need to pass the recorder when you set up the recorder for the controller when the event will be raised see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4-with-deploy-image/main.go#L103 | |
### Allowing usage of EventRecorder on the Controller | |
To raise an event, you must have access to `record.EventRecorder` in the Controller. Therefore, firstly let's update the controller implementation: | |
```go | |
import ( | |
... | |
"k8s.io/client-go/tools/record" | |
... | |
) | |
// MyKindReconciler reconciles a MyKind object | |
type MyKindReconciler struct { | |
client.Client | |
Scheme *runtime.Scheme | |
// See that we added the following code to allow us to pass the record.EventRecorder | |
Recorder record.EventRecorder | |
} |
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.
Lets first change the controller; otherwise, users will check the compiler failing.
We can also do a generic example and put the imports.
|
||
a) You need to pass the recorder when you set up the recorder for the controller when the event will be raised see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4-with-deploy-image/main.go#L103 | ||
b) You need to add the makers to add the RBAC permissions to allow your Operator/controller to raise the event see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4-with-deploy-image/controllers/memcached_controller.go#L66 and run make manifests | ||
c) You can check an example of the event being called in: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4-with-deploy-image/controllers/memcached_controller.go#L299-L303 |
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.
By last, we can describe how to grant the permissions. See that users must run make manifests.
c) You can check an example of the event being called in: https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4-with-deploy-image/controllers/memcached_controller.go#L299-L303 | |
### Granting the required permissions | |
You must also grant the RBAC rules permissions to allow your project to create Events. Therefore, ensure that you add the [RBAC][rbac-markers] into your controller: | |
```go | |
... | |
//+kubebuilder:rbac:groups=core,resources=events,verbs=create;patch | |
... | |
func (r *MyKindReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | |
``` | |
And then, run `$ make manifests` to update the rules under `config/rbac/rule.yaml`. |
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.
We will need to add at the bottom an alias [rbac-markers]
for the page: https://book.kubebuilder.io/reference/markers/rbac.html
It is a kubebuilder doc, so we should NOT use the above URL; instead, we should use a relative path from this place to the file.
Building on the example introduced in [Controller Example](../basics/simple_controller.md), we can add Events to our reconcile logic using `recorder` as our `EventRecorder` | ||
|
||
|
||
```go | ||
//Reconcile logic up here... | ||
|
||
// Create the resource | ||
found := &appsv1.Deployment{} | ||
err = r.Get(context.TODO(), types.NamespacedName{Name: deploy.Name, Namespace: deploy.Namespace}, found) | ||
if err != nil && errors.IsNotFound(err) { | ||
log.Printf("Creating Deployment %s/%s\n", deploy.Namespace, deploy.Name) | ||
err = r.Create(context.TODO(), deploy) | ||
if err != nil { | ||
return reconcile.Result{}, err | ||
} | ||
|
||
// Write an event to the ContainerSet instance with the namespace and name of the | ||
// created deployment | ||
r.recorder.Event(instance, "Normal", "Created", fmt.Sprintf("Created deployment %s/%s", deploy.Namespace, deploy.Name)) | ||
|
||
} else if err != nil { | ||
return reconcile.Result{}, err | ||
} | ||
|
||
// Preform update | ||
if !reflect.DeepEqual(deploy.Spec, found.Spec) { | ||
found.Spec = deploy.Spec | ||
log.Printf("Updating Deployment %s/%s\n", deploy.Namespace, deploy.Name) | ||
err = r.Update(context.TODO(), found) | ||
if err != nil { | ||
return reconcile.Result{}, err | ||
} | ||
|
||
// Write an event to the ContainerSet instance with the namespace and name of the | ||
// updated deployment | ||
r.recorder.Event(instance, "Normal", "Updated", fmt.Sprintf("Updated deployment %s/%s", deploy.Namespace, deploy.Name)) | ||
|
||
} | ||
return reconcile.Result{}, nil | ||
} | ||
``` | ||
Events should be raised in certain circumstances only and following this guideline acctually is not a good practice. See what the Kubernetes APIs convention describes when using them [here][Events] | ||
|
||
<aside class="note"> | ||
It is NOT recommended to emit events for all Operations. If authors raise too many events it brings bad UX experiences for those that are consuming the solutions on the cluster and they will not have attention to what they actually must be notified. | ||
</aside> |
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.
Building on the example introduced in [Controller Example](../basics/simple_controller.md), we can add Events to our reconcile logic using `recorder` as our `EventRecorder` | |
```go | |
//Reconcile logic up here... | |
// Create the resource | |
found := &appsv1.Deployment{} | |
err = r.Get(context.TODO(), types.NamespacedName{Name: deploy.Name, Namespace: deploy.Namespace}, found) | |
if err != nil && errors.IsNotFound(err) { | |
log.Printf("Creating Deployment %s/%s\n", deploy.Namespace, deploy.Name) | |
err = r.Create(context.TODO(), deploy) | |
if err != nil { | |
return reconcile.Result{}, err | |
} | |
// Write an event to the ContainerSet instance with the namespace and name of the | |
// created deployment | |
r.recorder.Event(instance, "Normal", "Created", fmt.Sprintf("Created deployment %s/%s", deploy.Namespace, deploy.Name)) | |
} else if err != nil { | |
return reconcile.Result{}, err | |
} | |
// Preform update | |
if !reflect.DeepEqual(deploy.Spec, found.Spec) { | |
found.Spec = deploy.Spec | |
log.Printf("Updating Deployment %s/%s\n", deploy.Namespace, deploy.Name) | |
err = r.Update(context.TODO(), found) | |
if err != nil { | |
return reconcile.Result{}, err | |
} | |
// Write an event to the ContainerSet instance with the namespace and name of the | |
// updated deployment | |
r.recorder.Event(instance, "Normal", "Updated", fmt.Sprintf("Updated deployment %s/%s", deploy.Namespace, deploy.Name)) | |
} | |
return reconcile.Result{}, nil | |
} | |
``` | |
Events should be raised in certain circumstances only and following this guideline acctually is not a good practice. See what the Kubernetes APIs convention describes when using them [here][Events] | |
<aside class="note"> | |
It is NOT recommended to emit events for all Operations. If authors raise too many events it brings bad UX experiences for those that are consuming the solutions on the cluster and they will not have attention to what they actually must be notified. | |
</aside> |
docs/book/src/SUMMARY.md
Outdated
@@ -108,6 +108,7 @@ | |||
|
|||
- [Makefile Helpers](./reference/makefile-helpers.md) | |||
- [Project config](./reference/project-config.md) | |||
- [Creating Events](./reference/creating-events.md) |
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.
- [Creating Events](./reference/creating-events.md) | |
- [Raising Events](./reference/creating-events.md) |
Can you please add it after [Using Finalizers](https://github.com/kubernetes-sigs/kubebuilder/blob/da10bf1678133baae9085a96aeffdb9a0dbaa12f/docs/book/src/reference/using-finalizers.md)
So that it would be in a more appropriated order if you check the current items. It seems fit better.
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.
I did a full review of this one. Please, feel free to check all suggestions and comments.
Also, see that I updated the PR description for you to check how we usually do that. We should not add maintainers or issue authors' names.
The purpose of the description is to be very short content that describes what the PR is about and its motivation. So that in the future, if we want to check a specific change, we can come back to check it, and also it helps to let the reviewers know what is done and why.
|
||
</aside> | ||
|
||
Events are published from a Controller using an [EventRecorder]`type CorrelatorOptions struct`, |
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.
@Sajiyah-Salat now see that you would replace the a, b, and c for the steps.
It means here we would have the first step as suggested: #3375 (comment)
And then, you would have the second step which is update the cmd/main.go see: #3375 (comment)
By last you have the step to grant the permissions; #3375 (comment)
cc : @camilamacedo86 |
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.
Can you please squash the commits so that we can move forward with this one?
After a fast look it seems fine now. Great work 🥇
when I run git rebase it gives some conflicting file which have generate something conflicting. We have two option there to choose the current or incoming change or to choose both. What should I choose here? |
Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.27.1 to 1.27.2. - [Release notes](https://github.com/onsi/gomega/releases) - [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md) - [Commits](onsi/gomega@v1.27.1...v1.27.2) --- updated-dependencies: - dependency-name: github.com/onsi/gomega dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
7d84603
to
5b0b500
Compare
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. |
5b0b500
to
0e7faa2
Compare
0e7faa2
to
f0bd010
Compare
@Sajiyah-Salat: The following tests 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. |
I ddid messed up the creating-events file and the commits related to it. However we still have the backup file. in another branch. I have started a new pr. Please review here. |
Descripton
Add the documentation to help out users know how to implement and raise events.
NOTE: we had this document in v1 docs but it is outdated and we are shaping it accordingly.
Motivation
Fixes #2897