Update driver to support compute staging #1586

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.

Sign up for GitHub

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

Jump to bottom

Merged

k8s-ci-robot merged 10 commits into kubernetes-sigs:master from Sneha-at:add_staging_support

Feb 16, 2024

Contributor

Sneha-at commented Feb 2, 2024 •

edited

Loading

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
The PR updates PDCSI driver to support GCE staging compute for internal testing.
Which issue(s) this PR fixes:

Fixes #
NA
Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot added release-note-none kind/feature cncf-cla: yes labels

k8s-ci-robot requested review from leiyiz and sunnylovestiramisu

February 2, 2024 21:30

k8s-ci-robot added the size/L label

Contributor

leiyiz commented Feb 2, 2024

/retest

sunnylovestiramisu reviewed

View reviewed changes

pkg/gce-pd-csi-driver/controller.go Show resolved Hide resolved

sunnylovestiramisu reviewed

View reviewed changes

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

sunnylovestiramisu reviewed

View reviewed changes

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

sunnylovestiramisu reviewed

View reviewed changes

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

Sneha-at force-pushed the add_staging_support branch from 4006ce7 to 2d40591 Compare

February 2, 2024 23:56


          update driver to support staging compute

f5071fa

Sneha-at force-pushed the add_staging_support branch from 2d40591 to f5071fa Compare

February 2, 2024 23:59

k8s-ci-robot added size/M and removed size/L labels

Contributor Author

Sneha-at commented Feb 5, 2024

/test pull-gcp-compute-persistent-disk-csi-driver-e2e

Contributor

sunnylovestiramisu commented Feb 5, 2024

/retest


          add logs for testing

4d931b6

k8s-ci-robot added size/L and removed size/M labels


          add logs for PID

6aae7d2

pwschuurman reviewed

View reviewed changes

cmd/gce-pd-csi-driver/main.go Outdated Show resolved Hide resolved

pwschuurman reviewed

View reviewed changes

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

pwschuurman reviewed

View reviewed changes

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

pkg/gce-pd-csi-driver/controller.go Show resolved Hide resolved

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

Sneha-at force-pushed the add_staging_support branch from fd460d3 to fe0f8dd Compare

February 13, 2024 23:25


          Add checks for compute environment flags and url checks

ce810c4

Sneha-at force-pushed the add_staging_support branch from fe0f8dd to ce810c4 Compare

February 14, 2024 00:18

Sneha-at added 2 commits

February 14, 2024 21:36


          add unit test

db3f3d9


          Cleaning up some added debug log

097e5b9

Sneha-at added 2 commits

February 14, 2024 23:20


          fail on incorrect endpoint

46ad7e7


          remove invalid compute endpoint testcase as updated logic causes cras…

13f981e

…h on invalid endpoint

pwschuurman reviewed

View reviewed changes

cmd/gce-pd-csi-driver/main.go Outdated Show resolved Hide resolved

pkg/gce-cloud-provider/compute/gce.go Outdated

+              		if err != nil {
+              			return nil, err
+              		}
+              		endpoint := computeURL.JoinPath(computeEnvironmentSuffix).String()

Contributor

pwschuurman Feb 15, 2024

Rather than JoinPath, I think we should ignore the path and just construct it ourselves. This will make the logic backwards compatible for anything that a calling process that runs the driver with --compute-endpoint string that includes a path.

computeURL.Path = computeEnvironmentSuffix
endpoint := computeURL.String()

You may need to update your getPath() function to include a leading / character when constructing the path.

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

pkg/gce-cloud-provider/compute/gce.go Outdated

