Update CRDs to include additional get information #256

huffmanca · 2020-02-14T21:04:15Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
Includes additional information for the get command for the VolumeSnapshot* CRDs.

Which issue(s) this PR fixes:
Fixes #247

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Prints additional details when using kubectl get on VolumeSnapshot, VolumeSnapshotContent, and VolumeSnapshotClass API objects.

k8s-ci-robot · 2020-02-14T21:04:23Z

Welcome @huffmanca!

It looks like this is your first PR to kubernetes-csi/external-snapshotter 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-snapshotter has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-02-14T21:04:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: huffmanca
To complete the pull request process, please assign jingxu97
You can assign the PR to them by writing /assign @jingxu97 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2020-02-14T21:04:23Z

Hi @huffmanca. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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/test-infra repository.

xing-yang · 2020-02-14T21:11:15Z

Thanks for the PR!

Did you add anything to types.go and generate this automatically? Please update README to describe how to generate CRDs with this additional info.

https://github.com/kubernetes-csi/external-snapshotter/blob/master/hack/README.md#update-crdsh

xing-yang · 2020-02-14T21:12:18Z

/ok-to-test

huffmanca · 2020-02-14T22:00:13Z

@xing-yang ,

Apologies. types.go has now been updated, and the CRDs were created following the instructions in https://github.com/kubernetes-csi/external-snapshotter/blob/master/hack/README.md#update-crdsh . Some notes:

I included the api-approved.kubernetes.io annotation, and pointed it to this PR.
The controller-gen.kubebuilder.io/version annotation was automatically updated to v0.2.4. I've left this as is for now.

pkg/apis/volumesnapshot/v1beta1/types.go

jsafrane · 2020-02-17T15:19:49Z

For us, mere mortals who don't read CRD YAML files nor kubebuilder tags, can you please add example output of kubectl get *?

huffmanca · 2020-02-17T21:38:36Z

Sample output:

$ oc get volumesnapshotclass
NAME                DRIVER                DELETIONPOLICY
csi-hostpath-snap   hostpath.csi.k8s.io   Delete

$ oc get volumesnapshot
NAME     READYTOUSE   PVCCLAIM   SNAPSHOTCLAIM   RESTORESIZE   SNAPSHOTCLASS       SNAPSHOTCONTENT                                    AGE
mysnap   true         myclaim                    1Gi           csi-hostpath-snap   snapcontent-2fc4c89d-c23d-411d-8e40-410f0681a065   2m57s

$ oc get volumesnapshotcontent
NAME                                               READYTOUSE   RESTORESIZE   DELETIONPOLICY   DRIVER                VOLUMESNAPSHOTCLASS   VOLUMESNAPSHOT   AGE
snapcontent-2fc4c89d-c23d-411d-8e40-410f0681a065   true         1073741824    Delete           hostpath.csi.k8s.io   csi-hostpath-snap     mysnap           4s

jsafrane · 2020-02-18T08:45:52Z

config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml

+    name: PVCClaim
+    type: string
+  - JSONPath: .spec.source.volumeSnapshotContentName
+    description: The VolumeSnapshotContent from where a snapshot will be created.
+    name: SnapshotClaim


PVCClaim / SnapshotClaim sounds strange. What about SourcePVC and SourceSnapshotContent?

In addition, do we need volumeSnapshotContentName in kubectl get? IMO, the number of pre-provisioned snapshots is going to be much smaller than number of dynamically provisioned ones. We can leave PVCClaim (SourcePVC) empty, indicating that this one is special, user will see status.boundVolumeSnapshotContentName anyway and if not, they can do kubectl describe to get the details.

SourcePVC and SourceSnapshotContent sounds good.

Pre-provisioned snapshots is often used in backup and restore use cases so it is important to show volumeSnapshotContentName in the source, which has a different meaning from status.boundVolumeSnapshotContentName.

If there's a way to dynamically display only one of SourcePVC and SourceSnapshotContent depending on which one is not nil, that will be great. I don't know if it is possible to do that using printerColumns.

It's probably more important to show the status content name since we expect them to be the same in valid cases

