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

Jump to bottom

update rook-ceph-mon endpoints cm #1673

Merged
merged 1 commit into from May 15, 2022
Merged

update rook-ceph-mon endpoints cm #1673

merged 1 commit into from May 15, 2022

Conversation

parth-gr
Copy link
Contributor

Currently the server was overriding the mon endpoints cm
with only 1 mon-IP,
Updated it to add all the 3 mon IPs

Signed-off-by: parth-gr paarora@redhat.com

@parth-gr parth-gr changed the title update rook-ceph-monendpoints cm update rook-ceph-mon endpoints cm May 11, 2022
@parth-gr
Copy link
Contributor Author

/retest-required ci/prow/red-hat-storage-ocs-ci-e2e-aws

@parth-gr
Copy link
Contributor Author

/retest-required

travisn
travisn reviewed May 11, 2022
View reviewed changes
services/provider/server/server.go Outdated
@@ -278,7 +278,7 @@ func (s *OCSProviderServer) getExternalResources(ctx context.Context, consumerRe
Name: monConfigMap,
Kind: "ConfigMap",
Data: mustMarshal(map[string]string{
"data": cmData[0], // Address of first mon
"data": strings.Join(cmData, ", "), // IP Address of all mon's
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure rook can handle the space. Here's a sample from a test cluster. And is the mon name included with the cmData?

data: a=10.110.186.73:6789,b=10.105.227.50:6789,c=10.104.4.237:6789
Suggested change
"data": strings.Join(cmData, ", "), // IP Address of all mon's
"data": strings.Join(cmData, ","), // IP Address of all mon's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yaa the mon Name would be included, as we are just unmarshalling the mon ConfigMap and appending it to external Resource

@parth-gr parth-gr requested a review from travisn May 11, 2022 14:55
@travisn
Copy link
Contributor

travisn commented May 11, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2022
nb-ohad
nb-ohad requested changes May 11, 2022
View reviewed changes
services/provider/server/server.go Outdated
@@ -278,7 +278,7 @@ func (s *OCSProviderServer) getExternalResources(ctx context.Context, consumerRe
Name: monConfigMap,
Kind: "ConfigMap",
Data: mustMarshal(map[string]string{
"data": cmData[0], // Address of first mon
"data": strings.Join(cmData, ","), // IP Address of all mon's
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to join information that was split just a few lines above

Suggested change
"data": strings.Join(cmData, ","), // IP Address of all mon's
"data": configmap.Data, // IP Address of all mon's

You can also remove the split above and change the condition to check empty string instead of len

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

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 11, 2022
@parth-gr
update rook-ceph-monendpoints cm
98144bf 
Currently the server was overriding the mon endpoints cm
with only 1 mon-IP,
Updated it to add all the 3 mon IPs

Signed-off-by: parth-gr <paarora@redhat.com>
@parth-gr parth-gr requested a review from nb-ohad May 12, 2022 06:50
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 12, 2022

@parth-gr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/red-hat-storage-ocs-ci-e2e-aws 98144bf link false /test red-hat-storage-ocs-ci-e2e-aws

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@nb-ohad
Copy link
Contributor

nb-ohad commented May 15, 2022

/lgtm
/hold

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 15, 2022
@nb-ohad
Copy link
Contributor

nb-ohad commented May 15, 2022

Added a hold to give @iamniting the opportunity to review as well

@iamniting
Copy link
Member

/lgtm

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, nb-ohad, parth-gr, travisn

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

The pull request process is described 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

@openshift-merge-robot openshift-merge-robot merged commit 73503d9 into red-hat-storage:main May 15, 2022
1 check passed
@agarwal-mudit
Copy link
Member

/cherry-pick release-4.11

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request May 16, 2022
Merged
@openshift-cherrypick-robot
Copy link

@agarwal-mudit: new pull request created: #1678

In response to this:

/cherry-pick release-4.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@iamniting
Copy link
Member

/cherry-pick release-4.10

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Nov 24, 2022
Merged
@openshift-cherrypick-robot
Copy link

@iamniting: new pull request created: #1891

In response to this:

/cherry-pick release-4.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

@travisn travisn Awaiting requested review from travisn

@nb-ohad nb-ohad Awaiting requested review from nb-ohad

Assignees

@travisn travisn

@nb-ohad nb-ohad

@iamniting iamniting

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@parth-gr @travisn @nb-ohad @iamniting @agarwal-mudit @openshift-cherrypick-robot @openshift-merge-robot