🌱 Upgrade CSI driver to v3.1.0 #2364

laozc · 2023-09-18T01:11:30Z

What this PR does / why we need it:
We're still using 2.1.0 in the templates while the latest CSI driver now is 3.1.0 with more features.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2331

codecov · 2023-09-18T01:19:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (47e62f1) 63.60% compared to head (e86f148) 63.58%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2364      +/-   ##
==========================================
- Coverage   63.60%   63.58%   -0.03%     
==========================================
  Files         123      123              
  Lines        8773     8773              
==========================================
- Hits         5580     5578       -2     
- Misses       2777     2778       +1     
- Partials      416      417       +1

see 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

killianmuldoon · 2023-09-18T08:44:39Z

/test

k8s-ci-robot · 2023-09-18T08:44:41Z

@killianmuldoon: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-vsphere-conformance-main
/test pull-cluster-api-provider-vsphere-e2e-full-main
/test pull-cluster-api-provider-vsphere-e2e-main
/test pull-cluster-api-provider-vsphere-test-integration-main
/test pull-cluster-api-provider-vsphere-test-main
/test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-vsphere-apidiff-main
pull-cluster-api-provider-vsphere-e2e-main
pull-cluster-api-provider-vsphere-test-integration-main
pull-cluster-api-provider-vsphere-test-main
pull-cluster-api-provider-vsphere-verify-main

In response to this:

/test

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.

killianmuldoon

/test pull-cluster-api-provider-vsphere-e2e-full-main

chrischdi

One question but no bigger opinion on this.
As of that:

/lgtm

packaging/flavorgen/flavors/crs/csi.go

chrischdi · 2023-09-18T11:25:14Z

templates/cluster-template-external-loadbalancer.yaml

+            - name: GODEBUG
+              value: x509sha1=1


Can we add a comment why this one is needed? Or is this simply copy/paste?

It's copied from the CSI manifest.
The flag is used to enable the deprecated SHA1 certificate support in some enterprise environment for on-prem Kubernetes installations.
https://github.com/kubernetes-sigs/vsphere-csi-driver/blob/616f90f7e38a24fceda3673819abafd20da08859/manifests/vanilla/vsphere-csi-driver.yaml#L367C1-L368C32

k8s-ci-robot · 2023-09-18T11:25:59Z

LGTM label has been added.

Git tree hash: 268c1bb64fe38acd284fe0db03d3197d1b39d0de

chrischdi

Wondering of it makes sense to copy all this over and make go code out of it, or if we should instead use go embed and only add the things we need (e.g. csi-config-secret)

However for now its good as it is.

/lgtm

k8s-ci-robot · 2023-09-19T07:27:33Z

LGTM label has been added.

Git tree hash: 1ce3dc3431be1fd94de71457661c3a5da5c5776f

sbueringer · 2023-09-19T08:59:04Z

Wondering of it makes sense to copy all this over and make go code out of it, or if we should instead use go embed and only add the things we need (e.g. csi-config-secret)

This seems pretty hard to maintain / keeping in sync going forward. @randomvariable WDYT? Should we follow the route to convert everything to go code and somehow try to keep this in sync or should we follow a more direct copying YAML over & diffing approach?

The latter seems a lot more maintainable to me

Q: In which format is CSI providing the YAML? Is there an all-in-one YAML or a Helm chart or both?

laozc · 2023-09-19T09:08:11Z

There is a full manifest at
https://github.com/kubernetes-sigs/vsphere-csi-driver/blob/master/manifests/vanilla/vsphere-csi-driver.yaml

I believe the CSI driver image in the file is generated for their CI.
For long term maintenance, it would be better to replicate the manifest content from CSI driver with our own customization.

chrischdi · 2023-09-19T09:47:09Z

Maybe a copy + kustomize patches for things we need to change could be a good way forward.

sbueringer · 2023-10-31T13:02:58Z

Okay let's go ahead with this. I took a look and I didn't see anything suspicious, but I also didn't diff it which seems a crazy amount of work. I'm fine with merging this PR after rebase & when all e2e tests are green.

But we have to find another way to do this. Let's create an issue to move this to using the upstream YAML + kustomizations on top. This is absolutely not sustainable (already wasn't before this PR, which is why I'm fine with merging this for now)

laozc · 2023-11-01T05:47:00Z

/retest-required

k8s-ci-robot · 2023-11-01T07:37:13Z

@laozc: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-vsphere-apidiff-main	`e86f148`	link	false	`/test pull-cluster-api-provider-vsphere-apidiff-main`

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.

sbueringer · 2023-11-02T08:15:24Z

/test pull-cluster-api-provider-vsphere-e2e-full-main

