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

lxd: Add support for forced deletion of projects #14343

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

boltmark
Copy link
Contributor

@boltmark boltmark commented Oct 25, 2024

Currently in draft state as this PR relies on deleting resources via requests to the server's own API. This is not ideal, and I am investigating if it is possible to avoid this. Right now it looks like this is the simplest option, but we could likely add implementation for each resource to perform the deletes without invoking the LXD API. Perhaps we could factor out some of the logic in the delete handlers so that we can reference it locally. Still looking into this -- WIP.


This PR adds support for forced deletion of projects. If the --force flag is provided when attempting to delete a project, and if the user confirms that forced deletion is what they want, then all resources associated with the project will be deleted and the project will be removed.

This PR also simplifies the projectIsEmpty function to utilize projectUsedBy instead of checking each resource manually.

Includes cherry-picks from lxc/incus#900.

Closes: #14316.

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Oct 25, 2024
Copy link

Heads up @mionaalex - the "Documentation" label was applied to this issue.

@tomponline
Copy link
Member

@markylaing this is the PR I mentioned earlier wrt to doing loopback client requests, not @kadinsayani PR in the end. Apologies for confusion.

}
}

err = s.DB.Cluster.Transaction(r.Context(), func(ctx context.Context, tx *db.ClusterTx) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow this is a headache to read but I think I understand the reasoning. We want to use existing code to perform the deletion and not to reimplement any logic in the deletion handlers. We still want the handlers to perform all of their internal logic (e.g. forwarding storage volume/bucket requests to the member containing the volume and so on).

This is an example of where it would have been useful to have a design pattern in out API endpoints (such as MVC) where request parsing is separated from business logic.

As it stands I think this can be improved in the following ways:

  1. Get the project used by list.
  2. Parse this list entries with entity.ParseURL and categorise them.
  3. For each entity type, iterate over the entities, and create a *http.Request and call the appropriate handler with it.

We should also consider the features configuration on the project here. Force deleting a project should delete any networks in the default project.

@markylaing
Copy link
Contributor

@boltmark also could you please add some tests for this. Thanks :)

@boltmark
Copy link
Contributor Author

Hi @tomponline, looping you into this question I had --

I'm facing a bit of an issue invoking deletion endpoints locally with dummy requests. Mainly, we currently seem to rely on everything going through the HTTP request routing/handling since we need mux to inject variables into the request context. For example, we expect to be able to access these mux vars here: https://github.com/canonical/lxd/blob/main/lxd/network_acls.go#L311.

I'm able to inject mux vars into the dummy requests via mux.SetURLVars, but this function is pretty explicit about only being there for testing purposes. Am I missing something here? If not and this is in fact an issue, is some sort of refactoring of endpoints worth it for this?

CC @markylaing

MaheshPunjabi and others added 5 commits November 14, 2024 07:31
Signed-off-by: Mahesh Punjabi <[email protected]>
(cherry picked from commit c6ba40a405897c1ca75ced9f0e8ac8698d0d913f)
Signed-off-by: Mark Bolton <[email protected]>
License: Apache-2.0
Signed-off-by: Stéphane Graber <[email protected]>
(cherry picked from commit ac67c4fe8996ce48f0785f605d40e7638bbe4b40)
Signed-off-by: Mark Bolton <[email protected]>
License: Apache-2.0
Signed-off-by: Long Tran <[email protected]>
(cherry picked from commit eaed1d4fc35c4deab260ae4388fbdde8d2fdb95f)
Signed-off-by: Mark Bolton <[email protected]>
License: Apache-2.0
Signed-off-by: Stéphane Graber <[email protected]>
(cherry picked from commit 59e53d5206cd3e53b9c57ce18c1758b9f716a5d7)
Signed-off-by: Mark Bolton <[email protected]>
License: Apache-2.0
stgraber and others added 2 commits November 14, 2024 08:10
Signed-off-by: Stéphane Graber <[email protected]>
(cherry picked from commit 057428d8f73b42e3c5989b289f842ceff71fff83)
Signed-off-by: Mark Bolton <[email protected]>
License: Apache-2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants