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

Migrate to AWS SDKv2 #1963

Merged
merged 1 commit into from
Apr 1, 2024
Merged

Migrate to AWS SDKv2 #1963

merged 1 commit into from
Apr 1, 2024

Conversation

torredil
Copy link
Member

@torredil torredil commented Mar 13, 2024

Is this a bug fix or adding new feature?

Dependency migration

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

In alignment with our SDKs and Tools Maintenance Policy, AWS SDK for Go (v1) will enter maintenance mode on July 31, 2024 and reach end-of-support on July 31, 2025. This PR implements the necessary changes to migrate to AWS SDK for Go v2.

Fixes #1684

Additional Information

https://aws.github.io/aws-sdk-go-v2/docs/migrating/

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 13, 2024
@torredil
Copy link
Member Author

/hold

@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 Mar 13, 2024
@k8s-ci-robot k8s-ci-robot requested a review from ConnorJC3 March 13, 2024 21:12
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 13, 2024
Copy link

github-actions bot commented Mar 13, 2024

Code Coverage Diff

File Old Coverage New Coverage Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/cloud.go 83.3% 84.4% 1.1
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/metadata.go 86.4% 96.7% 10.3
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/metadata_ec2.go 84.1% 94.4% 10.4
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/metadata_k8s.go 77.1% 80.0% 2.9
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/controller.go 83.5% 83.6% 0.1
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/node.go 80.8% 80.6% -0.2
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/util/util.go 86.4% 80.0% -6.4

@torredil
Copy link
Member Author

/retest

Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

Overall thank you so much for doing a great job on this herculean migration!

I still need to review a few more files, and run some more manual tests, but overall this looks great so far.

Have a gift of a few questions and nits to for now for your efforts 😅.

Files left to review:

cloud_test.go
handlers.go
metadata_test.go
controller.go
controller_test.go

pkg/cloud/metadata_ec2.go Show resolved Hide resolved
pkg/cloud/metadata_ec2.go Show resolved Hide resolved
pkg/cloud/cloud.go Outdated Show resolved Hide resolved
pkg/cloud/cloud.go Show resolved Hide resolved
pkg/cloud/cloud.go Show resolved Hide resolved
pkg/cloud/cloud.go Outdated Show resolved Hide resolved
pkg/cloud/cloud.go Show resolved Hide resolved
pkg/cloud/cloud.go Outdated Show resolved Hide resolved
pkg/cloud/cloud.go Show resolved Hide resolved
@AndrewSirenko
Copy link
Contributor

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2024
@torredil
Copy link
Member Author

Fixed CreateSnapshot unit test - turns out, tc.expInput is created in the test function while the actual input passed to CreateSnapshot is created inside the func, even though the contents of the structs are the same they are separate instances with different memory addresses and hence why the equality check failed.

Fixed now + the go sum merge conflict. No issues spotted in manual testing + external e2e tests are all green.

The last remaining action item is RCAing why make verify hangs here in CI environment, but succeeds locally:

$ make verify
go vet $(go list ./...)
INFO Execution took 960.184531ms
./hack/verify-update.sh
All verifications passed!

@torredil
Copy link
Member Author

Upgrading linter for make verify in #1971

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2024
@torredil torredil force-pushed the sdkv2 branch 2 times, most recently from 1f22563 to d23367d Compare March 21, 2024 21:11
@torredil
Copy link
Member Author

Logs captured during manual testing for new middleware handler implementation used to record AWS API metrics:

Handling connection for 3301
# HELP cloudprovider_aws_api_request_duration_seconds [ALPHA] ebs_csi_aws_com metric
# TYPE cloudprovider_aws_api_request_duration_seconds histogram
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="0.005"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="0.01"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="0.025"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="0.05"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="0.1"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="0.25"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="0.5"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="1"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="2.5"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="5"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="10"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="AttachVolume",le="+Inf"} 1
cloudprovider_aws_api_request_duration_seconds_sum{request="AttachVolume"} 0.386986709
cloudprovider_aws_api_request_duration_seconds_count{request="AttachVolume"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="0.005"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="0.01"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="0.025"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="0.05"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="0.1"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="0.25"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="0.5"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="1"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="2.5"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="5"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="10"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="CreateVolume",le="+Inf"} 1
cloudprovider_aws_api_request_duration_seconds_sum{request="CreateVolume"} 0.244747201
cloudprovider_aws_api_request_duration_seconds_count{request="CreateVolume"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="0.005"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="0.01"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="0.025"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="0.05"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="0.1"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="0.25"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="0.5"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="1"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="2.5"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="5"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="10"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeInstances",le="+Inf"} 1
cloudprovider_aws_api_request_duration_seconds_sum{request="DescribeInstances"} 0.067674695
cloudprovider_aws_api_request_duration_seconds_count{request="DescribeInstances"} 1
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeVolumes",le="0.005"} 0
cloudprovider_aws_api_request_duration_seconds_bucket{request="DescribeVolumes",le="0.01"} 0

/unhold

@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 Mar 21, 2024
Copy link
Contributor

@ConnorJC3 ConnorJC3 left a comment

Choose a reason for hiding this comment

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

Initial review - very little manual testing

Intend to do the bulk of manual testing early next week, after which I will publish a final review

(expect very little additional comments in the final review unless I catch a bug with the manual testing)

pkg/driver/controller.go Outdated Show resolved Hide resolved
tests/e2e/testsuites/e2e_utils.go Outdated Show resolved Hide resolved
pkg/util/util.go Outdated Show resolved Hide resolved
pkg/driver/controller_test.go Outdated Show resolved Hide resolved
pkg/cloud/cloud.go Outdated Show resolved Hide resolved
pkg/cloud/cloud.go Outdated Show resolved Hide resolved
pkg/cloud/metadata_test.go Outdated Show resolved Hide resolved
pkg/cloud/metadata_test.go Outdated Show resolved Hide resolved
pkg/cloud/cloud_test.go Outdated Show resolved Hide resolved
@ConnorJC3
Copy link
Contributor

/lgtm

I tried my hardest to break it, and I failed.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2024
pkg/cloud/cloud.go Outdated Show resolved Hide resolved
Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

Great work on this tricky migration. Thank you for delivering results.

Signed-off-by: torredil <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2024
@torredil
Copy link
Member Author

torredil commented Apr 1, 2024

/approve
cc: @ConnorJC3 @AndrewSirenko

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil

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 Apr 1, 2024
@ConnorJC3
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 Apr 1, 2024
@k8s-ci-robot k8s-ci-robot merged commit c424cbf into kubernetes-sigs:master Apr 1, 2024
19 checks passed
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.

Use AWS sdk v2 instead of v1
4 participants