Skip to content

Add snapshot deployment support #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 17, 2018
Merged

Add snapshot deployment support #120

merged 1 commit into from
Oct 17, 2018

Conversation

jingxu97
Copy link
Contributor

Add the snapshotter deployment support and example

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 28, 2018
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 28, 2018
@davidz627
Copy link
Contributor

@jingxu97 please rebase and fix conflicts

@jingxu97
Copy link
Contributor Author

jingxu97 commented Oct 3, 2018

/test pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration

@davidz627
Copy link
Contributor

@jingxu97 sorry integration test is broken right now, I am working on fixing

@davidz627
Copy link
Contributor

@jingxu97 could you also add snapshot user guide and link to it in the main README?

@davidz627
Copy link
Contributor

Hi @jingxu97 just waiting on this to merge now for Beta,
Could you also remove https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/120/files#diff-46e2ab87e8b542bb21f57e7268513d41R40 these changes, currently they are constants used for integration tests. I will fix it later.

@k8s-ci-robot 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 Oct 5, 2018
davidz627
davidz627 reviewed Oct 5, 2018
View reviewed changes
deploy/kubernetes/dev/controller.yaml Outdated
- name: gce-pd-driver
imagePullPolicy: Always
image: gcr.io/dyzz-csi-staging/csi/gce-pd-driver:latest
image: gcr.io/example/csi/gce-pd-driver:latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also change this: https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/test/k8s-integration/main.go#L250

It is a bad dependency I have, but I have a work item to fix later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, changed it back.

davidz627
davidz627 reviewed Oct 5, 2018
View reviewed changes
deploy/kubernetes/dev/setup-cluster.yaml Outdated
verbs: ["create"]
- apiGroups: [""]
resources: ["endpoints"]
verbs: ["list", "watch", "create", "update", "delete", "get"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the snapshotter really needs all of these permissions on endpoints?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it, removed watch. The others seems necessary

@jingxu97
Copy link
Contributor Author

jingxu97 commented Oct 5, 2018

@davidz627 addressed comments. PTAL

@davidz627
Copy link
Contributor

/lgtm
Thanks @jingxu97 !!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2018
@davidz627
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, jingxu97

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2018
@davidz627
Copy link
Contributor

/retest

@davidz627
Copy link
Contributor

davidz627 commented Oct 5, 2018 •
edited
Loading

Discussed integration test failure offline, 2 conclusions:

  1. Need to turn off topology on csi provisioner, add flag to turn it on, cut a new version and use that version here with topology off.
  2. Turn off leader election in csi provisioner - then remove the roles and rolebindings added in this PR for supporting leader election.

I will link the relevant issues in csi provisioner when they are created

@davidz627 davidz627 mentioned this pull request Oct 9, 2018
Merged
@davidz627
Copy link
Contributor

Blocked on:
kubernetes-csi/external-provisioner#147
kubernetes-csi/external-provisioner#146

When these two issues are resolved:

  • Leader election should be turned off, whatever roles were added to support that can be removed from this PR
  • Topology support should be turned off by default

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2018
@jingxu97
Add snapshot deployment support
1ec5d48 
Add the snapshotter deployment support and example
@jingxu97
Copy link
Contributor Author

@davidz627 The PR is updated with new provisioner version. PTAL.

@davidz627
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2018
@k8s-ci-robot k8s-ci-robot merged commit 7eee10d into kubernetes-sigs:master Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 Awaiting requested review from msau42

@saad-ali saad-ali Awaiting requested review from saad-ali

1 more reviewer

@davidz627 davidz627 davidz627 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@davidz627 davidz627

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jingxu97 @davidz627 @k8s-ci-robot