Add additional printer columns for CRDs #2881

zubron · 2020-08-31T21:43:16Z

This change modifies the kubebuilder annotations for the Velero CRDs to
include additionalPrinterColumns so that more information is exposed
when using kubectl get.

For each of the CRDs, annotations have been added to make the output
for kubectl get match the output from the equivalent velero get
command as closely as possible. There are some cases where this output
could not be replicated, such as the EXPIRES column for Backups, due
to the limitations of JSONPath expressions within the resulting CRD
defition. Some columns undergo processing and formatting before being
printed by the Velero CLI which cannot be replicated using JSONPath. In
these cases, these printer columns have been omitted.

For other CRDs where there is no velero get equivalent, such as
PodVolumeBackup and PodVolumeRestore, a best effort has been made to
expose information that provides value.

Fixes #1780

Signed-off-by: Bridget McErlean [email protected]

nrb · 2020-09-01T17:39:05Z

@zubron Since we've locked the v1.5.0 list except for critical fixes, I'm going to mark this for v1.5.1 in the GitHub milestones.

ashish-amarnath

@zubron I added a few minor comments. PTAL

ashish-amarnath · 2020-09-10T19:28:54Z

pkg/apis/velero/v1/backup.go

+// +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase",description="Backup status such as InProgress/Completed"
+// +kubebuilder:printcolumn:name="Errors",type="integer",JSONPath=".status.errors",description="Count of all error messages that were generated during execution of this backup"
+// +kubebuilder:printcolumn:name="Warnings",type="integer",JSONPath=".status.warnings",description="Count of all warning messages that were generated during execution of this backup"
+// +kubebuilder:printcolumn:name="Created",type="date",JSONPath=".status.startTimestamp",description="Time when this backup was started"


suggest: Start Time

I opted for the "Created" title to match the equivalent column from velero get backups. Same with "Age" to match the final column in standard kubectl get output (we already had that field/column for BSLs). Is consistency with those important?

+1 for being consistent with the columns for backups and using Created .

Agreed, I would prefer consistency.

I think I would prefer to match the current velero get backups output, since that's established and updating it may break user scripts, so Created works for me.

That does, however, put us in an awkward spot since I think all other .status.startTimestamp fields should match, due to the existence of the creationTimestamp field. Ideally, we'd have clean mappings between the field name in the YAML and printer column, but I think due to historical reasons we're a little stuck.

So my vote is this mapping:

Print .status.startTimestamp under Created
Print .metadata.creationTimestamp under Age

Sorry, I'm not sure I understand what you mean by:

That does, however, put us in an awkward spot since I think all other .status.startTimestamp fields should match, due to the existence of the creationTimestamp field.

As for the mapping you'd prefer, that is what is in this file. Am I missing something?

The awkward spot I was referring to is that we have creationTimestamp that isn't mapped to the Created display name. It's probably not a big deal except for people looking at the code.

It looks like there's 2 instances where status.startTimestamp is mapped to Start Time, rather than Created:

https://github.com/vmware-tanzu/velero/pull/2881/files#diff-2882940cdbbb4c1142f4b9468c593dc0R38

https://github.com/vmware-tanzu/velero/pull/2881/files#diff-315e9e294c9cc0ab062afc00990fa0aaR18

Ah, I see. Thanks for the explanation!

