require working maestro reading to consider cluster create successful - #4695
Conversation
d2d388c to
6daf8c1
Compare
bbb5efb to
dd0cbb7
Compare
| } | ||
| } | ||
|
|
||
| var provisioningStatePriority = map[arm.ProvisioningState]int{ |
There was a problem hiding this comment.
Is the idea of this that there are states from different areas/resources being calculated, and we want to only return one, and we define this to define which one we return when there are "competing/different" states between the resources?
What was the criteria on deciding the priority? I understand that we want to give priority to failure cases vs succeed cases but there are some where the line is a bit more gray like updating, accepted, provisioning
There was a problem hiding this comment.
Yes that is the idea. The priority is chosen based on, "which status is most useful for customers".
There was a problem hiding this comment.
Will add a comment into the code.
| if err := errors.Join(errs...); err != nil { | ||
| return nil, err | ||
| } | ||
| if len(operationStates) == 0 { |
There was a problem hiding this comment.
Is this check needed? i there were errors the check in line 226 should occur and return. If there were no errors we are guaranteed to have at least one element.
| return nil, errors.New("empty provisioning state") | ||
| } | ||
|
|
||
| return operationStates[0], nil |
There was a problem hiding this comment.
Because here we return the state with the most priority (lower integer number), should we add some logging so we can see the N states calculated to aid with debugging?
There was a problem hiding this comment.
good point, will add.
| } | ||
|
|
||
| slices.SortStableFunc(operationStates, compareOperationState) | ||
| if operationStates[0] == nil { |
There was a problem hiding this comment.
Can an operationState in the array be nil?
There was a problem hiding this comment.
people will mess up.
| if operationStates[0] == nil { | ||
| return nil, errors.New("nil operation state") | ||
| } | ||
| if len(operationStates[0].provisioningState) == 0 { |
There was a problem hiding this comment.
Can an operationState provisioningState be empty?
There was a problem hiding this comment.
its' not desired, but I had a couple accidents.
| continue | ||
| } | ||
| currMajorMinor := fmt.Sprintf("%d.%d", currVersion.Major, currVersion.Minor) | ||
| if minVersion, ok := minVersionsWithValidSuccessCondition[currMajorMinor]; ok && currVersion.LT(minVersion) { |
There was a problem hiding this comment.
Could you expand on the meaning of the map, and the usage of sentinel values? I am trying to understand its purpose.
| } | ||
|
|
||
| if anyVersionWithValidSuccessCondition { | ||
| // can only check this when the success condition works, because this is unreliable otherwise |
There was a problem hiding this comment.
Can the HostedClusterAvailable condition be true if there hasn't been any control plane version completed?
There was a problem hiding this comment.
Maybe. This is only used to report provisioning, not success.
| } | ||
| } | ||
|
|
||
| if len(hostedCluster.Status.ControlPlaneEndpoint.Host) == 0 { |
There was a problem hiding this comment.
Is ControlPlaneEndpoint the same as the K8s API URL that we store in the ServiceProviderCluster (.api.url) ?
There was a problem hiding this comment.
Not sure, but I figure so. Seems like we should have one.
164ac19 to
6ccc5c3
Compare
283d15a to
9b1e8ad
Compare
9b1e8ad to
3a0b8ba
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, miguelsorianod The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
David Eads (@deads2k): The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
/test e2e-parallel |
4 participants
New cluster have to pass a check of the HostedCluster itself to confirm that they are working. This is currently in addition to cluster-service. Eventually we will remove the check of cluster-service consistency.