sbueringer · 2023-11-02T08:15:39Z

/lgtm
/approve

k8s-ci-robot · 2023-11-02T08:15:46Z

LGTM label has been added.

Git tree hash: 499534c46a5a56cb82f6787c11267d1a1d473bfc

k8s-ci-robot · 2023-11-02T08:15:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

~~OWNERS~~ [sbueringer]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 18, 2023

k8s-ci-robot requested review from chrischdi and gab-satchi September 18, 2023 01:11

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 18, 2023

laozc force-pushed the csi-upgrade branch from 05ebe45 to eb750fe Compare September 18, 2023 02:40

killianmuldoon reviewed Sep 18, 2023

View reviewed changes

chrischdi reviewed Sep 18, 2023

View reviewed changes

k8s-ci-robot assigned chrischdi Sep 18, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 18, 2023

laozc force-pushed the csi-upgrade branch from eb750fe to e3b2dc3 Compare September 19, 2023 00:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2023

k8s-ci-robot requested a review from chrischdi September 19, 2023 00:51

laozc force-pushed the csi-upgrade branch from e3b2dc3 to 331e249 Compare September 19, 2023 01:00

chrischdi reviewed Sep 19, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2023

laozc force-pushed the csi-upgrade branch from 331e249 to 8f64c53 Compare October 25, 2023 02:43

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2023

k8s-ci-robot requested a review from chrischdi October 25, 2023 02:43

laozc force-pushed the csi-upgrade branch 3 times, most recently from d487184 to 453e24e Compare October 25, 2023 03:44

laozc force-pushed the csi-upgrade branch from 453e24e to 5926e53 Compare October 25, 2023 04:24

laozc force-pushed the csi-upgrade branch from 5926e53 to c3d14cf Compare October 31, 2023 23:56

Update CSI driver to v3.1.0

e86f148

laozc force-pushed the csi-upgrade branch from c3d14cf to e86f148 Compare November 1, 2023 07:31

k8s-ci-robot assigned sbueringer Nov 2, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2023

k8s-ci-robot merged commit 0984b05 into kubernetes-sigs:main Nov 2, 2023
17 of 18 checks passed

k8s-ci-robot added this to the v1.9 milestone Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Upgrade CSI driver to v3.1.0 #2364

🌱 Upgrade CSI driver to v3.1.0 #2364

laozc commented Sep 18, 2023

codecov bot commented Sep 18, 2023 •

edited

Loading

killianmuldoon commented Sep 18, 2023

k8s-ci-robot commented Sep 18, 2023

killianmuldoon left a comment

chrischdi left a comment

chrischdi Sep 18, 2023

laozc Sep 19, 2023

k8s-ci-robot commented Sep 18, 2023

chrischdi left a comment

This comment was marked as duplicate.

k8s-ci-robot commented Sep 19, 2023

sbueringer commented Sep 19, 2023 •

edited

Loading

laozc commented Sep 19, 2023

chrischdi commented Sep 19, 2023

sbueringer commented Oct 31, 2023

laozc commented Nov 1, 2023

k8s-ci-robot commented Nov 1, 2023 •

edited

Loading

sbueringer commented Nov 2, 2023

sbueringer commented Nov 2, 2023

k8s-ci-robot commented Nov 2, 2023

k8s-ci-robot commented Nov 2, 2023

🌱 Upgrade CSI driver to v3.1.0 #2364

🌱 Upgrade CSI driver to v3.1.0 #2364

Conversation

laozc commented Sep 18, 2023

codecov bot commented Sep 18, 2023 • edited Loading

Codecov Report

killianmuldoon commented Sep 18, 2023

k8s-ci-robot commented Sep 18, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

chrischdi left a comment

Choose a reason for hiding this comment

chrischdi Sep 18, 2023

Choose a reason for hiding this comment

laozc Sep 19, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 18, 2023

chrischdi left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

k8s-ci-robot commented Sep 19, 2023

sbueringer commented Sep 19, 2023 • edited Loading

laozc commented Sep 19, 2023

chrischdi commented Sep 19, 2023

sbueringer commented Oct 31, 2023

laozc commented Nov 1, 2023

k8s-ci-robot commented Nov 1, 2023 • edited Loading

sbueringer commented Nov 2, 2023

sbueringer commented Nov 2, 2023

k8s-ci-robot commented Nov 2, 2023

k8s-ci-robot commented Nov 2, 2023

codecov bot commented Sep 18, 2023 •

edited

Loading

sbueringer commented Sep 19, 2023 •

edited

Loading

k8s-ci-robot commented Nov 1, 2023 •

edited

Loading