allow missing cluster service cluster ID - #4752
Conversation
|
Skipping CI for Draft Pull Request. |
| database.OperationRequestCreate, | ||
| newInternalCluster.ID, | ||
| newInternalCluster.ServiceProviderProperties.ClusterServiceID, | ||
| *newInternalCluster.ServiceProviderProperties.ClusterServiceID, |
There was a problem hiding this comment.
need to track down operation clsuter-service-id usage to handle empty.
|
Jakob Gray (@JakobGray) PTAL, this is relevant for the work around moving cluster creation to the backend. /assign Jakob Gray (@JakobGray) |
dd66af0 to
9728a5a
Compare
9728a5a to
1f0df3d
Compare
|
/retest |
| // we do this to keep serialization the same so that we can go to n-1 where this field isn't a pointer. | ||
| // on the reading side, we handle the pointer as expected. | ||
| cosmosObj.InternalState.InternalAPI.ServiceProviderProperties.ClusterServiceID = &api.InternalID{} |
There was a problem hiding this comment.
critical part of the PR
| // TODO should we take into account that at some point in the future we will implement migration between management | ||
| // clusters, where a cluster could have bundles allocated to different provision shards at the same time? | ||
| clusterCSShard, err := c.clusterServiceClient.GetClusterProvisionShard(ctx, cluster.ServiceProviderProperties.ClusterServiceID) | ||
| clusterCSShard, err := c.clusterServiceClient.GetClusterProvisionShard(ctx, *cluster.ServiceProviderProperties.ClusterServiceID) |
There was a problem hiding this comment.
1f0df3d to
e93d6fc
Compare
| } | ||
| clustersByClusterServiceID := make(map[string]*api.HCPOpenShiftCluster) | ||
| for _, internalCluster := range internalClusterIterator.Items(ctx) { | ||
| if internalCluster.ServiceProviderProperties.ClusterServiceID == nil { |
There was a problem hiding this comment.
Does this mean ArmResourceListClusters will omit clusters that still don't have the CS ID set? it could be confusing to consumers of the API endpoint, just to be aware
There was a problem hiding this comment.
Does this mean ArmResourceListClusters will omit clusters that still don't have the CS ID set? it could be confusing to consumers of the API endpoint, just to be aware
It goes away in #4610
| logger.Info("clusterService cluster missing, trying to clean up", "err", err) | ||
| } else if err != nil { | ||
| return utils.TrackError(err) | ||
| if cluster.ServiceProviderProperties.ClusterServiceID != nil { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Similar answer
| // TODO this overwrite will transformed into a "set" function as we transition fields to ownership in cosmos | ||
| // TODO remove the azure location once we have migrated every record to store the location | ||
| func mergeToInternalCluster(csCluster *arohcpv1alpha1.Cluster, internalCluster *api.HCPOpenShiftCluster, azureLocation string) (*api.HCPOpenShiftCluster, error) { | ||
| if csCluster == nil { |
There was a problem hiding this comment.
Have we analyzed the implications of this nil check and error in mergeToInternalCluster and readInternalClusterFromClusterService from consumer code?
There was a problem hiding this comment.
Function goes away in #4610, which has to merge before actually write the nil, so it will self-solve
cefc148 to
780f1e0
Compare
This ensures the process hangs and doesn't fail if we run n-1 backend after removing synchronous creation. We need to remember to first add the controller handling.
780f1e0 to
a288ef3
Compare
|
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. |
|
/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 |
4 participants
Necessary for moving creation to the backend.
Critically, we never serialize a nil, we keep it as an empty. This allows the n-1 level (if we need to revert) to read data created by the new version. This version reads and tolerates nil, so the n+1 version can write the nil.