separate finalizer cleanup controller to allow re-use #276
Conversation
|
@deads2k: GitHub didn't allow me to assign the following users: sohankunkerkar. Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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. |
openshift-ci-robot
added
the
size/M
openshift-ci-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
it doesn't compile weird . . . |
| @@ -68,7 +73,7 @@ func NewFinalizerController( | |||
| } | |||
|
|
|||
| func (c finalizerController) sync() error { | |||
There was a problem hiding this comment.
so this controller simply removes all finalizers except FinalizerKubernetes from a namespace?
There was a problem hiding this comment.
so this controller simply removes all finalizers except
FinalizerKubernetesfrom a namespace?
It removes only the kube finalizer. The kube finalizer requires every aggregated apiserver be running. When you're removing an aggregated apiserver's namespace that will come back, you don't want to remove the apiservices or controllers will do bad things, so this allows the namespace to be removed and recreated.
deads2k
force-pushed
the
finalizing-controller
branch
from
a7b22d6
to
47e2719
Compare
|
/retest |
1 similar comment
|
/retest |
| "github.com/openshift/cluster-openshift-apiserver-operator/pkg/operator/resourcesynccontroller" | ||
| "github.com/openshift/cluster-openshift-apiserver-operator/pkg/operator/workloadcontroller" | ||
| "github.com/openshift/library-go/pkg/operator/staticpod/controller/revision" | ||
| "k8s.io/apimachinery/pkg/api/errors" |
There was a problem hiding this comment.
Why throw all the packages on one pile instead of keeping system, external, our and in-project imports separated in logical blocks? I know I asked this before, but this still feels counter-intuitive.
There was a problem hiding this comment.
Why throw all the packages on one pile instead of keeping system, external, our and in-project imports separated in logical blocks? I know I asked this before, but this still feels counter-intuitive.
- the tooling does a better job of keeping it in order
- this list is still sorted
- having separate blocks doesn't provide a practical benefit for tooling or hand inspection since they are alpha sorted.
There was a problem hiding this comment.
The tooling does the alpha sorting fine with multiple blocks, too.
I can see how having separate blocks could help expose certain copy-paste faster during development, it did for me in the past anyway.
I don't see how a single block provides a practical benefit for tooling or hand inspection either.
It was my impression that it was a common good practice that I've seen in multiple languages - C, Python, Haskell, Go, namely.
Being the only one who sees it like that probably makes me wrong though, I won't force this habit on projects that I do not own no more.
| return nil | ||
| } | ||
|
|
||
| pods, err := c.podLister.Pods(c.namespaceName).List(labels.Everything()) |
There was a problem hiding this comment.
@deads2k can we narrow this list to something that only select apiserver pods?
There was a problem hiding this comment.
@deads2k can we narrow this list to something that only select apiserver pods?
the point of this block is to ensure that all the pods are gone. Scoping it down defeats the purpose.
| if err != nil { | ||
| return err | ||
| } | ||
| if len(pods) > 0 { | ||
| return nil | ||
| } | ||
| dses, err := c.dsLister.DaemonSets(operatorclient.TargetNamespace).List(labels.Everything()) | ||
| dses, err := c.dsLister.DaemonSets(c.namespaceName).List(labels.Everything()) |
There was a problem hiding this comment.
do we expect more daemonsets in this namespace?
There was a problem hiding this comment.
do we expect more daemonsets in this namespace?
we know we create them for apiservers today.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, stlaz 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 |
All aggregated APIs face a finalization problem. This will allow us to re-use the logic in the authentication operator.
/assign @p0lyn0mial @sanchezl @sohankunkerkar