Skip to content
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.

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

Add snapshot-controller #10730

Merged
merged 1 commit into from
May 21, 2021

Conversation

olemarkus
Copy link
Member

Will install external-snapshotter
Also modifies AWS storage to use a csi-specific storageclass and default to gp3 by default if ebs csi driver is installed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 4, 2021
@k8s-ci-robot k8s-ci-robot added area/addons area/api approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 4, 2021
@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from ba2d638 to 12519fe Compare February 4, 2021 20:17
@@ -29,7 +29,7 @@ kind: StorageClass
metadata:
name: kops-ssd-1-17
annotations:
storageclass.beta.kubernetes.io/is-default-class: "true"
storageclass.beta.kubernetes.io/is-default-class: "{{ if .CloudConfig.AWSEBSCSIDriver.Enabled }}{{ else }}false{{ end }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a patch in the works that will require some coordination here, both with the annotation name and with surrounding the creation of the StorageClass objects in a conditional block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I really dislike the annotation path here for controlling which is the default. It made rolling out some best practices for new clusters + not breaking old oddly difficult back when I did this.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #10733 for my patch on the subject. We'll have to resolve a bunch of conflicts, depending on which one goes in first.

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll merge yours first

@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from 12519fe to 5330809 Compare February 5, 2021 19:29
@olemarkus
Copy link
Member Author

/milestone v1.20

@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Feb 5, 2021
@olemarkus
Copy link
Member Author

/milestone v1.21

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.20, v1.21 Feb 8, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2021
@olemarkus olemarkus modified the milestones: v1.21, v1.22 Mar 29, 2021
@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from 5330809 to 1a62361 Compare May 19, 2021 18:48
@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2021
@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from 1a62361 to 4c0538b Compare May 19, 2021 18:59
if c == nil {
return nil
cc := clusterSpec.CloudConfig
if cc.AWSEBSCSIDriver == nil {
Copy link
Member

Choose a reason for hiding this comment

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

do we need to check if cc is not nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

This would fail the tests if we had to, I think. This probably gets explicitly set in one of the former builders.

"ec2:DescribeAvailabilityZones",
"ec2:DescribeSnapshots",
),
Resource: stringorslice.Slice([]string{"*"}),
Copy link
Member

Choose a reason for hiding this comment

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

The example policy has tag conditions on DeleteSnapshot:

https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/0e9b0b92cd143c7a9d7390be54559e9ead167154/docs/example-iam-policy.json#L96-L120

I'm wondering if we could do the same, and perhaps even further restrict it based on a tag containing the cluster name

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. that makes sense.

@@ -16,7 +15,7 @@ kind: StorageClass
metadata:
name: gp2
annotations:
storageclass.kubernetes.io/is-default-class: "false"
storageclass.beta.kubernetes.io/is-default-class: "false"
Copy link
Member

Choose a reason for hiding this comment

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

is this addition still needed? I see upstream docs refer to the non-beta annotation.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was a mistake. Reverted.

@rifelpet
Copy link
Member

this looks good to me other than the integration test conflicts. I'll let someone else take a second pass at the new API fields.

We should revert #11530 in this PR as well.

@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from 4c0538b to 7242d4d Compare May 20, 2021 08:20
@k8s-ci-robot k8s-ci-robot added do-not-merge/contains-merge-commits and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 20, 2021
@olemarkus
Copy link
Member Author

this looks good to me other than the integration test conflicts. I'll let someone else take a second pass at the new API fields.

We should revert #11530 in this PR as well.

I want to cherry-pick this one to 1.21, so cannot revert that change directly in this PR. In addition the test need to be further modified to install the snapshot controller.

@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from 7242d4d to 94bd218 Compare May 20, 2021 08:21
@rifelpet
Copy link
Member

Looks like this still needs a rebase to get rid of a merge commit.

@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from 94bd218 to 073cab7 Compare May 20, 2021 14:21
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2021
@olemarkus
Copy link
Member Author

Classic mistake of only checking if the struct exists, not if it was actually enabled.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2021
@rifelpet
Copy link
Member

/hold
the default storage class logic in the template needs to be inverted

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2021
@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from 8071ae2 to 07e316b Compare May 21, 2021 11:50
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2021
@olemarkus
Copy link
Member Author

Also added encryption back in

Copy link
Member

@rifelpet rifelpet left a comment

Choose a reason for hiding this comment

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

/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 21, 2021
@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from 07e316b to 1740b23 Compare May 21, 2021 12:01
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2021
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 21, 2021
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 21, 2021

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

Test name Commit Details Rerun command
pull-kops-e2e-k8s-containerd 4c0538b link /test pull-kops-e2e-k8s-containerd

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.

Update upup/models/cloudup/resources/addons/storage-aws.addons.k8s.io/v1.15.0.yaml.template

Co-authored-by: Peter Rifel <[email protected]>

Update upup/models/cloudup/resources/addons/storage-aws.addons.k8s.io/v1.15.0.yaml.template

Co-authored-by: Peter Rifel <[email protected]>
@olemarkus olemarkus force-pushed the csi-snapshot-controller branch from 1740b23 to 46e13c0 Compare May 21, 2021 13:40
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman, rifelpet

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 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2021
@k8s-ci-robot k8s-ci-robot merged commit 864d2b6 into kubernetes:master May 21, 2021
k8s-ci-robot added a commit that referenced this pull request May 21, 2021
…730-origin-release-1.21

Automated cherry pick of #10730: Bump snapshot-controller version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/addons area/api area/nodeup 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants