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

provider: fix clientProfileName calculation #2932

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

provider: fix clientProfileName calculation #2932

wants to merge 1 commit into from

Conversation

rewantsoni
Copy link
Member

@rewantsoni rewantsoni commented Dec 9, 2024 •
edited
Loading

For the clientProfileMapping sent from provider to the client,

  1. use localClientId for localClientProfile name
  2. use remoteClientId for remoteClinetProfile name

@rewantsoni rewantsoni force-pushed the fix-client-profile-mapping branch from 9291720 to 8defa00 Compare December 9, 2024 07:34
leelavg
leelavg reviewed Dec 9, 2024
View reviewed changes
services/provider/server/server.go Outdated
Comment on lines 469 to 471
// Using the same hash function we have in client op to calculate the clientProfile Name
clientProfileHash := md5.Sum([]byte(fmt.Sprintf("%s-ceph-rbd", clientName)))
clientProfileName := hex.EncodeToString(clientProfileHash[:])
Copy link
Contributor

Choose a reason for hiding this comment

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

so in provider before hashing we are turning them into a slice 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes right

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2024
@rewantsoni
Copy link
Member Author

/retest-required

@nb-ohad
Copy link
Contributor

nb-ohad commented Dec 9, 2024

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2024
@rewantsoni rewantsoni force-pushed the fix-client-profile-mapping branch from 8defa00 to 3422906 Compare December 9, 2024 14:34
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 9, 2024

New changes are detected. LGTM label has been removed.

nb-ohad
nb-ohad requested changes Dec 10, 2024
View reviewed changes
services/provider/server/server.go
return "", client.IgnoreNotFound(err)
}

return clientMappingConfig.Data[consumer.Status.Client.ID], nil
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the key or the value does not exist? are we still going to return an empty value? and if so who is going to handle such a case?

@rewantsoni
provider: fix clientProfileName calculation
7b1164e 
use localClientId for localClientProfile name
use remoteClientId for remoteClinetProfile name

Signed-off-by: Rewant Soni <resoni@redhat.com>
@rewantsoni rewantsoni force-pushed the fix-client-profile-mapping branch from 3422906 to 7b1164e Compare December 11, 2024 04:49
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: leelavg, rewantsoni
Once this PR has been reviewed and has the lgtm label, please ask for approval from nb-ohad. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rewantsoni
Copy link
Member Author

Testing:

For the storage-client-mapping with data:
apiVersion: v1
data:
  1e99f74c-0ac7-419f-a4ca-587855a0bc17: f21e9544-1821-4834-b46b-49ac4c3db583
  92a5fe4b-c235-4b3b-9587-6c99f8564c8b: 60e9029b-e2fd-4664-8a4d-a1256de30b9c
kind: ConfigMap
metadata:
  name: storage-client-mapping

[rewantsoni tmp] grpcurl -insecure -d '{"storageConsumerUUID":["d0b0dde8-e3dd-4f53-8bc6-4e1de1ec4b5d"]}' localhost:50051 provider.OCSProvider/GetStorageConfig
{
....
    {
      "Name": "ocs-storagecluster",
      "kind": "ClientProfileMapping",
      "Data": "eyJtYXBwaW5ncyI6W3sibG9jYWxDbGllbnRQcm9maWxlIjoiMWU5OWY3NGMtMGFjNy00MTlmLWE0Y2EtNTg3ODU1YTBiYzE3IiwicmVtb3RlQ2xpZW50UHJvZmlsZSI6ImYyMWU5NTQ0LTE4MjEtNDgzNC1iNDZiLTQ5YWM0YzNkYjU4MyIsImJsb2NrUG9vbElkTWFwcGluZyI6W1siMSIsIjEiXV19XX0="
    }
  ],
  "desiredConfigHash": "573c3bf70695005cececd193341f4099",
  "systemAttributes": {
    "MirrorEnabled": true
  }
}

decoding the Data for ClientProfileMapping we get: 
{"mappings":[{"localClientProfile":"1e99f74c-0ac7-419f-a4ca-587855a0bc17","remoteClientProfile":"f21e9544-1821-4834-b46b-49ac4c3db583","blockPoolIdMapping":[["1","1"]]}]}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nb-ohad nb-ohad nb-ohad requested changes

@leelavg leelavg leelavg approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@nb-ohad nb-ohad

@leelavg leelavg

Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rewantsoni @nb-ohad @leelavg