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

If a backend is specified, an error in the preCheck during deletion will result in the backend becoming null #372

Open
wujiuye opened this issue Aug 17, 2023 · 4 comments

Comments

@wujiuye
Copy link
Contributor

wujiuye commented Aug 17, 2023

Due to the fact that the errors returned by preCheck during deletion are ignored, the processing logic for deletion is executed directly.

// pre-check Configuration
if err := r.preCheck(ctx, &configuration, meta); err != nil && !isDeleting {
return ctrl.Result{}, err
}

The initialization of the backend occurs in the preCheck method. If an error occurs in preCheck before calling the RenderConfiguration method to initialize the backend, the backend will not be initialized, resulting in the deletion processing logic using the default backend.

During the CheckProvider step, calling the meta.getCredentials method triggers the tfcfg.SetRegion method. If the configuration.Spec.Region is not specified, the tfcfg.SetRegion method will update the configuration. At this point, an error may occur with the message "the object has been modified."

I don't understand the code either, and it's unclear why configuration.Spec.Region is being updated here, as there are no other references to it elsewhere.

wujiuye pushed a commit to wujiuye/terraform-controller that referenced this issue Aug 17, 2023
@wujiuye
Copy link
Contributor Author

wujiuye commented Aug 31, 2023

Is there a plan to fix this bug?

@chivalryq
Copy link
Member

The reason of "the object has been modified" may be k8sClient got outdated Configuration in controller's cache. That is to say, someone update the object but somehow the controller didn't watch the event to update the cache.

I think we can refine the errors in preCheck. When chekcking preCheck errors, use a function say IsErrBlockDeletion to examine the error.

	if err := r.preCheck(ctx, &configuration, meta); err != nil {
		if !isDeleting {
			return ctrl.Result{}, err
		}
		// deleting
		if IsErrBlockDeletion(err) { 
			return ctrl.Result{}, err
		}
	}

@wujiuye
Copy link
Contributor Author

wujiuye commented Aug 31, 2023

The reason of "the object has been modified" may be k8sClient got outdated Configuration in controller's cache. That is to say, someone update the object but somehow the controller didn't watch the event to update the cache.

I think we can refine the errors in preCheck. When chekcking preCheck errors, use a function say IsErrBlockDeletion to examine the error.

	if err := r.preCheck(ctx, &configuration, meta); err != nil {
		if !isDeleting {
			return ctrl.Result{}, err
		}
		// deleting
		if IsErrBlockDeletion(err) { 
			return ctrl.Result{}, err
		}
	}

After IsErrBlockDeletion, what should we do next?

Should we retry by putting it back in the queue?
return ctrl.Result{Requeue: true}, nil

@chivalryq
Copy link
Member

After IsErrBlockDeletion, what should we do next?

Should we retry by putting it back in the queue?
return ctrl.Result{Requeue: true}, nil

If return err is not nil, object will be reequeued automatically.

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

No branches or pull requests

2 participants