Add annotation to pause resource reconciliation #346
Conversation
![openshift-ci[bot]](https://avatars.githubusercontent.com/in/91280?s=60&v=4){kind=small}
There was a problem hiding this comment.
@osmman: 1 warning.
In response to this:
/lint
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.
osmman
force-pushed
the
pause-annotation
branch
2 times, most recently
from
5576fe1
to
b004eee
Compare
|
/lint |
There was a problem hiding this comment.
@osmman: 0 warnings.
In response to this:
/lint
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.
|
target for 1.1.0 release |
| For(&rhtasv1alpha1.CTlog{}). | ||
| Owns(&v1.Deployment{}). | ||
| Owns(&v12.Service{}). | ||
| Watches(&v12.Secret{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, object client.Object) []reconcile.Request { | ||
| WatchesMetadata(&v12.Secret{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, object client.Object) []reconcile.Request { |
There was a problem hiding this comment.
Reading https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/builder#Builder.WatchesMetadata I am not sure about this. We never use &metav1.PartialObjectMetadata{} construct. Can you provide more info, why you are changing this?
There was a problem hiding this comment.
It is to reduce amount of data stored in cache memory. We do not need to fetch secret's data and store them in local cache. We only need metadata to make a decision to enqueue new event.
There was a problem hiding this comment.
I did not study the concept deeply but from the documentation
You'll need to pass metav1.PartialObjectMetadata to the client when fetching objects in your reconciler, otherwise you'll end up with a duplicate structured or unstructured cache.
I would be careful with it - we don't want to have duplicate structured or unstructured cache but if you are 100% sure what you are doing, we can merge it.
There was a problem hiding this comment.
I prefer to store small amount of duplicit data of our managed resources rather then pass thought all secrets in cluster through our local cache.
There was a problem hiding this comment.
Simple reproducer it to run debugger on this line and compare what is stored in o
https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/predicate/predicate.go#L409
|
|
||
| const ( | ||
| // PausedReconciliation Annotation used to pause resource reconciliation | ||
| PausedReconciliation = "rhtas.redhat.com/pause-reconciliation" |
There was a problem hiding this comment.
Can you move https://github.com/securesign/secure-sign-operator/blob/main/controllers/securesign/actions/segment_backup_cronjob.go#L34 to have everything defined on the same place?
There was a problem hiding this comment.
I prefer to do that in new PR because it is lot of changes which are not connected to this feature.
|
/unhold |
|
/LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bouskaJ, osmman 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 |
Add a new
rhtas.redhat.com/pause-reconciliationannotation to pause resource reconciliation. Annotation can be used on any Securesign's custom resource and resources which are owned by them like Deploymen, Service, IngressExample custom resource with a paused reconciliation condition