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

Eliminate use of problematic url.Parse() #1998

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

cphvmware
Copy link
Contributor

It doesn't work correctly with windows pathnames because they start with c: and the parser thinks that's a user name.

What this PR does / why we need it:
Without this change the windows driver won't start. The windows nodes get an error

invalid port :\csi\csi.sock after host

Testing done:
After the fix, windows Node Daemonsets Pods are no longer crashing. Test results (from PR #1996):

# kubectl get pods --namespace=vmware-system-csi
NAME                                      READY   STATUS    RESTARTS       AGE
vsphere-csi-controller-78fc759cb6-78vgd   7/7     Running   13 (15h ago)   3d2h
vsphere-csi-controller-78fc759cb6-l99bg   7/7     Running   18 (23h ago)   3d2h
vsphere-csi-controller-78fc759cb6-nl4mg   7/7     Running   24 (23h ago)   3d2h
vsphere-csi-node-dr8mx                    3/3     Running   3 (3d2h ago)   3d2h
vsphere-csi-node-g5n29                    3/3     Running   2 (23h ago)    3d2h
vsphere-csi-node-kzzqb                    3/3     Running   2 (3d2h ago)   3d2h
vsphere-csi-node-windows-dxcsq            3/3     Running   0              14m
vsphere-csi-node-windows-mfmtf            3/3     Running   0              14m
vsphere-csi-node-windows-zx76n            3/3     Running   0              12m
# kubectl describe csinodetopologies tkg-vc-antrea-md-0-windows-containerd-5c7888d6bc-85d9n
Name:         tkg-vc-antrea-md-0-windows-containerd-5c7888d6bc-85d9n
Namespace:    
Labels:       <none>
Annotations:  <none>
API Version:  cns.vmware.com/v1alpha1
Kind:         CSINodeTopology
Metadata:
  Creation Timestamp:  2022-09-23T21:24:23Z
  Generation:          2
  Managed Fields:
    API Version:  cns.vmware.com/v1alpha1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:ownerReferences:
          .:
          k:{"uid":"1dad2eee-be89-489f-b547-eb62e7bb271e"}:
      f:spec:
        .:
        f:nodeID:
        f:nodeuuid:
      f:status:
    Manager:      csi.exe
    Operation:    Update
    Time:         2022-09-23T21:24:23Z
    API Version:  cns.vmware.com/v1alpha1
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        f:status:
    Manager:    vsphere-syncer
    Operation:  Update
    Time:       2022-09-23T21:24:23Z
  Owner References:
    API Version:     v1
    Kind:            Node
    Name:            tkg-vc-antrea-md-0-windows-containerd-5c7888d6bc-85d9n
    UID:             1dad2eee-be89-489f-b547-eb62e7bb271e
  Resource Version:  1313374
  UID:               2abf091d-2752-4ed2-8a40-87a4145f12fb
Spec:
  Node ID:   tkg-vc-antrea-md-0-windows-containerd-5c7888d6bc-85d9n
  Nodeuuid:  4229924b-8335-d977-f382-cf465661ac60
Status:
  Status:  Success
Events:
  Type    Reason                      Age    From            Message
  ----    ------                      ----   ----            -------
  Normal  TopologyRetrievalSucceeded  2m36s  cns.vmware.com  Not a topology aware cluster.

Special notes for your reviewer:

Release note:

Don't use url.Parse() on Windows unix:// sockets since it doesn't parse Windows filenames correctly.

It doesn't work correctly with windows pathnames because they start with c: and
the parser thinks that's a user name.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 24, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @cphvmware. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 24, 2022
@divyenpatel
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 24, 2022
@divyenpatel divyenpatel added release-2.7.0-candidate release-2.6.0-candidate Indicates PR needs to be cherry-picked for 2.6.0 release and removed release-2.6.0-candidate Indicates PR needs to be cherry-picked for 2.6.0 release labels Sep 26, 2022
@svcbot-qecnsdp
Copy link

Started Vanilla block pre-checkin pipeline... Build Number: 1388

Copy link
Member

@divyenpatel divyenpatel left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cphvmware, divyenpatel

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 Sep 26, 2022
@svcbot-qecnsdp
Copy link

Build ID: 1388
Block vanilla build status: FAILURE 
Stage before exit: e2e-tests 
Jenkins E2E Test Results: 
------------------------------

Ran 1 of 638 Specs in 317.946 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 637 Skipped
PASS

Ginkgo ran 1 suite in 6m11.854631279s
Test Suite Passed
--
------------------------------

Ran 12 of 638 Specs in 3957.348 seconds
SUCCESS! -- 12 Passed | 0 Failed | 0 Pending | 626 Skipped
PASS

Ginkgo ran 1 suite in 1h6m12.830098659s
Test Suite Passed
--

Ran 40 of 638 Specs in 842.669 seconds
FAIL! -- 38 Passed | 2 Failed | 0 Pending | 598 Skipped


Ginkgo ran 1 suite in 14m18.657294184s

Test Suite Failed

@divyenpatel
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2022
@k8s-ci-robot k8s-ci-robot merged commit 9809abb into kubernetes-sigs:master Sep 26, 2022
divyenpatel pushed a commit to divyenpatel/vsphere-csi-driver that referenced this pull request Sep 26, 2022
It doesn't work correctly with windows pathnames because they start with c: and
the parser thinks that's a user name.
k8s-ci-robot pushed a commit that referenced this pull request Sep 26, 2022
* fix GetSystemUUID to return correct VM UUID for Windows Nodes (#1996)

* Eliminate use of problematic url.Parse() (#1998)

It doesn't work correctly with windows pathnames because they start with c: and
the parser thinks that's a user name.

Co-authored-by: cphvmware <111934873+cphvmware@users.noreply.github.com>
adikul30 pushed a commit to adikul30/vsphere-csi-driver that referenced this pull request Sep 26, 2022
It doesn't work correctly with windows pathnames because they start with c: and
the parser thinks that's a user name.
k8s-ci-robot pushed a commit that referenced this pull request Sep 27, 2022
* fix GetSystemUUID to return correct VM UUID for Windows Nodes (#1996)

* Eliminate use of problematic url.Parse() (#1998)

It doesn't work correctly with windows pathnames because they start with c: and
the parser thinks that's a user name.

* update golangci-lint to 1.49.0

Co-authored-by: Divyen Patel <divyenp@vmware.com>
Co-authored-by: cphvmware <111934873+cphvmware@users.noreply.github.com>
adikul30 pushed a commit to adikul30/vsphere-csi-driver that referenced this pull request Oct 26, 2022
* fix GetSystemUUID to return correct VM UUID for Windows Nodes (kubernetes-sigs#1996)

* Eliminate use of problematic url.Parse() (kubernetes-sigs#1998)

It doesn't work correctly with windows pathnames because they start with c: and
the parser thinks that's a user name.

Co-authored-by: cphvmware <111934873+cphvmware@users.noreply.github.com>
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-2.7.0-candidate release-2.7.0-cherry-picked size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants