Provide better defaults for external-snapshotter #459

kvaps · 2020-12-24T01:00:52Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Provided manifests for external-snapshotter are not optimal in most use-cases.

Since external-snapshotter is most common component among various Kubernetes distributions and csi-plugins, I think we need to correct the manifests and include the following changes. They are needed to simplify the setup and provide optimal defaults for every user.

Deploy external-snapshotter into kube-system namespace by default
Enable leader-election
Use Deployment instead of Statefulset and deploy minimum 2 replicas for redundancy

The following changes would allow to simple install working external-snapshotter using just few commands, as described here:
https://kubernetes-csi.github.io/docs/snapshot-controller.html#deployment

but without having to manually edit manifests

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

external-snapshotter manifests adjusted to reflect more common example

k8s-ci-robot · 2020-12-24T01:01:00Z

Welcome @kvaps!

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-12-24T01:01:00Z

Hi @kvaps. 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.

kvaps · 2020-12-24T01:15:41Z

/assign @jingxu97

xing-yang · 2020-12-24T02:37:12Z

/ok-to-test

kvaps · 2020-12-24T13:39:56Z

/retest

kvaps · 2020-12-24T19:27:02Z

/retest

xing-yang · 2021-01-04T18:24:40Z

deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml

@@ -9,13 +9,12 @@ apiVersion: v1
 kind: ServiceAccount
 metadata:
  name: snapshot-controller
-  namespace: default # TODO: replace with the namespace you want for your controller, e.g. kube-system
+  namespace: kube-system


This is used in CI so I'd like to ask Patrick to take a look.
@pohly Do you have any concerns for this namespace to be changed from "default" to "kube-system"?

No objections.

Just beware that it might be necessary to change prow.sh so that it works with a rbac-snapshot-controller.yaml that has namespace: default (current stable release) and a newer rbac-snapshot-controller.yaml that has namespace: kube-system (canary) because the same prow.sh file is used with both, depending on the test job.

xing-yang · 2021-01-04T18:25:02Z

release-tools/prow.sh

    if [ $cnt -gt 30 ]; then
        echo "snapshot-controller pod status:"
-        kubectl describe pods -l app=snapshot-controller
+        kubectl describe pods -n "$expected_namespace" -l app=snapshot-controller


prow.sh should be changed in csi-release-tools first.

Sure, It has been changed already, PR kubernetes-csi/csi-release-tools#121

xing-yang · 2021-01-26T20:41:18Z

The release-tools change is already included in this PR: #464. Can you remove those changes from your PR and rebase?

kvaps · 2021-01-27T15:49:01Z

Sure, rebased

xing-yang · 2021-01-29T04:20:06Z

/lgtm
/approve

k8s-ci-robot · 2021-01-29T04:20:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kvaps, xing-yang

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xing-yang]

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

kvaps · 2021-02-15T13:34:39Z

Hi. Is it possible to create release with this change?

xing-yang · 2021-02-17T15:45:48Z

This affects multiple sidecars because we have to make changes in release-tools. I don't think this is a bug fix that can be backported. The yaml files are provided as examples. User can modify their own yamls if needed. Our usual release timeline aligns with the next k8s release.

kvaps · 2021-02-17T22:56:55Z

Got it! Thank you

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 24, 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 Dec 24, 2020

k8s-ci-robot requested review from xing-yang and yuxiangqian December 24, 2020 01:01

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 24, 2020

kvaps force-pushed the common-example branch 3 times, most recently from ed88539 to 2db4645 Compare December 24, 2020 01:07

k8s-ci-robot assigned jingxu97 Dec 24, 2020

kvaps force-pushed the common-example branch 3 times, most recently from e33a43f to cc157ce Compare December 24, 2020 17:31

kvaps mentioned this pull request Dec 24, 2020

Check namespace for snapshot-controller kubernetes-csi/csi-release-tools#121

Merged

kvaps force-pushed the common-example branch from cc157ce to 2db4645 Compare December 24, 2020 18:05

kvaps force-pushed the common-example branch from c14e1af to 7ac6f0c Compare December 24, 2020 18:37

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 24, 2020

xing-yang reviewed Jan 4, 2021

View reviewed changes

kvaps force-pushed the common-example branch from 7ac6f0c to cf22542 Compare January 19, 2021 00:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 19, 2021

kvaps force-pushed the common-example branch 3 times, most recently from f6a1b62 to 778e3c0 Compare January 25, 2021 23:15

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 Jan 25, 2021

kvaps force-pushed the common-example branch from 778e3c0 to 12f6e13 Compare January 27, 2021 15:42

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 27, 2021

upd: more common manifests

d138216

kvaps force-pushed the common-example branch from 12f6e13 to d138216 Compare January 27, 2021 15:48

k8s-ci-robot assigned xing-yang Jan 29, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2021

k8s-ci-robot merged commit 165f6e0 into kubernetes-csi:master Jan 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide better defaults for external-snapshotter #459

Provide better defaults for external-snapshotter #459

kvaps commented Dec 24, 2020 •

edited

Loading

k8s-ci-robot commented Dec 24, 2020

k8s-ci-robot commented Dec 24, 2020

kvaps commented Dec 24, 2020

xing-yang commented Dec 24, 2020

kvaps commented Dec 24, 2020

kvaps commented Dec 24, 2020

xing-yang Jan 4, 2021

pohly Jan 4, 2021

xing-yang Jan 4, 2021

kvaps Jan 4, 2021

xing-yang commented Jan 26, 2021 •

edited

Loading

kvaps commented Jan 27, 2021

xing-yang commented Jan 29, 2021

k8s-ci-robot commented Jan 29, 2021

kvaps commented Feb 15, 2021

xing-yang commented Feb 17, 2021

kvaps commented Feb 17, 2021

Provide better defaults for external-snapshotter #459

Provide better defaults for external-snapshotter #459

Conversation

kvaps commented Dec 24, 2020 • edited Loading

k8s-ci-robot commented Dec 24, 2020

k8s-ci-robot commented Dec 24, 2020

kvaps commented Dec 24, 2020

xing-yang commented Dec 24, 2020

kvaps commented Dec 24, 2020

kvaps commented Dec 24, 2020

xing-yang Jan 4, 2021

Choose a reason for hiding this comment

pohly Jan 4, 2021

Choose a reason for hiding this comment

xing-yang Jan 4, 2021

Choose a reason for hiding this comment

kvaps Jan 4, 2021

Choose a reason for hiding this comment

xing-yang commented Jan 26, 2021 • edited Loading

kvaps commented Jan 27, 2021

xing-yang commented Jan 29, 2021

k8s-ci-robot commented Jan 29, 2021

kvaps commented Feb 15, 2021

xing-yang commented Feb 17, 2021

kvaps commented Feb 17, 2021

kvaps commented Dec 24, 2020 •

edited

Loading

xing-yang commented Jan 26, 2021 •

edited

Loading