The source field will never be nil. Can we have a "VolumeSnapshotSource" field which is either PVC or content name and have a "SourceType" field that indicates whether it is PVC name or content name?

I've updated the column names to be SourcePVC and SouceSnapshotContent to be clearer.

If there's a way to dynamically display only one of SourcePVC and SourceSnapshotContent depending on which one is not nil, that will be great. I don't know if it is possible to do that using printerColumns.

Looking at https://book.kubebuilder.io/reference/markers/crd.html I don't see way to include conditionals when using printcolumn.

The source field will never be nil. Can we have a "VolumeSnapshotSource" field which is either PVC or content name and have a "SourceType" field that indicates whether it is PVC name or content name?

I'm assuming this would require a code change? I don't think this is possible currently from modifying types.go.

It's probably more important to show the status content name since we expect them to be the same in valid cases

Don't we do this presently in the PR?

// +kubebuilder:printcolumn:name="SnapshotContent",type=string,JSONPath=`.status.boundVolumeSnapshotContentName`,description="The name of the VolumeSnapshotContent to which this VolumeSnapshot is bound."

pkg/apis/volumesnapshot/v1beta1/types.go

xing-yang · 2020-02-18T21:00:10Z

Since types.go is modified, should https://github.com/kubernetes-csi/external-snapshotter/blob/master/hack/update-generated-code.sh be called again to re-generate code? Note that this is getting complicated after we added v2 to the package path. See details here: #239 (comment)

k8s-ci-robot · 2020-02-20T20:39:19Z

The following users are mentioned in OWNERS file(s) but are not members of the kubernetes-csi org.

Once all users have been added as members of the org, you can trigger verification by writing /verify-owners in a comment.

huffmanca
- OWNERS_ALIASES
tsmetana
- OWNERS_ALIASES

xing-yang · 2020-02-20T20:47:41Z

Getting a "invalid-owners-file" label.

huffmanca · 2020-02-20T20:52:39Z

The invalid owners should be corrected. It was an issue when attempting to rebase.

I attempted to run the instructions in the #239 (comment) , but it doesn't generate anything under pkg/client. I've rebased off of the master branch and ran the instructions, but nothing is created.

I'm working through these to see if I can generate this in a separate commit.

huffmanca · 2020-02-20T21:40:23Z

@xing-yang ,

I've reset the head of this branch to master and attempted following the instructions in the link, but nothing is generated. The commands complete without error, but there's nothing new created under pkg/client.

Do these steps need to be performed for the master branch as well as the 2.x ones?

xing-yang · 2020-02-20T21:46:18Z

@huffmanca you should only need to run it on master. Maybe it's because you didn't change the API itself, so there's no changes in the generated files. I'll give a try myself to make sure.

xing-yang · 2020-02-20T21:54:54Z

The "invalid-owners-file" issue needs to be resolved though, otherwise this can't be merged. Maybe you need a rebase.

huffmanca · 2020-02-20T22:15:37Z

/verify-owners

xing-yang · 2020-02-21T00:32:21Z

I tried and all files under pkg/client are re-generated, however, the only change is the year.

~/go/src/github.com/kubernetes-csi/external-snapshotter# git diff pkg/client/listers/volumesnapshot/v1beta1/volumesnapshot.go
diff --git a/pkg/client/listers/volumesnapshot/v1beta1/volumesnapshot.go b/pkg/client/listers/volumesnapshot/v1beta1/volumesnapshot.go
index ed445782..48013ce0 100644
--- a/pkg/client/listers/volumesnapshot/v1beta1/volumesnapshot.go
+++ b/pkg/client/listers/volumesnapshot/v1beta1/volumesnapshot.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Kubernetes Authors.
+Copyright 2020 The Kubernetes Authors.

 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

So I think we don't need to do this code generation in this PR. We can do it later when there are more changes.

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml

config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml

huffmanca · 2020-02-21T14:58:05Z

@xing-yang

I think this is ready to go. In regards to the invalid owners - the owners file should be correct (it was modified by the rebase, but has since been reset to the original file), and it seems the /verify-owners command did not trigger verification.

Any advice on how to correct this label?

