does this need to be logged since it will be returned as an error?

This was in the original code but unnecessary. Will remove.

Another negative test case could be not enough active zones

it's technically "not enough" replicaCount

this comment is not correct

ExpectedZone

not correct

I know ExpectedZone is redundant when ExpectSpecificZones is true, but may be good to explicitly call out anyway.

ExpectedZone

this error message seems to belong more inside the "ZonesToSet" method since its pretty specific, then it can just bubble up to here

I have added the error to ZonesToSet. Will adjust the error message in other consumers (like EBS/Azure) in a separate PR.

when returning error I think you can just return nil for the sets.String value

why are we dereferencing allowedZones here. I'm not sure we need to pass in "allowedTopologies" here, its only used in an error message in the function. But if you absolutely have to have it, make it a constant

I clearly missed the fact that nil can be used to indicate an empty set. Thanks for pointing that above. With that, no dereferencing is necessary and I will fix the code around this.

I was using the label to be able to print out meaningful error messages without having to process the errors in the callers. However I am not sure that saves me much - so I will get rid of this label and error string inside chooseZonesForVolumeIncludingZone and process the errors in the callers.

Could you also add unit tests for: chooseZonesForVolumeIncludingZone

For regional PD, the label value would be something like us-central1-a__us-central1-b

Thanks for catching this.

can this have a unit test?

Check whether requirements is empty

nit: Keep the same glog message as before: glog.V(2).Infof("error getting zone information from GCE: %v", err)

There was a review comment earlier to remove the log as the error already captures it. #67530 (comment) . Do we really need it?

Generally, we should avoid logging error messages that we also return because that creates duplicate log messages.

ACK

The volumeutil.ValidateZone() logic is missing here

Is it necessary? All ValidateZone is doing is making sure the string does not have spaces/tabs. The cloud API call for PD creation will fail for an invalid zone like that eventually. Maybe I can invoke ValidateZone on the list of returned zones from SelectZonesForVolume if it is important.

I think this is no longer needed, because it's captured by line 388 in pkg/volume/util/util.go, inside SelectZonesForVolume()?

return nil to be consistent

return nil to be consistent with the rest of SelectZonesForVolume().

What if zoneToInclude is not in zones?

If this function assumes zoneToInclude is always in zones, it'd be helpful to call it out in comments

The logic here makes sure zoneToInclude and zones is mutually exclusive. If zoneToInclude is set, zones should not be specified and that is what the logic here detects and error below calls out. To specify a list of "allowed" zones, the error also instructs that allowedTopologies should be specified with the list of allowed zones since that is integrated into the scheduler while zones is not.

Maybe you are referring to a different function? Both calls to choozeZonesForVolumeIncludingZone() pass in non-empty zones and they both allow for the possibility of non-empty zoneToInclude

Sorry - I was indeed referring to a different function. Agree with your comment above - choozeZonesForVolumeIncludingZone() does expect zoneToInclude to be part of zones and I will clarify that in a comment.

Should we validate that zoneFromNode is inside allowedZones before this call?

Not necessary since the scheduler guarantees that. I can call out the assumption explicitly in a comment.

zoneFromNode could be outside allowed zones

Can you clarify what you are referring to as allowed zones here? If you are referring to the zones parameter, then zonesParameterPresent need to be set and we check and make sure zonesParameterPresent is not specified in this code path up above in L349 (since the zones parameter is not integrated well with the scheduler). Unit test Node_with_Zone_labels_and_Zones_parameter_present covers this as well.

If you are referring to zones from allowedTopologies, then the scheduler guarantees the node selected has a zone (i.e. zoneFromNode) that is part of allowedTopologies. Maybe I can add a comment here to clarify this.

Sorry I was referring to allowedTopologies. Thanks for the explanation!

nit: Is it possible to do without some of these parameters? If we can reduce the input set, it'll help guarantee test coverage and possibly improve code maintainability

since a lot of these parameters are mutually exclusive to each other, it might be worth having a SelectZonesForVolume that only takes in one of these params called zones, and a wrapper function that does all this logic to determine mutual exclusivity as well as picking which of the zone|zones|topology to pass to SelectZonesForVolume

@verult The idea behind SelectZonesForVolume is all the in-tree providers like EBS, Azure and GCE PD should be able to parse out volume zone and topology parameters from the storage class, invoke SelectZoneForVolume/SelectZonesForVolume and get a (set of) volume(s) back to pass to the cloud provider API without having to duplicate any of the logic to enforce the rules. So SelectZonesForVolume is the wrapper that implements all the common logic to determine mutual exclusivity and for that it needs the full set of parameters around zone/zones/node/topology. I agree that the long list of parameters is not pretty but to help with maintenance, we have an extensive set of unit tests in TestSelectZonesForVolume and TestSelectZoneForVolume. Those should make sure none of the rules around exclusivity of the parameters are broken.

@davidz627 SelectZonesForVolume is the wrapper that enforces the compatibility rules among the parameters. chooseZonesForVolumeIncludingZone/ChooseZonesForVolume invoked from SelectZonesForVolume does the actual picking of zone(s) from a set of zones. Let me know if it makes sense and already aligns with the above suggestion.

nit: wrap error here

can we go straight from label to list here instead of going through an intermediate set?

since a lot of these parameters are mutually exclusive to each other, it might be worth having a SelectZonesForVolume that only takes in one of these params called zones, and a wrapper function that does all this logic to determine mutual exclusivity as well as picking which of the zone|zones|topology to pass to SelectZonesForVolume

nit: doesn't have to be in else block

Is zoneFromNode only being set inside the scope of this block?

Yes this is the only place zoneFromNode is set.

Sorry I meant that by using :=, I think zoneFromNode is redeclared inside the block and the variable outside the if branch isn't actually being set.

Good catch! This is something I clearly missed. I was incorrectly thinking that := will just initialize the ok and honor zoneFromNode from the broader scope. But as you mentioned, zoneFromNode gets reinitialized due to :=. The unit-test to detect this was also getting lucky it seems - will fix that too.

"<= zones.Len()"

nit: use positive expressions when possible, especially here this feels a little backwards

instead, maybe:

if r.Key == key{
  return &r, nil
}

nit: var ok bool