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

fix: add Etag field to request body to let Etag work #8106

Merged
merged 2 commits into from
Jan 26, 2025

Conversation

mainred
Copy link
Contributor

@mainred mainred commented Jan 22, 2025

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR fill vmss and vmss vm Etag into request body before sending the put request, also this PR invalidated the vmss cache after updating the vmss to try to use the latest vmss mode with latest Etag before putting vmss. This logic has already been achieved in put vmss vm operations.

Tests have been done:
to let the EtagMiatch Error happen, I manually changed the code to let the first vmss update put with Etag "1", and the second one will pick the correct Etag, the same case will succeed. For the case of VMSS vm, I will let the put vmss vm fail by half by chaing the number on the first bulk put, and the second bulk puts succeed.
Reference: put vmss vmwith etagmimatch error mainred@5ef7fe9

  • kusto query from put vmss vm EtagMismatch

image

  • kusto query from put vmss EtagMismatch

image

I also tested add internal lb and delete internal lb for both cases above. For a detailed kusto query you may check

https://msazure.visualstudio.com/CloudNativeCompute/_wiki/wikis/personalplayground/746780/EtagMismatch-Error

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jan 22, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 22, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @mainred. 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 22, 2025
@feiskyer
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 Jan 22, 2025
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

LGTM for VMSS related changes, let's run some e2e tests to validate

@feiskyer
Copy link
Member

@MartinForReal we'd also need to check whether other resources need similar ETAG fix (that could be done separately)

@mainred
Copy link
Contributor Author

mainred commented Jan 22, 2025

fixing the UT

Signed-off-by: Qingchuan Hao <[email protected]>
@mainred mainred force-pushed the crp-etag-track2-fix branch from b163fd9 to 500b0af Compare January 22, 2025 09:30
@coveralls
Copy link

Coverage Status

coverage: 76.62% (+0.02%) from 76.603%
when pulling 500b0af on mainred:crp-etag-track2-fix
into ec40d31 on kubernetes-sigs:master.

@MartinForReal
Copy link
Contributor

superseded by #8117

@mainred
Copy link
Contributor Author

mainred commented Jan 23, 2025

@MartinForReal
I am not sure if you understand respecting the other's work.
I have a PR to add crp etag support in track2, you superseded my PR by saying my PR requires test.
and you superseded this PR by adding etag in putvm, but invalidating vmss vm cache is not covered in your PR but is essential.

I hope you can have a better test coverage of the your code change, since I am testing on my code heavily, but you superseded them.

secondly, the most prioritized work is to cut new cloud provider azure releases, which means your put vmss vm pr can be hold after your test credential provider track2 SDK,

cc @feiskyer

@feiskyer
Copy link
Member

#8117 is not related to this change, it is addressing a different problem of vmss batch operations.

Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, mainred

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 Jan 24, 2025
@feiskyer
Copy link
Member

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed do-not-merge/needs-kind labels Jan 24, 2025
@MartinForReal
Copy link
Contributor

MartinForReal commented Jan 24, 2025

@mainred please also update ensureBackendPoolDeletedFromNode
I'm not sure whether your test result is reproducible and related.
Could you please update the pr and get new test result?

@MartinForReal
Copy link
Contributor

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2025
@MartinForReal
Copy link
Contributor

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2025
@mainred
Copy link
Contributor Author

mainred commented Jan 26, 2025

/test pull-cloud-provider-azure-e2e-ccm-capz

@k8s-ci-robot k8s-ci-robot merged commit 83bc60b into kubernetes-sigs:master Jan 26, 2025
18 checks passed
@mainred
Copy link
Contributor Author

mainred commented Jan 26, 2025

Could you please update the pr and get new test result?

Updated the PR description with the test scenarios and test result.
By the same, the test scenarios and test result has been put internal teams change after my test. I guess you did not see it.

@mainred
Copy link
Contributor Author

mainred commented Jan 26, 2025

Follow up:
need to add Etag in ensureBackendPoolDeletedFromNode, and add UT
I lost the Etag change in ensureBackendPoolDeletedFromNode because I forgot the copy the change from test branch

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. kind/bug Categorizes issue or PR as related to a bug. 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-note-none Denotes a PR that doesn't merit a release note. 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.

5 participants