xing-yang · 2020-02-21T15:17:26Z

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

@@ -4,19 +4,32 @@ apiVersion: apiextensions.k8s.io/v1beta1
 kind: CustomResourceDefinition
 metadata:
  annotations:
-    controller-gen.kubebuilder.io/version: (devel)
-    api-approved.kubernetes.io: "https://github.com/kubernetes-csi/external-snapshotter/pull/139"
+    controller-gen.kubebuilder.io/version: v0.2.4


I see that the latest release is v0.2.5, which was released 23 days ago. Is there any reason not to use the latest release?

There's not - This was the automatically generated version. I've updated this to be 0.2.5 in all the CRDs.

Did you manually update them to 0.2.5? I think it is the version of controller-gen you installed on your system. Can you update controller-gen on your system to be 0.2.5 and have this generated automatically?

Can you answer my question?

I did manually update it - to my knowledge, I haven't built controller-gen on my system. I can see it in my gopath modules, though, and am attempting to update it before re-creating the CRDs.

Do you have kubebuilder installed?

I haven't installed or used it before.

What tool are you using to generate the CRDs?

I'd been using hack/update-crd. Checking this now I see:

go get sigs.k8s.io/controller-tools/cmd/[email protected];

Looks the repo defaults to v0.2.4 if controller-gen isn't detected. Let me update this and push again this evening.

That makes sense. Thanks.

xing-yang · 2020-02-21T15:19:54Z

I have never seen this label "do-not-merge/invalid-owners-file" before. Not sure how to fix it. @msau42 @jsafrane any ideas?

xing-yang · 2020-02-21T15:24:52Z

Can you change the release note to the following:
Prints additional details when using kubectl get on VolumeSnapshot, VolumeSnapshotContent, and VolumeSnapshotClass API objects.

huffmanca · 2020-02-21T19:08:52Z

I've updated the release notes text. Now we just need to clear the invalid-owners label.

xing-yang · 2020-02-24T22:06:34Z

To get the owners file issue resolved, maybe you need to submit a new PR with a new repo? I don't know if there's a better way.

k8s-ci-robot · 2020-02-25T04:46:56Z

@huffmanca: PR needs rebase.

Instructions 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/test-infra repository.

k8s-ci-robot · 2020-02-25T04:48:12Z

