Add finalizer for AddressPool #168
Conversation
| // IsUsed returns true if the pool is used by some AddressBlock. | ||
| func (p *pool) IsUsed(ctx context.Context) (bool, error) { | ||
| blocks := &coilv2.AddressBlockList{} | ||
| err := p.reader.List(ctx, blocks, client.MatchingLabels{ |
There was a problem hiding this comment.
Let me make sure. isUsed uses the direct API client, because AddressPool can be removed in the situation that a client returns no AddressBlock, but informer cache is staled(some AddressPools exists in fact). Is my understanding correct?
There was a problem hiding this comment.
@ysksuzuki
The primary reason of using p.reader is that it is used in pool.SyncBlocks(). I don't want to increase cached resources.
Typically the finalizer of AddressPool would be invoked in response to the deletion of the final AddressBlock. IsUsed() should certainly return false, or else the finalizer will be delayed until the next forced reconcilliation. So using cache in IsUsed() would be inappropriate.
| @@ -41,6 +51,7 @@ var _ = Describe("AddressPool Webhook", func() { | |||
| ap := &AddressPool{} | |||
| err = k8sClient.Get(ctx, client.ObjectKey{Name: "test"}, ap) | |||
| Expect(err).NotTo(HaveOccurred()) | |||
| Expect(controllerutil.ContainsFinalizer(ap, constants.FinCoil)) | |||
| @@ -51,6 +53,28 @@ func (r *AddressPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) | |||
| } | |||
|
|
|||
| logger.Info("synchronized") | |||
|
|
|||
| if !ap.ObjectMeta.DeletionTimestamp.IsZero() { | |||
There was a problem hiding this comment.
DeletionTimestamp should be compared to nil.
Also, ObjectMeta is embedded so can be omitted.
There was a problem hiding this comment.
and it is simpler to negate the logic:
if ap.DeletionTimpstamp == nil {
return ctrl.Result{}, nil
}
...There was a problem hiding this comment.
Ah, this line is copied from the kubebuilder book.
Though the book uses IsZero(), it seems unnatural, so I'll fix it.
A failure in eviction may leave the zero value in the deletion timestamp, but I'm not sure this is meaningful.
kubernetes/kubernetes#54525
Signed-off-by: morimoto-cybozu <kenji_morimoto@cybozu.co.jp>
Signed-off-by: morimoto-cybozu <kenji_morimoto@cybozu.co.jp>
morimoto-cybozu
force-pushed
the
addresspool-finalizer
branch
from
e74106a
to
293b6fb
Compare
Signed-off-by: morimoto-cybozu <kenji_morimoto@cybozu.co.jp>
morimoto-cybozu
force-pushed
the
addresspool-finalizer
branch
from
293b6fb
to
a6b9602
Compare
This PR adds a finalizer for an AddressPool. The finalizer is kept while the AddressPool is used by some AddressBlock.
Signed-off-by: morimoto-cybozu kenji_morimoto@cybozu.co.jp
Fixes: #132