✨ (deployimage/v1alpha1): Improve error handling and pointer usage for value setting in controller #4399
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Hi @mateusoliveira43. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
testdata/project-v4-with-plugins/internal/controller/memcached_controller.go
Show resolved
Hide resolved
| // Let's add a finalizer. Then, we can define some operations which should | ||
| // occur before the custom resource is deleted. | ||
| // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers | ||
| if !controllerutil.ContainsFinalizer(memcached, memcachedFinalizer) { |
There was a problem hiding this comment.
@mateusoliveira43 Why remove the func from controller-runtime used to add the finalizer we should keep it as it is
There was a problem hiding this comment.
this is before logic to delete resource
I was concerned if this fails
we would add finalizer again and try to delete again. I think this is corner case, so should be ok to let as is
| log.Error(err, "Failed to add finalizer into the custom resource") | ||
| return ctrl.Result{Requeue: true}, nil |
There was a problem hiding this comment.
The only change to enhance would be here right?
Just create the error and return in the result to re-queue
| if controllerutil.ContainsFinalizer(memcached, memcachedFinalizer) { | ||
| log.Info("Performing Finalizer Operations for Memcached before delete CR") | ||
|
|
||
| // Let's add here a status "Downgrade" to reflect that this resource began its process to be terminated. | ||
| meta.SetStatusCondition(&memcached.Status.Conditions, metav1.Condition{Type: typeDegradedMemcached, | ||
| changed := meta.SetStatusCondition(&memcached.Status.Conditions, metav1.Condition{Type: typeDegradedMemcached, |
There was a problem hiding this comment.
Why add this changed?
We set the status and we update it directly as in other places
There was a problem hiding this comment.
I was thinking on user interactions scenarios
example: user creates resource -> controller reconciles it -> user changes resource label -> controller reconciles it
even though spec was not changed, without a custom predicate, every update triggers reconcile, right?
since it was just metadata change, reconcile should do nothing with object
| @@ -167,19 +154,21 @@ func (r *MemcachedReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( | |||
| return ctrl.Result{}, err | |||
| } | |||
|
|
|||
| meta.SetStatusCondition(&memcached.Status.Conditions, metav1.Condition{Type: typeDegradedMemcached, | |||
| changed = meta.SetStatusCondition(&memcached.Status.Conditions, metav1.Condition{Type: typeDegradedMemcached, | |||
There was a problem hiding this comment.
The change is not required necessary
.We are updating the status, and we are calling to update always that it needs to be updated.
Lets keep things simple has no reason to complicate
| // Let's add a finalizer. Then, we can define some operations which should | ||
| // occur before the custom resource is deleted. | ||
| // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers | ||
| if !controllerutil.ContainsFinalizer(memcached, memcachedFinalizer) { |
| // Check if the deployment already exists, if not create a new one | ||
| found := &appsv1.Deployment{} | ||
| err = r.Get(ctx, types.NamespacedName{Name: memcached.Name, Namespace: memcached.Namespace}, found) | ||
| if err != nil && apierrors.IsNotFound(err) { | ||
| if err != nil { |
There was a problem hiding this comment.
I think it easier to read
it was
if err != nil && apierrors.IsNotFound(err) {
// create a new deployment
} else if err != nil {
// error
}my suggestion is
if err != nil {
if !apierrors.IsNotFound(err) {
// error
}
// create a new deployment
}
testdata/project-v4-with-plugins/internal/controller/memcached_controller.go
Outdated
Show resolved
Hide resolved
testdata/project-v4-with-plugins/internal/controller/memcached_controller.go
Outdated
Show resolved
Hide resolved
| // Re-fetch the memcached Custom Resource before updating the status | ||
| // so that we have the latest state of the resource on the cluster and we will avoid | ||
| // raising the error "the object has been modified, please apply | ||
| // your changes to the latest version and try again" which would re-trigger the reconciliation | ||
| if err := r.Get(ctx, req.NamespacedName, memcached); err != nil { | ||
| log.Error(err, "Failed to re-fetch memcached") | ||
| return ctrl.Result{}, err |
There was a problem hiding this comment.
Before change we need fetch so why we are removing it
There was a problem hiding this comment.
I was confused by the code
first Get call the idea is to do Update call after it
But in subsequent Get calls, the idea is to do Update call before it
Tried to standardize to do a Get call after each Update call
(another solution would be to requeue after each Update call
)
k8s-ci-robot
added
size/S
There was a problem hiding this comment.
Everything looks great! 🎉
We just need to run make generate to ensure that all samples are updated accordingly.
Also, since this change impacts end users and improves their experience, the appropriate emoji would be ✨. I believe this is more of an enhancement rather than a bug fix.
To help us generate accurate release notes when we make a release, I’ve updated the title accordingly. I hope you don’t mind. 😊
camilamacedo86
changed the title
Signed-off-by: Mateus Oliveira <[email protected]>
mateusoliveira43
force-pushed
the
fix/deploy-image-plugin-refactor
branch
from
285b29d to
48eaf6b
Compare
k8s-ci-robot
added
size/L
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
k8s-ci-robot
added
the
ok-to-test
k8s-ci-robot
removed
the
needs-ok-to-test
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, mateusoliveira43 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 |
k8s-ci-robot
added
the
approved
Fix #4397 (comment)