validate if cluster FSID is empty #185
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: umangachapagain 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 |
| } | ||
|
|
||
| if len(fsids) < 2 { | ||
| return "", fmt.Errorf("replicationID can not be generated due to missing cluster FSID") |
There was a problem hiding this comment.
Add which cluster ID we are actually missing. It should be present as the key in clusterFSIDs
There was a problem hiding this comment.
There could be multiple and I didn't want to do a bunch of string manipulation. Since this is a very corner case, can we keep that as a follow up item?
There was a problem hiding this comment.
String manipulation may not be required if you add it in the above loop. We can check if something is empty and just log that this spoke cluster's fsid is nil.
There was a problem hiding this comment.
or it can done in the place where we are fetching the clusterFSIDs. It should be easier and will make more sense to log that
| fsids = append(fsids, v) | ||
| replicationId, err := utils.CreateUniqueReplicationId(clusterFSIDs) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Returning an error will keep it in continous reconcilation and will keep printing the logs.
Do we want to log the info and reconcile at a later interval ?
There was a problem hiding this comment.
Error loop is fine I guess, since we do not expect it to be for long. Also, better to error out that a manual restart.
When generating replicationID in CreateUniqueReplicationId(), ensure that at least 2 non-empty cluster FSID strings are available. Also, refactor to include sorting logic to enure consistent hashing. Signed-off-by: Umanga Chapagain <[email protected]>
In some cases, FSID can be nil and later be populated. For such cases, we need to ensure that ManifestWork for VRC is updated with newly generated replication ID. Signed-off-by: Umanga Chapagain <[email protected]>
umangachapagain
force-pushed
the
fsid-mismatch
branch
from
9dd7569 to
5d1c029
Compare
|
LGTM! Looping on the reconcile should be fine, is there a max reconcile exponential backoff set for this reconciler? Asking to see if this could result in a backoff due to cluster claims not present for a larger interval. |
@ShyamsundarR We use the defaults. Not sure what those values are. |
|
/lgtm |
|
/unhold |
openshift-merge-bot
bot
merged commit 1b647ef
into
red-hat-storage:main
|
/cherry-pick release-4.14 |
|
@umangachapagain: new pull request created: #186 In response to this:
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. |
When generating replicationID in CreateUniqueReplicationId(), ensure that at least 2 non-empty cluster FSID strings are available. Also, refactor to include sorting logic to enure consistent hashing.