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

Reduce time complexity by modifying the data structure #26

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

hlts2
Copy link
Member

@hlts2 hlts2 commented Nov 6, 2024

WHAT

Modified the data structure to reduce the time complexity of the access mode existence check for volumes during RPC calls. This enables existence checks to be performed in O(1) time.

WHY

A loop was used each time to check if there was a matching access mode for Volume. Currently, the loop only runs twice, so it's not an issue, but optimizing to O(1) check is better in case the number of access modes for Volume increases in the future.

Below is a list of possible options.

- csi.pb.go
  - VolumeCapability_AccessMode_SINGLE_NODE_WRITER
  - VolumeCapability_AccessMode_SINGLE_NODE_READER_ONLY
  - VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY
  - VolumeCapability_AccessMode_MULTI_NODE_SINGLE_WRITER
  - VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER 
  - VolumeCapability_AccessMode_SINGLE_NODE_SINGLE_WRITER
  - VolumeCapability_AccessMode_SINGLE_NODE_MULTI_WRITER

FYI

This PR includes only changes to the data structure, so no logic modifications have been made 🙏
Additionally, the supportedAccessModes variable was not used anywhere outside of pkg/driver/controller_server.go, so there were no changes made in other areas.

❯ rg supportedAccessModes
pkg/driver/controller_server.go
22:var supportedAccessModes = map[csi.VolumeCapability_AccessMode_Mode]struct{}{
43:		if _, ok := supportedAccessModes[cap.GetAccessMode().GetMode()]; !ok {
469:		if _, ok := supportedAccessModes[cap.GetAccessMode().GetMode()]; ok {

The following is the test result.

❯ go test -v ./controller_server_test.go
=== RUN   TestCreateVolume
=== RUN   TestCreateVolume/Create_a_default_size_volume
{"level":"info","api_url":"https://civo-api.example.com","region":"TEST1","namespace":"default","cluster_id":"12345678","socketFilename":"unix:///var/lib/kubelet/plugins/civo-csi/csi.sock","user_agent":"civo-csi","time":"2024-11-07T03:07:29+09:00","message":"Created a new driver"}
=== RUN   TestCreateVolume/Disallow_block_volumes
=== RUN   TestCreateVolume/Create_a_specified_size_volume
=== RUN   TestCreateVolume/Don't_create_if_the_volume_already_exists_and_just_return_it
--- PASS: TestCreateVolume (0.00s)
    --- PASS: TestCreateVolume/Create_a_default_size_volume (0.00s)
    --- PASS: TestCreateVolume/Disallow_block_volumes (0.00s)
    --- PASS: TestCreateVolume/Create_a_specified_size_volume (0.00s)
    --- PASS: TestCreateVolume/Don't_create_if_the_volume_already_exists_and_just_return_it (0.00s)
=== RUN   TestDeleteVolume
=== RUN   TestDeleteVolume/Delete_a_volume
--- PASS: TestDeleteVolume (0.00s)
    --- PASS: TestDeleteVolume/Delete_a_volume (0.00s)
=== RUN   TestControllerPublishVolume
=== RUN   TestControllerPublishVolume/Publish_a_volume
--- PASS: TestControllerPublishVolume (5.00s)
    --- PASS: TestControllerPublishVolume/Publish_a_volume (5.00s)
=== RUN   TestControllerUnpublishVolume
=== RUN   TestControllerUnpublishVolume/Unpublish_a_volume_if_attached_to_the_correct_node
=== RUN   TestControllerUnpublishVolume/Doesn't_unpublish_a_volume_if_attached_to_a_different_node
--- PASS: TestControllerUnpublishVolume (5.00s)
    --- PASS: TestControllerUnpublishVolume/Unpublish_a_volume_if_attached_to_the_correct_node (5.00s)
    --- PASS: TestControllerUnpublishVolume/Doesn't_unpublish_a_volume_if_attached_to_a_different_node (0.00s)
=== RUN   TestListVolumes
=== RUN   TestListVolumes/Lists_available_existing_volumes
--- PASS: TestListVolumes (0.00s)
    --- PASS: TestListVolumes/Lists_available_existing_volumes (0.00s)
=== RUN   TestGetCapacity
=== RUN   TestGetCapacity/Has_available_capacity_from_usage_and_limit
=== RUN   TestGetCapacity/Has_no_capacity_from_usage_and_limit
=== RUN   TestGetCapacity/Has_no_capacity_from_volume_count_limit
--- PASS: TestGetCapacity (0.00s)
    --- PASS: TestGetCapacity/Has_available_capacity_from_usage_and_limit (0.00s)
    --- PASS: TestGetCapacity/Has_no_capacity_from_usage_and_limit (0.00s)
    --- PASS: TestGetCapacity/Has_no_capacity_from_volume_count_limit (0.00s)
PASS
ok  	command-line-arguments	10.012s

@hlts2 hlts2 self-assigned this Nov 6, 2024
@hlts2 hlts2 changed the title Ruduce time complexity by modifying the data structure Reduce time complexity by modifying the data structure Nov 6, 2024
Copy link
Member

@rytswd rytswd left a comment

Choose a reason for hiding this comment

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

Thanks! Small and iterative changes which we all love 😆

@hlts2
Copy link
Member Author

hlts2 commented Nov 8, 2024

@Praveen005 @rytswd
Thank you for reviewing it!
I will merge this PR 🚀

@hlts2 hlts2 merged commit eef1a8b into master Nov 8, 2024
4 checks passed
@hlts2 hlts2 deleted the fix/reduce-time-complexity branch November 8, 2024 08:51
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.

3 participants