+              	return computeOpts, nil
+              }
+              func getPath(env Environment, version Version) string {

Contributor

pwschuurman Feb 15, 2024

nit: I would call this constructComputeEndpointPath, rather than getPath. The name getPath is genericvery generic, and get implies we're either fetching a field from an object, or we're retrieving specific data from a higher level resource.

pkg/gce-cloud-provider/compute/gce.go Outdated

               	if computeEndpoint != "" {
-              		v1Endpoint := fmt.Sprintf("%s/compute/v1/", computeEndpoint)
-              		computeOpts = append(computeOpts, option.WithEndpoint(v1Endpoint))
+              		computeURL, err := url.ParseRequestURI(computeEndpoint)

Contributor

pwschuurman Feb 15, 2024

The validation of computeEndpoint could be done at the initialization layer. You could do something very similar to your enumFlag() call, where you validate the computeEndpoint string set a computeURL value (pointer) with either the value, or nil if no computeURL is provided.

This allows you to avoid needing to check for err here, and your function on the construction of the URL and path.

sunnylovestiramisu reviewed

View reviewed changes

test/e2e/tests/single_zone_e2e_test.go

@@ @@ -1295,15 +1295,6 @@ var _ = Describe("GCE PD CSI Driver", func() { @@
               		klog.Infof("Creating new driver and client for node %s\n", i.GetName())
-              		// Create new driver and client w/ invalid endpoint
-              		tcInvalid, err := testutils.GCEClientAndDriverSetup(i, "invalid-string")

Contributor

sunnylovestiramisu Feb 15, 2024

So we drop the invalid endpoint case?

Contributor

pwschuurman Feb 15, 2024

Yes, IMO this should be treated as input validation for the program rather than the RPC layer. Passing an invalid endpoint should crash the program, rather than cause a delayed RPC error.

Contributor

sunnylovestiramisu commented Feb 15, 2024

/lgtm

k8s-ci-robot assigned sunnylovestiramisu

k8s-ci-robot added the lgtm label


          Add url check during initialization

81485ea

k8s-ci-robot removed the lgtm label


          Update flag to use pointers

afe6c38

Contributor

pwschuurman commented Feb 16, 2024

/lgtm
/approve

k8s-ci-robot assigned pwschuurman

k8s-ci-robot added the lgtm label

Contributor

k8s-ci-robot commented Feb 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pwschuurman, Sneha-at

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~~ [pwschuurman]

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 approved label

k8s-ci-robot merged commit 05d893a into kubernetes-sigs:master

Member

amacaskill commented Feb 17, 2024

/cherry-pick release-1.13

k8s-infra-cherrypick-robot commented Feb 17, 2024

@amacaskill: #1586 failed to apply on top of branch "release-1.13":

Applying: update driver to support staging compute
Applying: add logs for testing
Applying: add logs for PID
Applying: Add checks for compute environment flags and url checks
Using index info to reconstruct a base tree...
M	test/e2e/tests/single_zone_e2e_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/tests/single_zone_e2e_test.go
CONFLICT (content): Merge conflict in test/e2e/tests/single_zone_e2e_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0004 Add checks for compute environment flags and url checks
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.13

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.

Contributor

pwschuurman commented Feb 17, 2024

@amacaskill Can you also cherrypick #1609 once that PR is merged? Something like:

~/go/src/k8s.io/kubernetes/hack/cherry_pick_pull.sh upstream/release-1.13 1586 1609

Sneha-at mentioned this pull request

Automated cherry pick of #1586: update driver to support staging compute #1609: fix pointer issue for GCE staging support #1614

Merged

k8s-ci-robot added a commit that referenced this pull request


          Merge pull request #1614 from Sneha-at/automated-cherry-pick-of-#1586-#…

52239f2

…1609-upstream-release-1.13

Automated cherry pick of #1586: update driver to support staging compute
#1609: fix pointer issue for GCE staging support

amacaskill added a commit that referenced this pull request


          Revert "Automated cherry pick of #1586: update driver to support stag…

38dd889

…ing compute

#1609: fix pointer issue for GCE staging support"

k8s-ci-robot added a commit that referenced this pull request


          Merge pull request #1615 from kubernetes-sigs/revert-1614-automated-c…

c60d05b

…herry-pick-of-#1586-#1609-upstream-release-1.13

Revert "Automated cherry pick of #1586: update driver to support staging compute#1609: fix pointer issue for GCE staging support"

Sneha-at mentioned this pull request

Automated cherry pick of #1586: update driver to support staging compute #1609: fix pointer issue for GCE staging support #1617

Merged

k8s-ci-robot added a commit that referenced this pull request


          Merge pull request #1617 from Sneha-at/automated-cherry-pick-of-#1586-#…

722430e

…1609-upstream-release-1.13

Automated cherry pick of #1586: update driver to support staging compute
#1609: fix pointer issue for GCE staging support

Sneha-at mentioned this pull request

Fix error when no compute endpoint is passed #1620

Merged

Sneha-at mentioned this pull request

Automated cherry pick of #1586: update driver to support staging compute #1609: fix pointer issue for GCE staging support #1620: Update to handle when no compute endpoint is passed #1653

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cncf-cla: yes kind/feature lgtm release-note-none size/L