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.

Sign up for GitHub

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

Jump to bottom

Consumer deletion #154

Merged
merged 15 commits into from
Jul 11, 2024
Merged

Consumer deletion #154

merged 15 commits into from
Jul 11, 2024

Conversation

yanmxa
Copy link
Contributor

@yanmxa yanmxa commented Jul 3, 2024 •
edited
Loading

Signed-off-by: myan [email protected]

Note:

Delete will remove the consumer from the storage:

  1. Perform a hard delete on the consumer, the resource creation will be blocked by it.
  2. Forbid consumer deletion if there are associated resources(include the marked as deleted resources).

TODO: Additional deletion options or strategies might be added in the future.

@yanmxa yanmxa force-pushed the br_delete_consumer branch 2 times, most recently from a6f86da to b0fa117 Compare July 3, 2024 08:51
@yanmxa yanmxa changed the title [ WIP ] Consumer deletion Consumer deletion Jul 4, 2024
@yanmxa yanmxa force-pushed the br_delete_consumer branch 2 times, most recently from b1c649f to a059a24 Compare July 4, 2024 03:48
@yanmxa
Copy link
Contributor Author

yanmxa commented Jul 4, 2024

/ok-to-test

@yanmxa yanmxa force-pushed the br_delete_consumer branch from aff98b5 to ed27d5b Compare July 5, 2024 03:33
@yanmxa
Copy link
Contributor Author

yanmxa commented Jul 5, 2024

/assign @qiujian16 @morvencao @clyang82

@yanmxa
Copy link
Contributor Author

yanmxa commented Jul 5, 2024

The e2e case might sometimes fail with the same error in this PR #156. It has no relation to the current changes and won't be fixed here.

qiujian16
qiujian16 reviewed Jul 5, 2024
View reviewed changes
pkg/dao/resource.go Outdated
@@ -115,3 +116,10 @@ func (d *sqlResourceDao) All(ctx context.Context) (api.ResourceList, error) {
}
return resources, nil
}

func (d *sqlResourceDao) FirstByConsumerName(ctx context.Context, consumerName string) (api.Resource, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why first?

Copy link
Contributor Author

@yanmxa yanmxa Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just chose one(first) item to determine whether the resource exists for the consumer. Of course, we can choose to use the current FindByConsumerName to do that. The latter may need more memory footprint, cause it will load all the resources on the consumer to the memory. What do you think of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think we need to document this with the reason. Also what if the resource is marked as deleted or deleting? do we still block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added comments for the FirstByConsumerName.

If a resource is marked as deleted, we will still block the deletion.

qiujian16
qiujian16 reviewed Jul 5, 2024
View reviewed changes
pkg/services/consumer.go Outdated

if e != gorm.ErrRecordNotFound {
return handleGetError("Resource", "consumer_name", consumer.Name, e)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

return error with other type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pkg/services/consumer.go Outdated
return handleGetError("Resource", "consumer_name", consumer.Name, e)
}

// e is record not found
Copy link
Contributor

Choose a reason for hiding this comment

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

something to consider is, we need to block creation of resource before delete consumer.

Copy link
Contributor Author

@yanmxa yanmxa Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @qiujian16!

I think of two ways to block the resource creation when deleting the consumer.

Optional 1(the current): Hard delete the consumer, so there will be no deleting state for the consumer. And the resource creation will be blocked by the deleted consumer.

Optional 2: Delete the consumer softly(mark it as deleted), and add validation to check the consumer when creating resources. If the consumer is not found or marked as deleted, then block the resource creating.
This doesn't seem like an elegant way, as it requires checking each resource creation to ensure that.

The Optional 1 might be feasible for now. Do you have any other suggestions?

@yanmxa yanmxa force-pushed the br_delete_consumer branch 3 times, most recently from d404abb to 604637e Compare July 5, 2024 15:27
@clyang82
Copy link
Contributor

clyang82 commented Jul 8, 2024

/ok-to-test

@yanmxa yanmxa force-pushed the br_delete_consumer branch from b76fa9b to baf8e76 Compare July 9, 2024 03:25
@yanmxa
update comment
b68cc08 
Signed-off-by: myan <[email protected]>
morvencao
morvencao reviewed Jul 9, 2024
View reviewed changes
pkg/services/consumer.go Outdated Show resolved Hide resolved
@yanmxa yanmxa force-pushed the br_delete_consumer branch from 021a1f0 to 5183cee Compare July 9, 2024 07:33
@yanmxa yanmxa force-pushed the br_delete_consumer branch from 5183cee to b3db85a Compare July 9, 2024 08:51
@yanmxa
add conmment
87b42e3 
Signed-off-by: myan <[email protected]>
@yanmxa yanmxa force-pushed the br_delete_consumer branch 4 times, most recently from 04f7315 to 8c6acbc Compare July 10, 2024 06:06
@yanmxa yanmxa force-pushed the br_delete_consumer branch from 8c6acbc to e687ffc Compare July 10, 2024 06:20
@yanmxa yanmxa requested review from clyang82 and qiujian16 July 10, 2024 08:21
@yanmxa yanmxa force-pushed the br_delete_consumer branch 8 times, most recently from 5789d89 to 83a2b71 Compare July 11, 2024 06:17
@yanmxa
rerun
4fa474d 
Signed-off-by: myan <[email protected]>
@yanmxa yanmxa force-pushed the br_delete_consumer branch from 83a2b71 to 4fa474d Compare July 11, 2024 06:48
@yanmxa
rerun
aaf25f7 
Signed-off-by: myan <[email protected]>
@clyang82 clyang82 merged commit 3c02243 into openshift-online:main Jul 11, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clyang82 clyang82 clyang82 left review comments

@morvencao morvencao morvencao left review comments

@qiujian16 qiujian16 Awaiting requested review from qiujian16

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yanmxa @clyang82 @qiujian16 @morvencao