Skip to content

Conversation

@MartinWeindel
Copy link
Member

@MartinWeindel MartinWeindel commented Jan 10, 2023

What this PR does / why we need it:
To reduce the impact of a failed route deletion or creation operation, all route updates are executed, even if any of them fails.
In this way, new routes are added even if a deletion fails (which should not happen, but have already been seen).

It should slightly improve robustness, but will not resolve any problem if the route of a deleted node cannot be deleted and its pod CIDR is reused by a new node.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Try all route update operations on all route tables before returning errors

/cc @kon-angelo

@MartinWeindel MartinWeindel requested a review from a team as a code owner January 10, 2023 15:33
@gardener-robot gardener-robot added needs/review Needs review size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 10, 2023
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 10, 2023
Copy link
Member

@ScheererJ ScheererJ left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor nit.

@MartinWeindel MartinWeindel force-pushed the improv/update-all-before-returning-errs branch from 2d8865b to a4a297b Compare January 10, 2023 16:23
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 10, 2023
Copy link
Contributor

@kon-angelo kon-angelo left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Jan 10, 2023
@MartinWeindel MartinWeindel added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 11, 2023
@ghost ghost added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 11, 2023
@MartinWeindel MartinWeindel merged commit 51c2873 into main Jan 11, 2023
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jan 11, 2023
@MartinWeindel MartinWeindel deleted the improv/update-all-before-returning-errs branch January 11, 2023 07:56
MartinWeindel added a commit that referenced this pull request Jan 30, 2023
…g-errs

Try all route update ops before returning errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. status/closed Issue is closed (either delivered or triaged)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants