Remove SriovNetwork finalizers on controller shutdown #174
Conversation
|
We also use a similar finalizer mechanism for the SriovNetworkPoolConfig. So maybe we should do the same for the SriovNetworkPoolConfig CRs. |
main.go
Outdated
| @@ -178,6 +179,8 @@ func main() { | |||
| setupLog.Error(err, "problem running manager") | |||
| os.Exit(1) | |||
| } | |||
| // Remove all finalizers after controller is shut down | |||
| utils.Shutdown(mgr.GetClient()) | |||
There was a problem hiding this comment.
Shall we move this function before the main loop and use defer call instead?
There was a problem hiding this comment.
done
Adding defer before we start the manager
pkg/utils/shutdown.go
Outdated
| "context" | ||
|
|
||
| "github.com/golang/glog" | ||
| sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" |
There was a problem hiding this comment.
nit: just change the order to be
"context"
"github.com/golang/glog"
"sigs.k8s.io/controller-runtime/pkg/client"
sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1"
| if err != nil { | ||
| glog.Errorf("Failed to list SriovNetworks: %+v", err) | ||
| } else { | ||
| for _, instance := range networkList.Items { |
There was a problem hiding this comment.
I think you need first to check if there is a networkAttachmentDefinition and remove it first
There was a problem hiding this comment.
Note that we are not deleting the SriovNetworks here.
All we do in this PR is to remove the sriovoperator related finalizer, saying "sriov operator not does not hold a grudge when you delete this", so this change would be outside the scope of this PR.
| ) | ||
|
|
||
| func Shutdown(c client.Client) { | ||
| networkList := &sriovnetworkv1.SriovNetworkList{} |
There was a problem hiding this comment.
Can we add some log messages here?
This patch overall looks good to me.
There was a problem hiding this comment.
Added logging.
Note that the log messages might be hard to see, as the pod will be removed right after this is executed.
The PR is added in response to customer complaints about SriovNetworks being stuck as terminating after uninstalling the sriov operator.
This PR removes the "netattdef.finalizers.sriovnetwork.openshift.io" finalizer from SriovNetworks when the sriovnetwork controller is deleted. This finalizer is added to each SriovNetwork on reconciliation, but it is not removed when the operator is uninstalled, leaving SriovNetworks with finalizers and the SriovNetwork crd hanging in the terminating state forever.
The finalizers will be removed whenever the sriov operator pod is shut down, including pod restarts. The removed finalizer will be added when the pod restarts and the networks are reconciled.