kubernetes-csi / csi-proxy

CSI Proxy utility to enable CSI Plugins on Windows
Apache License 2.0
58 stars 61 forks source link

Failing closed after maximum retry is achieved to avoid inf recursion #336

Closed knabben closed 6 days ago

knabben commented 6 months ago

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 /kind bug

What this PR does / why we need it: Adding a maximum retry for the CSI to find the volume target from a mountpath.

Which issue(s) this PR fixes:

Fixes #193

Special notes for your reviewer: Tracking the recursion call trace here: https://github.com/kubernetes-csi/csi-proxy/issues/193#issuecomment-2059665463

Does this PR introduce a user-facing change?:

Maximum retry (3) when for the getTarget method iterate and find the correct volume of the mount path. 
k8s-ci-robot commented 6 months ago

Hi @knabben. Thanks for your PR.

I'm waiting for a kubernetes-csi 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
knabben commented 6 months ago

/assign @mauriciopoppe

jsturtevant commented 6 months ago

/ok-to-test

k8s-ci-robot commented 6 months ago

@jsturtevant: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to [this](https://github.com/kubernetes-csi/csi-proxy/pull/336#issuecomment-2061907121): >/ok-to-test Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
mauriciopoppe commented 6 months ago

/ok-to-test

mauriciopoppe commented 5 months ago

I believe CI is failing because the kubernetes startup script on GCE instances is not working for Windows, cc @anishshah

AnishShah commented 5 months ago

I had filed kubernetes/kubernetes#124047 to track sig-windows-gce test job failures.

mauriciopoppe commented 4 months ago

/retest

knabben commented 4 months ago

@mauriciopoppe any news in the gcp windows testing?

mauriciopoppe commented 4 months ago

Thanks for following up, our CI infra is up and running but a presubmit within Github Actions failed with tests unrelated with this change:

=== RUN   TestSmbAPIGroup/v1alpha1SmbTests
    smb_v1alpha1_test.go:30: TestSmbAPIGroup setupUser failed: exit status 1, output: "ConvertTo-SecureString : The 'ConvertTo-SecureString' command was found in the module 'Microsoft.PowerShell.Security', \r\nbut the module could not be loaded. For more information, run 'Import-Module Microsoft.PowerShell.Security'.\r\nAt line:1 char:10\r\n+ $PWord = ConvertTo-SecureString $Env:password -AsPlainText -Force;New ...\r\n+          ~~~~~~~~~~~~~~~~~~~~~~\r\n    + CategoryInfo          : ObjectNotFound: (ConvertTo-SecureString:String) [], CommandNotFoundException\r\n    + FullyQualifiedErrorId : CouldNotAutoloadMatchingModule\r\n \r\nNew-LocalUser : Cannot validate argument on parameter 'Password'. The argument is null. Provide a valid value for the \r\nargument, and then try running the command again.\r\nAt line:1 char:132\r\n+ ... w-Localuser -name $Env:username -accountneverexpires***                                                                    ~~~~~~\r\n    + CategoryInfo          : InvalidData: (:) [New-LocalUser], ParameterBindingValidationException\r\n    + FullyQualifiedErrorId : ParameterArgumentValidationError,Microsoft.PowerShell.Commands.NewLocalUserCommand\r\n \r\n"

The failure above (only CI setup in Github actions) would need to be fixed in another PR and then we can retrigger presubmit tests in this PR to finally get it merged.

k8s-triage-robot commented 1 month ago

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

mauriciopoppe commented 1 week ago

/remove-lifecycle stale

mauriciopoppe commented 1 week ago

Closing and reopening to trigger presubmit checks.

mauriciopoppe commented 1 week ago

/lgtm /approved

mauriciopoppe commented 1 week ago

/approve

k8s-ci-robot commented 1 week ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knabben, mauriciopoppe

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](https://github.com/kubernetes-csi/csi-proxy/blob/master/OWNERS)~~ [mauriciopoppe] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
mauriciopoppe commented 1 week ago

/retest

mauriciopoppe commented 1 week ago

Seems like Prow is refusing to merge because of a Github Action run that's obsolete but that it's still considering for merge

Failing closed after maximum retry is achieved to avoid inf recursion error integration_tests (1.20, windows-latest) https://github.com/kubernetes-csi/csi-proxy/actions/runs/9211146453/job/26152630597

@knabben would it be possible to force push a new commit without any changes? I think that'd force Prow to check new Github Action runs.

mauriciopoppe commented 1 week ago

/lgtm

mauriciopoppe commented 1 week ago

/hold

Would it be possible to squash the commits? I think we should be able to cherrypick the commit to the library branch for CSI Proxy v2 after that.

mauriciopoppe commented 6 days ago

/remove hold /lgtm

mauriciopoppe commented 6 days ago

/remove-hold