Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resource deletion and adoption for 3 controllers #777

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

ksdpmx
Copy link
Collaborator

@ksdpmx ksdpmx commented Jun 6, 2023

What this PR does / why we need it:

resources like sa, svc and sts haven't been generated before calling Delete() which leads to the remaining finalizer for collector.
resource deletion and adoption cleanup and fix for 3 controllers

Which issue(s) this PR fixes:

Fixes #

Does this PR introduced a user-facing change?

None

Additional documentation, usage docs, etc.:


@ksdpmx ksdpmx changed the title fix: fix finalizer deletion logic for fluentbit collector fix: finalizer deletion logic for fluentbit collector Jun 6, 2023
@ksdpmx
Copy link
Collaborator Author

ksdpmx commented Jun 6, 2023

"2023-06-06T03:10:30Z ERROR Reconciler error {"controller": "collector", "controllerGroup": "fluentbit.fluent.io", "controllerKind": "Collector", "Collector": {"name":"fluent-bit-collector","namespace":"fluent"}, "namespace": "fluent", "name": "fluent-bit-collector", "reconcileID": "b0128934-0ce5-4a21-9105-aa7df920cddd", "error": "error when handling finalizer: resource name may not be empty"}"

related comment for reference: #776 (comment)

steps to reproduce:
1.

cat <<EOF > co.yaml
apiVersion: fluentbit.fluent.io/v1alpha2
kind: Collector
metadata:
  labels:
    app.kubernetes.io/name: fluent-bit
  name: fluent-bit-collector
  namespace: fluent
spec:
  annotations:
    fluentbit.io/exclude: "true"
  fluentBitConfigName: fluent-bit-config
  image: kubesphere/fluent-bit:v2.1.4
EOF
  1. kubectl create -f ./co.yaml
  2. kubectl delete -f ./co.yaml, this step will get stuck
  3. check logs from operator

@@ -170,17 +170,32 @@ func (r *CollectorReconciler) mutate(obj client.Object, co fluentbitv1alpha2.Col

func (r *CollectorReconciler) delete(ctx context.Context, co *fluentbitv1alpha2.Collector) error {
var sa corev1.ServiceAccount
if err := r.Delete(ctx, &sa); err != nil && !errors.IsNotFound(err) {
err := r.Get(ctx, client.ObjectKey{Namespace: co.Namespace, Name: co.Name}, &sa)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can call the delete directly like this

	sa := corev1.ServiceAccount{
		ObjectMeta: metav1.ObjectMeta{
			Name: co.Name,
			Namespace: co.Namespace,
		},
	}
	if err := r.Delete(ctx, &sa); err != nil && !errors.IsNotFound(err) {
		return err
	}

No need to call get.

Copy link
Collaborator Author

@ksdpmx ksdpmx Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like other controllers (fluentbit, fluentd) have to update like this as well.
btw, does it make sense to check the owner reference before deletion? what if there's another resource which is not managed by this operator (i.e. fluentbit collector) with the same name?

@ksdpmx ksdpmx changed the title fix: finalizer deletion logic for fluentbit collector fix: finalizer deletion and adoption logic Jun 7, 2023
@ksdpmx ksdpmx changed the title fix: finalizer deletion and adoption logic fix: finalizer deletion and adoption logic for 3 controllers Jun 7, 2023
@ksdpmx ksdpmx changed the title fix: finalizer deletion and adoption logic for 3 controllers fix: finalizer deletion and adoption cleanup and fix for 3 controllers Jun 7, 2023
@ksdpmx ksdpmx changed the title fix: finalizer deletion and adoption cleanup and fix for 3 controllers fix: finalizer deletion and resource adoption cleanup and fix for 3 controllers Jun 7, 2023
@ksdpmx
Copy link
Collaborator Author

ksdpmx commented Jun 7, 2023

@wanjunlei PTAL. thank you. cc @benjaminhuo

delete for finalizer & dangling resources and mutate for resource reconciliation & adoption have been updated for all 3 controllers.

what if there's another resource which is not managed by this operator (i.e. fluentbit collector) with the same name?

the lifecycle of the existing resources without owner reference but with the same name will be managed by this operator, like creation, updating, deletion.
the deletion logic for clusterrole and clusterrolebinding is not included in this pr yet.

@ksdpmx ksdpmx requested a review from wanjunlei June 7, 2023 08:22
@ksdpmx ksdpmx changed the title fix: finalizer deletion and resource adoption cleanup and fix for 3 controllers fix: resource deletion and adoption for 3 controllers Jun 8, 2023
Signed-off-by: Jason Zhang <ksdpmx@gmail.com>
@benjaminhuo
Copy link
Member

@wanjunlei are you ok with @ksdpmx‘s new change?

@benjaminhuo benjaminhuo merged commit 080555c into fluent:master Jun 9, 2023
6 checks passed
@benjaminhuo
Copy link
Member

@ksdpmx Thank you for this big refactoring

@ksdpmx ksdpmx deleted the collector_finalizer branch June 9, 2023 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants