Fix ServerMaintenance removal when Server is gone#373
Conversation
afritzler
changed the title
ServerMaintenance removal when Server is gone
afritzler
left a comment
afritzler
left a comment
There was a problem hiding this comment.
Thanks @stefanhipfel! Some cosmetics.
|
|
||
| // SetupWithManager sets up the controller with the Manager. | ||
| func (r *ServerMaintenanceReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| if err := mgr.GetFieldIndexer().IndexField( |
There was a problem hiding this comment.
Can we move the field indexer setup to main.go of the manger - that way we can use a proper context handling.
There was a problem hiding this comment.
why not leave it here and just pass the context:
func (r *ServerMaintenanceReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager) error {
There was a problem hiding this comment.
I am with @afritzler here, I would expect to indices in the main.go. Technically, indices could used by multiple different reconcilers.
There was a problem hiding this comment.
I would rather not put it in main.go. This file gets too cluttered otherwise. Maybe we can put it into the controller package and call it for example index.go, for all indexes.
There was a problem hiding this comment.
Fine with me, I have no issue with larger source files, though.
|
Is this PR superseded by #387, which forbids deletion of |
the pr also includes the use of FieldIndexer to find maintenances. So it is still valid |
|
|
||
| // SetupWithManager sets up the controller with the Manager. | ||
| func (r *ServerMaintenanceReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| if err := mgr.GetFieldIndexer().IndexField( |
There was a problem hiding this comment.
I am with @afritzler here, I would expect to indices in the main.go. Technically, indices could used by multiple different reconcilers.
| if apierrors.IsNotFound(err) { | ||
| // Server not found, delete the ServerMaintenance CR | ||
| log.Info("Server no longer found, deleting ServerMaintenance", "ServerMaintenance", serverMaintenance.Name) | ||
| if err := r.Delete(ctx, serverMaintenance); err != nil { |
There was a problem hiding this comment.
Is deleting self a good idea?
There was a problem hiding this comment.
I think in this very rare occasion (or bad state) it is fine. Otherwise we would need to garbage collect those orphaned maintenances somewhere else.
An orphaned maintenance (without any server) is pointless and can be deleted without any side-effects
nagadeesh-nagaraja
left a comment
nagadeesh-nagaraja
left a comment
There was a problem hiding this comment.
LGTM! except for 1 comment about self deleting.
|
closed in favour of: #272 |
5 participants
Proposed changes
when bmc/server gets deleted which has a maintenance reference, the maintenance CRD will be automatically deleted now.
Fixes #367