Skip to content

Install snapshot controller independently of helm for e2e tests#968

Merged
k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom
wongma7:fixeks
Jul 8, 2021
Merged

Install snapshot controller independently of helm for e2e tests#968
k8s-ci-robot merged 1 commit intokubernetes-sigs:masterfrom
wongma7:fixeks

Conversation

@wongma7
Copy link
Copy Markdown

@wongma7 wongma7 commented Jul 7, 2021

Is this a bug fix or adding new feature?

What is this PR about? / Why do we need it?
In preparation for removing snapshot controller installation from our helm chart #965, try using other methods to install it as a prerequisite.

  • kops e2e tests:
    • upgrade kops to 1.21.0 and add snapshotController.enabled: true to hack/kops-patch.yaml (the resources are embedded in kops, copied from external-snapshotter repo) same as below
  • eks e2e tests:
    • kubectl apply the resources from external-snapshotter github repo.

What testing is done?

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wongma7

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

The pull request process is described here

Details 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 requested review from gnufied and vdhanan July 7, 2021 21:11
@k8s-ci-robot k8s-ci-robot added 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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 7, 2021
@wongma7
Copy link
Copy Markdown
Author

wongma7 commented Jul 7, 2021

error: error creating cluster: [spec.snapshotController.enabled: Forbidden: Snapshot controller requires that cert manager is enabled, spec.snapshotController.enabled: Forbidden: Snapshot controller requires external CSI Driver]

hmm, this is disappointing

@wongma7
Copy link
Copy Markdown
Author

wongma7 commented Jul 7, 2021

So as a kops user, if I want it to install snapshot controller, i must also have it install the driver for me. i.e.

kops snapshot controller + helm ebs csi driver = invalid
kops snapshot controller + kops ebs csi driver = valid
kubectl apply snapshot controller + helm ebs csi driver = valid (our tests will do this then for both kops and eks )

@wongma7
Copy link
Copy Markdown
Author

wongma7 commented Jul 7, 2021

error listing SQS queues: AccessDenied: Access to the resource https://sqs.us-west-2.amazonaws.com/ is denied.
status code: 403, request id: 4d2f6ebe-7072-539f-8e6d-37de6fea95d3

what the heck?

/retest

@wongma7
Copy link
Copy Markdown
Author

wongma7 commented Jul 7, 2021

/test pull-aws-ebs-csi-driver-e2e-single-az

should have sqs permissions now. kubernetes/kops#11299

@wongma7
Copy link
Copy Markdown
Author

wongma7 commented Jul 7, 2021

/retest

Comment thread hack/e2e/run.sh Outdated

if [[ "${EBS_INSTALL_SNAPSHOT}" == true ]]; then
loudecho "Installing snapshot controller and CRDs"
kubectl apply --kubeconfig "${KUBECONFIG}" -f https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/v4.1.1/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's ok to be hardcoding the snapshot-controller version here? Do we at least want to extract it into a variable to make it easier to change?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed, made it variable

@nckturner
Copy link
Copy Markdown

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit 168310e into kubernetes-sigs:master Jul 8, 2021
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. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants