Automated cherry pick of #504: Make sure the VolumeSnapshots v1 CRDs exist before starting #505: Update sample implementation #507: fix typo in CRD check

[mauriciopoppe]

Cherry pick of #504 #505 #507 on release-4.0.

#504: Make sure the VolumeSnapshots v1 CRDs exist before starting
#505: Update sample implementation
#507: fix typo in CRD check

For details on the cherry pick process, see the cherry pick requests page.

Does this PR introduce a user-facing change?:

Add check for v1 CRDs to allow for rolling update of the snapshot-controller

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mauriciopoppe
To complete the pull request process, please assign yuxiangqian after the PR has been reviewed.
You can assign the PR to them by writing /assign @yuxiangqian 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]

Hi @mauriciopoppe. 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]

/ok-to-test

[k8s-ci-robot]

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

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-external-snapshotter-1-19-on-kubernetes-1-19	a2af37a	link	/test pull-kubernetes-csi-external-snapshotter-1-19-on-kubernetes-1-19
pull-kubernetes-csi-external-snapshotter-1-18-on-kubernetes-1-18	a2af37a	link	/test pull-kubernetes-csi-external-snapshotter-1-18-on-kubernetes-1-18
pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20	a2af37a	link	/test pull-kubernetes-csi-external-snapshotter-1-20-on-kubernetes-1-20

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.

[xing-yang]

I see the following errors. This is caused by the changes in the snapshot controller deployment file. Probably the new changes in StatefulSet API is not recognizable by an earlier version of K8S cluster.

Deploying snapshot-controller
   /home/prow/go/src/github.com/kubernetes-csi/external-snapshotter/deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml
        using           image: snapshot-controller:csiprow
error: error validating "STDIN": error validating data: [ValidationError(StatefulSet.spec): unknown field "minReadySeconds" in io.k8s.api.apps.v1.StatefulSetSpec, ValidationError(StatefulSet.spec): unknown field "strategy" in io.k8s.api.apps.v1.StatefulSetSpec]; if you choose to ignore these errors, turn validation off with --validate=false

[mauriciopoppe]

I think this is failing because #459 wasn't cherrypicked to the release branch, I read this comment too #459 (comment), what do you suggest?

[xing-yang]

I think this is failing because #459 wasn't cherrypicked to the release branch, I read this comment too #459 (comment), what do you suggest?

I don't think that is the problem. That PR only deploys snapshot-controller in "kube-system" namespace. The error we are getting is not "namespace not found". It is complaining about specific fields in StatefulSet Spec not existing. So there's a mismatch somewhere. It seems that the changes you added to "setup-snapshot-controller.yaml " is not compatible with what the StatefulSet Spec is expecting on this cluster.

[xing-yang]

I know what the problem is now. In release-4.0, we are using "statefulset" to deploy snapshot-controller. In master, we are using "deployment". So the new fields you added to "setup-snapshot-controller.yaml " are not valid fields on release-4.0 branch.

I'm not sure that we can backport a change that needs to change from "statefulset" to "deployment". I think for this type of change, we need to bump the minor version, i.e., from 4.0.0 to 4.1.0. We are about to release 4.1.0. So maybe we don't need to backport to release-4.0.

[mauriciopoppe]

if we can't backport #459 then I can probably just cherrypick the changes on the startup code and not the manifests, what do you think?

Also the idea of releasing 4.1.0 would be great!

cc @msau42

[xing-yang]

Hi @mauriciopoppe, v4.1.0 is just released.

Looks like StatefulSet also has an UpdateStrategy field. Do you want to take a look and see if similar changes can be applied?
https://github.com/kubernetes/kubernetes/blob/v1.22.0-alpha.1/pkg/apis/apps/types.go#L153

[mauriciopoppe]

Thanks for the suggestion @xing-yang, I've looked at this field and it has this shape:

updateStrategy:
  type: "RollingUpdate" | "OnDelete"
  rollingUpdate:
    partition: int32

Unfortunately for the fix in #504 to work we rely on minReadySeconds, however because there's a new release I'm going to use that instead.