I used Started as those column names which matches the output from velero get restores... another inconsistency :(

Gotcha - yeah, that was our past mistake.

I think we could put that on the docket for v2.0 to update those velero commands so we don't break any scripts.

Created #2989 to address this.

pkg/apis/velero/v1/delete_backup_request.go

ashish-amarnath · 2020-09-10T19:33:32Z

pkg/apis/velero/v1/download_request.go

+// +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase",description="DownloadRequest status such as New/Processed"
+// +kubebuilder:printcolumn:name="Target Name",type="string",JSONPath=".spec.target.name",description="Name of the associated Kubernetes resource"
+// +kubebuilder:printcolumn:name="Target Kind",type="string",JSONPath=".spec.target.kind",description="Type of file to download"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"


suggest: Created At

ashish-amarnath · 2020-09-10T19:34:53Z

pkg/apis/velero/v1/pod_volume_backup.go

@@ -100,6 +100,14 @@ type PodVolumeBackupStatus struct {

 // +genclient
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+// +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase",description="Pod Volume Backup status such as New/InProgress"
+// +kubebuilder:printcolumn:name="Created",type="date",JSONPath=".status.startTimestamp",description="Time when this backup was started"


suggest: Start Time

It'd be helpful to know the reasoning for this request. Otherwise, Created is consistent with the column name for our other resources.

ashish-amarnath · 2020-09-10T19:35:33Z

pkg/apis/velero/v1/pod_volume_backup.go

+// +kubebuilder:printcolumn:name="Volume",type="string",JSONPath=".spec.volume",description="Name of the volume to be backed up"
+// +kubebuilder:printcolumn:name="Restic Repo",type="string",JSONPath=".spec.repoIdentifier",description="Restic repository identifier for this backup"
+// +kubebuilder:printcolumn:name="Storage Location",type="string",JSONPath=".spec.backupStorageLocation",description="Name of the Backup Storage Location where this backup should be stored"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"


Suggest: Created At

ashish-amarnath · 2020-09-10T19:36:03Z

pkg/apis/velero/v1/pod_volume_restore.go

+// +kubebuilder:printcolumn:name="Volume",type="string",JSONPath=".spec.volume",description="Name of volume to restore"
+// +kubebuilder:printcolumn:name="Bytes Done",type="string",JSONPath=".status.progress.bytesDone",description="Number of bytes restored"
+// +kubebuilder:printcolumn:name="Total Bytes",type="string",JSONPath=".status.progress.totalBytes",description="Total number of bytes to be restored"
+// +kubebuilder:printcolumn:name="Started",type="date",JSONPath=".status.startTimestamp",description="Time when the restore operation was started"


nit: Start Time
for consistency

I'll note that this doesn't match what we've got specified in the backup.go file right now; I think whichever we choose, the column name should be consistent across object types.

ashish-amarnath · 2020-09-10T19:36:18Z

pkg/apis/velero/v1/pod_volume_restore.go

+// +kubebuilder:printcolumn:name="Total Bytes",type="string",JSONPath=".status.progress.totalBytes",description="Total number of bytes to be restored"
+// +kubebuilder:printcolumn:name="Started",type="date",JSONPath=".status.startTimestamp",description="Time when the restore operation was started"
+// +kubebuilder:printcolumn:name="Completed",type="date",JSONPath=".status.completionTimestamp",description="Time when the restore operation was completed"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"


suggest: Created At

ashish-amarnath · 2020-09-10T19:36:55Z

pkg/apis/velero/v1/restore.go

@@ -251,6 +251,13 @@ type RestoreStatus struct {

 // +genclient
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+// +kubebuilder:printcolumn:name="Backup",type="string",JSONPath=".spec.backupName",description="Name of backup to restore from"
+// +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase",description="Restore status such as InProgress/Completed"
+// +kubebuilder:printcolumn:name="Started",type="date",JSONPath=".status.startTimestamp",description="Time when the restore operation was started"


nit: Start Time
for consistency

ashish-amarnath · 2020-09-10T19:37:07Z

pkg/apis/velero/v1/restore.go

+// +kubebuilder:printcolumn:name="Completed",type="date",JSONPath=".status.completionTimestamp",description="Time when the restore operation was completed"
+// +kubebuilder:printcolumn:name="Errors",type="integer",JSONPath=".status.errors",description="Count of all errors generated during the restore"
+// +kubebuilder:printcolumn:name="Warnings",type="integer",JSONPath=".status.warnings",description="Count of all warnings generated during the restore"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"


suggest: Created At

ashish-amarnath · 2020-09-10T19:37:28Z

pkg/apis/velero/v1/volume_snapshot_location.go

@@ -20,6 +20,8 @@ import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

 // +genclient
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+// +kubebuilder:printcolumn:name="Provider",type="string",JSONPath=".spec.provider"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"


suggest: Created At

carlisia · 2020-09-14T23:23:29Z

@zubron when you have a chance please resolve the conflicts, thanks.

This change modifies the kubebuilder annotations for the Velero CRDs to include `additionalPrinterColumns` so that more information is exposed when using `kubectl get`. For each of the CRDs, annotations have been added to make the output for `kubectl get` match the output from the equivalent `velero get` command as closely as possible. There are some cases where this output could not be replicated, such as the `EXPIRES` column for Backups, due to the limitations of JSONPath expressions within the resulting CRD defition. Some columns undergo processing and formatting before being printed by the Velero CLI which cannot be replicated using JSONPath. In these cases, these printer columns have been omitted. For other CRDs where there is no `velero get` equivalent, such as `PodVolumeBackup` and `PodVolumeRestore`, a best effort has been made to expose information that provides value. Signed-off-by: Bridget McErlean <[email protected]>

nrb

I know we had a discussion about calculated fields in the community meeting, and we can't do them without extra code.

With that in mind, I think renaming the current Age fields would be desirable, because kubectl uses a calculated value as opposed to a bare timestamp for this when querying for built-in resources.

For example, here's what it looks like when querying for nodes:

% kubectl get nodes
NAME                 STATUS   ROLES    AGE   VERSION
kind-control-plane   Ready    master   19h   v1.17.0

In this case, the Age column is actually the difference between now and the creation timestamp. If we use the creationTimestamp directly, I worry about overloading the field's name with a different type of data.

carlisia

👍

My change request was based on inaccurate information.

nrb · 2020-10-13T23:11:51Z

I think I'm going to lean towards keeping the fields internally consistent, and letting this PR merge as is. We now have an action item to clean things up in v2.0, so I think that will work.

nrb · 2020-10-26T18:27:35Z

@ashish-amarnath Could you review the comments and update your review?

This change modifies the kubebuilder annotations for the Velero CRDs to include `additionalPrinterColumns` so that more information is exposed when using `kubectl get`. For each of the CRDs, annotations have been added to make the output for `kubectl get` match the output from the equivalent `velero get` command as closely as possible. There are some cases where this output could not be replicated, such as the `EXPIRES` column for Backups, due to the limitations of JSONPath expressions within the resulting CRD defition. Some columns undergo processing and formatting before being printed by the Velero CLI which cannot be replicated using JSONPath. In these cases, these printer columns have been omitted. For other CRDs where there is no `velero get` equivalent, such as `PodVolumeBackup` and `PodVolumeRestore`, a best effort has been made to expose information that provides value. Signed-off-by: Bridget McErlean <[email protected]>

This reverts commit 4178d9d.

This reverts commit 4178d9d. Signed-off-by: Carlisia <[email protected]>

* Revert "Add additional printer columns for CRDs (#2881)" This reverts commit 4178d9d. Signed-off-by: Carlisia <[email protected]> * Add generated files Signed-off-by: Carlisia <[email protected]>

* Revert "Add additional printer columns for CRDs (vmware-tanzu#2881)" This reverts commit 4178d9d. Signed-off-by: Carlisia <[email protected]> * Add generated files Signed-off-by: Carlisia <[email protected]> Signed-off-by: Pradeep Jigalur <[email protected]>

This change modifies the kubebuilder annotations for the Velero CRDs to include `additionalPrinterColumns` so that more information is exposed when using `kubectl get`. For each of the CRDs, annotations have been added to make the output for `kubectl get` match the output from the equivalent `velero get` command as closely as possible. There are some cases where this output could not be replicated, such as the `EXPIRES` column for Backups, due to the limitations of JSONPath expressions within the resulting CRD defition. Some columns undergo processing and formatting before being printed by the Velero CLI which cannot be replicated using JSONPath. In these cases, these printer columns have been omitted. For other CRDs where there is no `velero get` equivalent, such as `PodVolumeBackup` and `PodVolumeRestore`, a best effort has been made to expose information that provides value. Signed-off-by: Bridget McErlean <[email protected]>

* Revert "Add additional printer columns for CRDs (vmware-tanzu#2881)" This reverts commit 4178d9d. Signed-off-by: Carlisia <[email protected]> * Add generated files Signed-off-by: Carlisia <[email protected]>

This change modifies the kubebuilder annotations for the Velero CRDs to include `additionalPrinterColumns` so that more information is exposed when using `kubectl get`. For each of the CRDs, annotations have been added to make the output for `kubectl get` match the output from the equivalent `velero get` command as closely as possible. There are some cases where this output could not be replicated, such as the `EXPIRES` column for Backups, due to the limitations of JSONPath expressions within the resulting CRD defition. Some columns undergo processing and formatting before being printed by the Velero CLI which cannot be replicated using JSONPath. In these cases, these printer columns have been omitted. For other CRDs where there is no `velero get` equivalent, such as `PodVolumeBackup` and `PodVolumeRestore`, a best effort has been made to expose information that provides value. Signed-off-by: Bridget McErlean <[email protected]>

* Revert "Add additional printer columns for CRDs (vmware-tanzu#2881)" This reverts commit 4178d9d. Signed-off-by: Carlisia <[email protected]> * Add generated files Signed-off-by: Carlisia <[email protected]>

This change modifies the kubebuilder annotations for the Velero CRDs to include `additionalPrinterColumns` so that more information is exposed when using `kubectl get`. For each of the CRDs, annotations have been added to make the output for `kubectl get` match the output from the equivalent `velero get` command as closely as possible. There are some cases where this output could not be replicated, such as the `EXPIRES` column for Backups, due to the limitations of JSONPath expressions within the resulting CRD defition. Some columns undergo processing and formatting before being printed by the Velero CLI which cannot be replicated using JSONPath. In these cases, these printer columns have been omitted. For other CRDs where there is no `velero get` equivalent, such as `PodVolumeBackup` and `PodVolumeRestore`, a best effort has been made to expose information that provides value. Signed-off-by: Bridget McErlean <[email protected]>

* Revert "Add additional printer columns for CRDs (vmware-tanzu#2881)" This reverts commit 4178d9d. Signed-off-by: Carlisia <[email protected]> * Add generated files Signed-off-by: Carlisia <[email protected]>

github-actions bot requested review from ashish-amarnath, carlisia and nrb August 31, 2020 21:43

github-actions bot assigned zubron Aug 31, 2020

zubron force-pushed the define-printer-columns-for-velero-crds-1780 branch from 473eec3 to a2028e9 Compare August 31, 2020 21:57

nrb added this to the v1.5.1 milestone Sep 1, 2020

ashish-amarnath suggested changes Sep 10, 2020

View reviewed changes

carlisia requested a review from ashish-amarnath September 14, 2020 23:23

zubron force-pushed the define-printer-columns-for-velero-crds-1780 branch from a2028e9 to 97037ff Compare September 15, 2020 14:11

carlisia modified the milestones: v1.5.1, v1.5.2 Sep 17, 2020

nrb previously requested changes Sep 17, 2020

View reviewed changes

carlisia requested a review from nrb September 17, 2020 18:51

carlisia approved these changes Sep 17, 2020

View reviewed changes

zubron mentioned this pull request Oct 6, 2020

Make printer column titles consistent across all types #2989

Open

nrb approved these changes Oct 26, 2020

View reviewed changes

ashish-amarnath approved these changes Oct 27, 2020

View reviewed changes

ashish-amarnath merged commit 4178d9d into vmware-tanzu:main Oct 27, 2020

georgettica mentioned this pull request Dec 23, 2020

georgettica/bump external snapshotter version #3201

Closed

carlisia pushed a commit to carlisia/velero that referenced this pull request Mar 31, 2021

Revert "Add additional printer columns for CRDs (vmware-tanzu#2881)"

fce1ab8

This reverts commit 4178d9d.

carlisia pushed a commit to carlisia/velero that referenced this pull request Mar 31, 2021

Revert "Add additional printer columns for CRDs (vmware-tanzu#2881)"

4358b18

This reverts commit 4178d9d. Signed-off-by: Carlisia <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional printer columns for CRDs #2881

Add additional printer columns for CRDs #2881

zubron commented Aug 31, 2020 •

edited

Loading

nrb commented Sep 1, 2020

ashish-amarnath left a comment

ashish-amarnath Sep 10, 2020

zubron Sep 11, 2020

carlisia Sep 14, 2020

nrb Sep 17, 2020

nrb Sep 28, 2020

zubron Oct 6, 2020

nrb Oct 6, 2020

zubron Oct 6, 2020

nrb Oct 6, 2020

zubron Oct 6, 2020

ashish-amarnath Sep 10, 2020

carlisia Sep 14, 2020

ashish-amarnath Sep 10, 2020

carlisia Sep 14, 2020

ashish-amarnath Sep 10, 2020

ashish-amarnath Sep 10, 2020

nrb Sep 17, 2020

ashish-amarnath Sep 10, 2020

ashish-amarnath Sep 10, 2020

ashish-amarnath Sep 10, 2020

ashish-amarnath Sep 10, 2020

carlisia commented Sep 14, 2020

nrb left a comment

carlisia left a comment

nrb commented Oct 13, 2020

nrb commented Oct 26, 2020

Add additional printer columns for CRDs #2881

Add additional printer columns for CRDs #2881

Conversation

zubron commented Aug 31, 2020 • edited Loading

nrb commented Sep 1, 2020

ashish-amarnath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlisia commented Sep 14, 2020

nrb left a comment

Choose a reason for hiding this comment

carlisia left a comment

Choose a reason for hiding this comment

nrb commented Oct 13, 2020

nrb commented Oct 26, 2020

zubron commented Aug 31, 2020 •

edited

Loading