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

Rebase 1.21 #828

Merged
merged 15 commits into from
Apr 16, 2021
Merged

Rebase 1.21 #828

merged 15 commits into from
Apr 16, 2021

Conversation

jsafrane
Copy link
Contributor

@jsafrane jsafrane commented Apr 13, 2021

Is this a bug fix or adding new feature?
feature (?)

What is this PR about? / Why do we need it?

Rebase these dependencies:

  • Kubernetes libs to 1.21.
  • CSI spec to 1.3 (pulled by Kubernetes libs).
    • I had to "implement" ControllerGetVolume to satisfy the new interface (it returns NotImplemented).
  • Remove klog v1. Not that it is needed, maybe it could go to a separate PR.
  • Use go 1.16.1

The main motivation is to upgrade to k8s.io/client-go and k8s.io/api that does not have CVE-2021-3121

What testing is done?
Very little! I checked that everything compiles, relying on e2e tests to catch errors.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 13, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 13, 2021
@wongma7
Copy link
Contributor

wongma7 commented Apr 13, 2021

travis fail:
go: inconsistent vendoring in /home/travis/gopath/src/github.com/kubernetes-sigs/aws-ebs-csi-driver:

github.com/mattn/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

you can pick up this change in my pr to fix it: f0489b3#diff-6ac3f79fc25d95cd1e3d51da53a4b21b939437392578a35ae8cd6d5366ca5485

(my pr to go to 1.20 took so long, and you are already doing 1.21 🤦 i will rebase my PR on yours, thank you.)

@coveralls
Copy link

coveralls commented Apr 13, 2021

Pull Request Test Coverage Report for Build 1839

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 81.741%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/driver/controller.go 0 4 0.0%
Totals Coverage Status
Change from base Build 1823: -0.1%
Covered Lines: 1822
Relevant Lines: 2229

💛 - Coveralls

@jsafrane jsafrane force-pushed the rebase-1.21 branch 3 times, most recently from 900a8e5 to a604055 Compare April 15, 2021 11:46
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 15, 2021
@jsafrane
Copy link
Contributor Author

I ended up taking #792 and adding 1.21 libraries on the top. @wongma7 still should get the credit for most of the commits.

@wongma7
Copy link
Contributor

wongma7 commented Apr 15, 2021

@jsafrane dont worry about it 😂 i did notice that your PR was slowly ballooning to look like mine, so may as well use my commits instead of duplciating efforts!

@ayberk can you help look at the test failures if u have time? Want to get this merged ASAP so it doesn't get stuck like my old pr.

@wongma7
Copy link
Contributor

wongma7 commented Apr 15, 2021

here is the issue :

Apr 15 12:13:10.769: INFO: At 2021-04-15 12:08:10 +0000 UTC - event for pvc-dr4t4: {ebs.csi.aws.com_ebs-csi-controller-55f457484f-f8kjb_1645b834-1bc4-405c-9794-4357cdd21ab5 } ProvisioningFailed: failed to provision volume with StorageClass "topology-62099z2rm": failed to translate storage class: failed translating allowed topologies: unknown topology key: topology.kubernetes.io/zone

@wongma7
Copy link
Contributor

wongma7 commented Apr 15, 2021

The error is coming from here: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/csi-translation-lib/plugins/in_tree_volume.go#L328.

but it looks to me that topology.kubernetes.io/zone == LabelTopologyZone = "topology.kubernetes.io/zone" so IDK why it's erroring.

@wongma7
Copy link
Contributor

wongma7 commented Apr 15, 2021

OK, looks like we need to update external-provisioner in order for it to recognize the GA label. I'm going to make a PR to eks-distro and ask them to build & push a new one to ECR:

In 1.21, we updated the in-tree driver to use the GA label when setting test StorageClasses' AllowedTopologies: kubernetes/kubernetes@339b8b4#diff-5470c26feb298d13a90aed98f13ff97cdbb327b121ef0976631f41553026a06fL1686 .

However, our v2.0.2 of the external-provisioner doesn't recognize the GA label as a valid translatable topology key, so in order to pass the new 1.21 test we need a newer version of external-provisioner (like v2.1.2) containing this change:
kubernetes/kubernetes@67fed31#diff-40f96191a7878d92ff0d264697956ff4aeb25da46763ec3367350c8c77a05598R405

@wongma7
Copy link
Contributor

wongma7 commented Apr 15, 2021

actually v2.1.1 is already present in ECR @jsafrane can you try bumping https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/charts/aws-ebs-csi-driver/values.yaml#L15 to v2.1.1-eks-1-19-3 https://gallery.ecr.aws/eks-distro/kubernetes-csi/external-provisioner and seeing if it helps migration test pass, thanks!!

To get updated CSI translation label names.
@jsafrane
Copy link
Contributor Author

Updated to 2.1.1 and everything passed. Trying second time
/test all

@wongma7
Copy link
Contributor

wongma7 commented Apr 16, 2021

/lgtm

thank you

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2021
@wongma7 wongma7 merged commit 498ef51 into kubernetes-sigs:master Apr 16, 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants