Fix download retry when get_url has no status_code.

[RomainMou]

What type of PR is this?
/kind bug

What this PR does / why we need it:
Sometime the return of get_url doesn't have a status_code, so we need to check if it's defined before we check its value to avoid an evaluation error.

Which issue(s) this PR fixes:
Fixes #10494

Special notes for your reviewer:
Should be backported in 2.23, the bug is introduce in the new 2.23.1 version.

Does this PR introduce a user-facing change?:

Fix download retry when get_url has no status_code

[k8s-ci-robot]

Hi @RomainMou. 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.

[VannTen]

Probably that's all it needs.
Not sure if I have the rights for that but anyway:
/lgtm

[k8s-ci-robot]

@VannTen: changing LGTM is restricted to collaborators

In response to this:

Probably that's all it needs.
Not sure if I have the rights for that but anyway:
/lgtm

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.

[Saigut]

Hello, from this code and here, we can see get_url_result will not have all attributes whenget_url failed. So the best way is to check the common attributes get_url_result.failed right?
And the code maybe something like:

    until: "(not get_url_result.failed) and ('OK' in get_url_result.msg or
      'file already exists' in get_url_result.msg or
      get_url_result.status_code == 304)"

[RomainMou]

I must admit, I don't know why we don't just do something like until: not get_url_result.failed

[VannTen]

Maybe @sathieu can provide some insights ?
I think it's because there was spurious failures when the http call returned 304

[RomainMou]

It wasn't use before the 304 stuff was added either: https://github.com/kubernetes-sigs/kubespray/pull/10452/files#diff-d53e3dc8dc33cc53fb013d9507a329cc7cadce6061d93ba648183bbd47906105L104

[sathieu]

Maybe @sathieu can provide some insights ? I think it's because there was spurious failures when the http call returned 304

Ansible uses If-Modified-Since when the file already exists on disk. If the webserver returns 304 Not Modified with this header, this should be considered a success, and not retried.

It seems that get_url is successful when the status is 304. I think until: not get_url_result.failed is good (at least for my usecase).

[yankay]

/ok-to-test

[RomainMou]

Hello,

Is there anything to do to move forward on this?
Should we try until: not get_url_result.failed?

[jayonlau]

/lgtm

[yankay]

Thanks @RomainMou

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RomainMou, yankay

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 [yankay]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment