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

Client server updates are attempted in full #422

Merged
merged 2 commits into from
Dec 20, 2024
Merged

Conversation

arussellf5
Copy link
Contributor

@arussellf5 arussellf5 commented Dec 4, 2024

Proposed changes

Client's UpdateHTTPServers and UpdateStreamServers methods now attempt to update all servers and return a combined error

Previously these methods would return as soon as an error was encountered. Now they attempt to apply all the server updates and collect all errors encountered along the way. The reasoning behind this is that we want to apply as many of the desired changes as we possibly can and don't want any transitory errors that may affect a single server update to cause all the rest of the updates to be skipped.

Use case:

I work for the NGINXaaS for Azure team and we using the plus client to apply on-the-fly upstream server updates to deployments linked with kubernetes clusters. Our aim is to react to scaling and other events on the cluster by applying the server updates to the customer's dynamic upstreams. We want to apply as many of the server updates as we can through a single call to UpdateHTTPServers() or UpdateStreamServers().

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@arussellf5 arussellf5 requested a review from a team as a code owner December 4, 2024 20:40
@github-actions github-actions bot added the tests Pull requests that update tests label Dec 4, 2024
Copy link

github-actions bot commented Dec 4, 2024

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@arussellf5
Copy link
Contributor Author

I have hereby read the F5 CLA and agree to its terms

@arussellf5 arussellf5 marked this pull request as draft December 4, 2024 21:04
…t to update all servers and return a combined error

Previously these methods would return as soon as an error was encountered. Now they attempt to apply all the server updates and collect all errors encountered along the way. The reasoning behind this is that we want to apply as many of the desired changes as we possibly can and don't want any transitory errors that may affect a single server update to cause all the rest of the updates to be skipped.
@github-actions github-actions bot removed the tests Pull requests that update tests label Dec 5, 2024
@arussellf5 arussellf5 marked this pull request as ready for review December 5, 2024 16:29
@UnwashedMeme
Copy link

This looks pretty good to me.

client/nginx.go Show resolved Hide resolved
client/nginx.go Show resolved Hide resolved
@jjngx
Copy link
Contributor

jjngx commented Dec 20, 2024

Hey @arussellf5, do all tests pass when you run make? Ok, I see now.

LGTM

Copy link
Contributor

@jjngx jjngx left a comment

Choose a reason for hiding this comment

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

🚀

Copy link

@sjberman sjberman left a comment

Choose a reason for hiding this comment

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

The one downside I could see with this is that if there's some error connecting to the API, and you're trying to update 1000 upstreams, you're going to get a massive error message.

@arussellf5
Copy link
Contributor Author

arussellf5 commented Dec 20, 2024

The one downside I could see with this is that if there's some error connecting to the API, and you're trying to update 1000 upstreams, you're going to get a massive error message.

@sjberman I agree that this situation sucks. But consider another scenario where there is a single malformed upstream server in the update list. The current behavior might prevent all other 999 servers from getting updated, because of a problem updating a single server. I think that this change will be worth it to address that scenario, even if there are some downsides.

@sjberman
Copy link

@arussellf5 That's fair.

@arussellf5
Copy link
Contributor Author

arussellf5 commented Dec 20, 2024

@jjngx @sjberman if one of you two has the privilege to merge to this repo, would you please do that, because I'm not authorized to do so? Thanks

@sjberman sjberman merged commit 753539e into nginx:main Dec 20, 2024
14 checks passed
@lucacome lucacome added the enhancement Pull requests for new features/feature enhancements label Jan 8, 2025
@lucacome lucacome mentioned this pull request Jan 9, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants