STOR-1049: Add topology aware sc #121
Conversation
| return nil, fmt.Errorf("failed to access datacenter %s: %s", dcName, err) | ||
| } | ||
|
|
||
| finder = find.NewFinder(vmClient.Client, false) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied 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 |
openshift-ci
bot
added
the
approved
gnufied
force-pushed
the
add-topology-aware-sc
branch
from
889a905
to
af44173
Compare
|
/retest |
|
@gnufied: The following tests failed, say
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. |
| err = v.createOrUpdateTag(ctx, ds) | ||
| if err != nil { | ||
| return v.policyName, fmt.Errorf("error creating or updating tag %s: %v", v.tagName, err) | ||
| } |
There was a problem hiding this comment.
Instead of stopping the execution, should it try the next one?
There was a problem hiding this comment.
I think it might be better to hard error out, rather than continuing with tagging whatever datastores we could access, because that could result in a storagepolicy which has unpredictable behaviour.
There was a problem hiding this comment.
My point was when you have say 5 failureDomains/datastores, then you successfully tag 3 of them and the 4th fail. Sounds like it's better to either:
- Try tagging all of them and return an aggregated error
- Return early in case of error, but don't leave behind tagged datastores
There was a problem hiding this comment.
okay I have aggregated the errors and returned them. I think that is a good idea in case we have multiple datastores and we don't have access to them, so as rather than returning one error at a time - we will return error about all inaccessible datastores at the same time.
I have pushed that change to - #125 however, which also includes #121
There was a problem hiding this comment.
Just a comment related to error aggregation, otherwise LGTM (not tagging to give @jsafrane a chance to review).
| vSphereInfraConfig := v.infra.Spec.PlatformSpec.VSphere | ||
| if vSphereInfraConfig != nil && len(vSphereInfraConfig.FailureDomains) > 1 { | ||
| return v.createZonalStoragePolicy(ctx) | ||
| } | ||
|
|
There was a problem hiding this comment.
createZonalStoragePolicy and rest of createStoragePolicy below are almost the same, createZonalStoragePolicy only loops / tags over more datastores. Would it be possible to join them together somehow? Like getZonalDatastores() + getDefaultDatastore() and then a common tagging loop + createStorageProfile over the result.
There was a problem hiding this comment.
okay I have unified that code. But I have pushed my new changes to https://github.com/openshift/vmware-vsphere-csi-driver-operator/pull/125/files#diff-cd720ec01eefb8566ee378bc53aef398da5550c725aeacd65aba7efb2b39e311R146
That branch already includes commit from this branch and has more tests and ensures tags are recreated if deleted etc.
| if vSpherePlatformConfig != nil { | ||
| failureDomains := vSpherePlatformConfig.FailureDomains | ||
| if len(failureDomains) > 0 { | ||
| return []string{defaultOpenshiftZoneCategory, defaultOpenshiftRegionCategory} |
There was a problem hiding this comment.
I'd prefer a warning when both infra and clusterCSIDriver specify topology. Maybe with a metric + alert, but that probably does not belong to this function.
There was a problem hiding this comment.
ack. I can file this as a story attached to the same epic in jira.
There was a problem hiding this comment.
|
I am going to close this favour of #125 /close |
|
@gnufied: Closed this PR. 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. |
Fixes https://issues.redhat.com/browse/STOR-1049