Add status sub-resource to ResourceExport/Import and ClusterSet #2367
Conversation
|
@suwang48404 let me know if this looks good to you. I have incorporated your suggestion in to this. |
|
also run "make manifest" so that CRD files and manifests are updated accordingly, |
Signed-off-by: abhiraut <[email protected]>
Signed-off-by: abhiraut <[email protected]>
done |
|
|
||
| const ( | ||
| // ClusterSetReady indicates whether ClusterSet is ready. | ||
| ClusterSetReady ClusterSetConditionType = "ClusterSetReady" |
There was a problem hiding this comment.
I feel like this is typically just "Ready" in K8s APIs. Am I wrong? Maybe we can avoid repeating the resource name in the status name: Ready, IsLeader, ImportSucceeded, etc.
There was a problem hiding this comment.
i saw a mix in Pod, where there is Ready and PodScheduled. but lets do Ready.
| Type ResourceImportConditionType `json:"type,omitempty"` | ||
| // Status of the condition, one of True, False, Unknown | ||
| Status v1.ConditionStatus `json:"status,omitempty"` | ||
| ClusterID string `json:"clusterID,omitempty"` |
There was a problem hiding this comment.
- I'd suggest to extract ClusterID out, and put it in ResourceImportClusterStatus; where I suppose each ResourceImportClusterStatus reports import status from one cluster?
- consider rename ResourceImportClusterCondition to ResourceImportCondition to match ResourceExportCondition?
There was a problem hiding this comment.
done. I also updated the ClusterStatus to include the clusterID in there.
There was a problem hiding this comment.
Guess the difference is: ResourceExportStatus is for a single cluster, but ResourceImportStatus includes multiple clusters. But agree the naming might cause some confusion.
There was a problem hiding this comment.
agreed. updated in the latest commit
| Type ResourceImportConditionType `json:"type,omitempty"` | ||
| // Status of the condition, one of True, False, Unknown | ||
| Status v1.ConditionStatus `json:"status,omitempty"` | ||
| ClusterID string `json:"clusterID,omitempty"` |
There was a problem hiding this comment.
Guess the difference is: ResourceExportStatus is for a single cluster, but ResourceImportStatus includes multiple clusters. But agree the naming might cause some confusion.
| // Important: Run "make" to regenerate code after modifying this file | ||
| // TBD | ||
| // Total number of member clusters configured in the set. | ||
| TotalClusters int32 `json:"totalClusters,omitempty"` |
There was a problem hiding this comment.
Could we calculate from member clusters and their status? Maybe we just like a more convenient way?
| // Total number of clusters ready and connected. | ||
| ReadyClusters int32 `json:"readyClusters,omitempty"` | ||
| // The overall condition of the cluster set. | ||
| Conditions []ClusterSetCondition `json:"conditions,omitempty"` |
There was a problem hiding this comment.
Probably I did not follow all discussions here, but could we know the overall status from the member cluster cluster status and conditions?
There was a problem hiding this comment.
we can.. i think this was for convenience, a bit like how Deployment has some overall status at the top and more details in conditions
Signed-off-by: abhiraut <[email protected]>
|
@suwang48404 any more comments from you? |
| // Total number of clusters ready and connected. | ||
| ReadyClusters int32 `json:"readyClusters,omitempty"` | ||
| // The overall condition of the cluster set. | ||
| Conditions []ClusterSetCondition `json:"conditions,omitempty"` |
| ) | ||
|
|
||
| // ResourceImportCondition indicates the condition of the ResourceImport in a cluster | ||
| type ResourceImportCondition struct { |
There was a problem hiding this comment.
But do you think later we can have overall ResourceImportCondition, in addition to the per Cluster Conditions? I do not have an opinion what naming is better; just like to check what you think.
There was a problem hiding this comment.
that was my original intention that's why named it with Cluster in it originally. Maybe if we decide to introduce overall status later, something like ResourceImportOverallStatus or ResourceImportClusterSetStatus, if we want to keep this (ResourceImportCondition) naming today to be consistent with the ResourceExportCondition
|
@antoninbas @tnqn @jianjuns any more comment before we merge this? |
Not from me. |
| ResourceExportSucceeded ResourceExportConditionType = "Succeeded" | ||
| ) | ||
|
|
||
| // ResourceExportCondition indicates the readiness condition of the ResourceExport |
There was a problem hiding this comment.
nit: missing trailing dot in several places.
Signed-off-by: abhiraut <[email protected]>
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
Status sub-resource for multi cluster CRDs
Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs
…ea-io#2367) Status sub-resource for multi cluster CRDs Signed-off-by: Abhishek Raut <[email protected]>
…ea-io#2367) Status sub-resource for multi cluster CRDs Signed-off-by: Abhishek Raut <[email protected]>
Status sub-resource for multi cluster CRDs Signed-off-by: Abhishek Raut <[email protected]>
…ea-io#2367) Status sub-resource for multi cluster CRDs Signed-off-by: Abhishek Raut <[email protected]>
…ea-io#2367) Status sub-resource for multi cluster CRDs Signed-off-by: Abhishek Raut <[email protected]>
…ea-io#2367) Status sub-resource for multi cluster CRDs Signed-off-by: Abhishek Raut <[email protected]>
Signed-off-by: abhiraut [email protected]