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

Bump k8s dependencies to 1.20.4, csi spec to 1.3, external-snapshotter client to v4, go to 1.16 #792

Closed
wants to merge 13 commits into from

Conversation

wongma7
Copy link
Contributor

@wongma7 wongma7 commented Mar 12, 2021

Is this a bug fix or adding new feature? fixes #723

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

commits 1-5: go dependency bumps

commits 6-9: YAML image/API version bumps

commits 10-13: e2e test kops version bumps

What testing is done?

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 12, 2021
@k8s-ci-robot
Copy link
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

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 Mar 12, 2021
@coveralls
Copy link

coveralls commented Mar 12, 2021

Pull Request Test Coverage Report for Build 1808

  • 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.2%) to 81.636%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/driver/controller.go 0 4 0.0%
Totals Coverage Status
Change from base Build 1801: -0.2%
Covered Lines: 1756
Relevant Lines: 2151

💛 - Coveralls

@wongma7 wongma7 force-pushed the windows branch 3 times, most recently from efb68bd to fa0ae77 Compare March 12, 2021 21:48
@wongma7
Copy link
Contributor Author

wongma7 commented Mar 12, 2021

client api changed, will fix.

../../../../pkg/mod/github.com/kubernetes-csi/external-snapshotter/[email protected]/pkg/client/clientset/versioned/typed/volumesnapshot/v1beta1/volumesnapshot.go:188:5: not enough arguments in call to c.client.Patch(pt).Namespace(c.ns).Resource("volumesnapshots").SubResource(subresources...).Name(name).Body(data).Do
have ()
want (context.Context)
../../../../pkg/mod/github.com/kubernetes-csi/external-snapshotter/[email protected]/pkg/client/clientset/versioned/typed/volumesnapshot/v1beta1/volumesnapshotclass.go:70:5: not enough arguments in call to c.client.Get().Resource("volumesnapshotclasses").Name(name).VersionedParams(&options, scheme.ParameterCodec).Do

@wongma7 wongma7 changed the title Bump k8s dependencies to 1.20.4 Bump k8s dependencies to 1.20.4, csi spec to 1.3, external-snapshotter client to v4, go to 1.16 Mar 12, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Mar 12, 2021

pkg/driver/driver.go:128:33: cannot use d (type *Driver) as type csi.ControllerServer in argument to csi.RegisterControllerServer:
*Driver does not implement csi.ControllerServer (missing ControllerGetVolume method)

I 'm going to leave it unimplemented

@wongma7
Copy link
Contributor Author

wongma7 commented Mar 15, 2021

/test pull-aws-ebs-csi-driver-external-test-latest

@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, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Mar 15, 2021

The code added in 1.19 for getting metrics from kcm pod doesn't work for us:

Mar 15 16:56:01.625: INFO: ERROR unable to parse requirement: invalid label value: "kube-controller-manager-ip-172-20-39-70.us-west-2.compute.internal": at key: "name": must be no more than 63 characters

@wongma7
Copy link
Contributor Author

wongma7 commented Mar 15, 2021

Also, there is no way to disable this metrics grabbing code. We already check KCM metrics ourselves at the end of the test to see if csi or in tree was getting called so it's redundant :/

@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 16, 2021
@wongma7 wongma7 force-pushed the windows branch 3 times, most recently from 274dac8 to dbf91b0 Compare March 16, 2021 19:57
@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 19, 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 Mar 24, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Mar 24, 2021

hmmm I need to fix my logging code:

Printing pod ebs-csi-controller-77fd584dc8-hkg6k ebs-plugin container logs

Error from server (Forbidden): Forbidden (user=system:anonymous, verb=get, resource=nodes, subresource=proxy) ( pods/log ebs-csi-controller-77fd584dc8-hkg6k)

@wongma7
Copy link
Contributor Author

wongma7 commented Mar 24, 2021

actually it's causing test failures as well since some of them check pod logs.

                  s: "error running /usr/local/bin/kubectl --server=https://api-test-cluster-8868-k8s-rjdnu3-1175551248.us-west-2.elb.amazonaws.com --kubeconfig=/root/.kube/config --namespace=volume-3874 exec aws-injector --namespace=volume-3874 -- /bin/sh -c echo 'Hello from aws from namespace volume-3874' > /opt/0/index.html:\nCommand stdout:\n\nstderr:\nerror: unable to upgrade connection: Forbidden (user=system:anonymous, verb=create, resource=nodes, subresource=proxy)\n\nerror:\nexit status 1",
              }, 

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Apr 7, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Apr 7, 2021

something to do with the blacklist /denylist change. kubernetes/test-infra#21687

@wongma7
Copy link
Contributor Author

wongma7 commented Apr 7, 2021

TooManyLoadBalancers

@wongma7
Copy link
Contributor Author

wongma7 commented Apr 7, 2021

/retest

@wongma7
Copy link
Contributor Author

wongma7 commented Apr 7, 2021

/verify-owners

@wongma7
Copy link
Contributor Author

wongma7 commented Apr 8, 2021

/verify-owners

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Apr 8, 2021
@wongma7 wongma7 removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Apr 8, 2021
@@ -32,7 +31,7 @@ spec:
tolerationSeconds: 300
containers:
- name: ebs-plugin
image: k8s.gcr.io/provider-aws/aws-ebs-csi-driver:v0.10.0
image: k8s.gcr.io/provider-aws/aws-ebs-csi-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.

Hmm this is kind of dangerous. What if we start building alpha images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

people should be using the stable overlay. However I agree best not to even mention latest tag, I'l lcheck where it's coming from (like is it being propagated from a helm chart?)

@k8s-ci-robot
Copy link
Contributor

@wongma7: PR needs rebase.

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.

@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 13, 2021
@jsafrane jsafrane mentioned this pull request Apr 15, 2021
@wongma7
Copy link
Contributor Author

wongma7 commented Apr 16, 2021

replaced by #828

@wongma7 wongma7 closed this 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

Bump kubernetes dependency to 1.19/1.20
4 participants