@huffmanca: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-external-snapshotter-1-15-on-kubernetes-1-15	`e1cb0d2`	link	`/test pull-kubernetes-csi-external-snapshotter-1-15-on-kubernetes-1-15`
pull-kubernetes-csi-external-snapshotter-1-16-on-kubernetes-1-16	`e1cb0d2`	link	`/test pull-kubernetes-csi-external-snapshotter-1-16-on-kubernetes-1-16`
pull-kubernetes-csi-external-snapshotter-1-17-on-kubernetes-1-17	`e1cb0d2`	link	`/test pull-kubernetes-csi-external-snapshotter-1-17-on-kubernetes-1-17`
pull-kubernetes-csi-external-snapshotter-unit	`e1cb0d2`	link	`/test pull-kubernetes-csi-external-snapshotter-unit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions 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/test-infra repository. I understand the commands that are listed here.

huffmanca · 2020-02-25T14:51:04Z

@xing-yang ,

I'm going to close this PR out in favor of #260 . PR 260 should include the rebase, not adjust the owners file, and have the hack/update-crd.sh script updated to use version 0.2.5.

Instruction update

f40f0cc Merge pull request kubernetes-csi#256 from solumath/master cfa9210 Instruction update 379a1bb Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bb fix typo in sidecar release process 4967685 Merge pull request kubernetes-csi#254 from bells17/add-github-actions d9bd160 Update skip list in codespell GitHub Action f5aebfc Add GitHub Actions workflows git-subtree-dir: release-tools git-subtree-split: f40f0cc

988496a1f Merge pull request kubernetes-csi#257 from jakobmoellerdev/csi-prow-sidecar-e2e-path 028f8c698 chore: bump to Go 1.22.5 69bd71e8a chore: add CSI_PROW_SIDECAR_E2E_PATH f40f0cc Merge pull request kubernetes-csi#256 from solumath/master cfa9210 Instruction update 379a1bb Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bb fix typo in sidecar release process 4967685 Merge pull request kubernetes-csi#254 from bells17/add-github-actions d9bd160 Update skip list in codespell GitHub Action f5aebfc Add GitHub Actions workflows git-subtree-dir: release-tools git-subtree-split: 988496a1fc3849ed793e03012fdd56813d13d46c

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 14, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 14, 2020

k8s-ci-robot requested review from lpabon and xing-yang February 14, 2020 21:04

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 14, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 14, 2020

huffmanca force-pushed the update_getters branch from b76b4ef to eb923ed Compare February 14, 2020 21:58

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 14, 2020

xing-yang reviewed Feb 15, 2020

View reviewed changes

huffmanca force-pushed the update_getters branch from eb923ed to dd2e943 Compare February 17, 2020 21:38

jsafrane reviewed Feb 18, 2020

View reviewed changes

huffmanca force-pushed the update_getters branch from dd2e943 to c3de1b1 Compare February 18, 2020 20:42

xing-yang reviewed Feb 18, 2020

View reviewed changes

pkg/apis/volumesnapshot/v1beta1/types.go Outdated Show resolved Hide resolved

xing-yang mentioned this pull request Feb 19, 2020

Invalid api-conventions URL link #258

Closed

huffmanca force-pushed the update_getters branch from c3de1b1 to 96b7270 Compare February 20, 2020 20:38

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 20, 2020

huffmanca force-pushed the update_getters branch from 96b7270 to c3de1b1 Compare February 20, 2020 20:40

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 20, 2020

huffmanca force-pushed the update_getters branch from c3de1b1 to c4b7ebb Compare February 20, 2020 20:43

xing-yang reviewed Feb 21, 2020

View reviewed changes

huffmanca force-pushed the update_getters branch from c4b7ebb to ddd22a1 Compare February 21, 2020 14:55

xing-yang reviewed Feb 21, 2020

View reviewed changes

huffmanca force-pushed the update_getters branch from ddd22a1 to b3e3687 Compare February 21, 2020 19:03

Update CRDs to include additional get information

e1cb0d2

huffmanca force-pushed the update_getters branch from b3e3687 to e1cb0d2 Compare February 25, 2020 04:46

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2020

huffmanca closed this Feb 25, 2020

andyzhangx pushed a commit to andyzhangx/external-snapshotter that referenced this pull request Jun 5, 2024

Merge pull request kubernetes-csi#256 from solumath/master

f40f0cc

Instruction update

Update CRDs to include additional get information #256

Update CRDs to include additional get information #256

Conversation

huffmanca commented Feb 14, 2020 • edited Loading

k8s-ci-robot commented Feb 14, 2020

k8s-ci-robot commented Feb 14, 2020

k8s-ci-robot commented Feb 14, 2020

xing-yang commented Feb 14, 2020

xing-yang commented Feb 14, 2020

huffmanca commented Feb 14, 2020

jsafrane commented Feb 17, 2020

huffmanca commented Feb 17, 2020

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

huffmanca Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

xing-yang commented Feb 18, 2020

k8s-ci-robot commented Feb 20, 2020

xing-yang commented Feb 20, 2020

huffmanca commented Feb 20, 2020 • edited Loading

huffmanca commented Feb 20, 2020

xing-yang commented Feb 20, 2020

xing-yang commented Feb 20, 2020

huffmanca commented Feb 20, 2020

xing-yang commented Feb 21, 2020

huffmanca commented Feb 21, 2020

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

huffmanca Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented Feb 21, 2020

xing-yang commented Feb 21, 2020

huffmanca commented Feb 21, 2020

xing-yang commented Feb 24, 2020

k8s-ci-robot commented Feb 25, 2020

k8s-ci-robot commented Feb 25, 2020

huffmanca commented Feb 25, 2020

huffmanca commented Feb 14, 2020 •

edited

Loading

huffmanca Feb 18, 2020 •

edited

Loading

huffmanca commented Feb 20, 2020 •

edited

Loading

huffmanca Feb 24, 2020 •

edited

Loading