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

Trigger orphan collection on OutOfRange machine status #667

Merged

Conversation

himanshu-kun
Copy link
Contributor

@himanshu-kun himanshu-kun commented Dec 28, 2021

What this PR does / why we need it:
Till now orphan collection triggers on machine obj deletion or after 30min.
But to handle cases which arise due to eventual inconsistency in AWS , where multiple VMs can start backing a machine obj, in that case to stop machine-controller to reach the VM quota , orphan collection is now also triggered if machine object is updated with a OutOfRange error description.
The update of machine obj to OutOfRange can only happen by GetMachineStatus call , which is called only during a machine creation or deletion.

So in summary , orphan collection will be triggered in three cases now

  • during creation if multiple VM are seen
  • during deletion
  • after 30min of last logic run for each machine object.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
I haven't enqueued in the orphan queue if status contains OOR error now , but only if the machine obj status earlier didn't have OOR error and now have it.
This is to deal with scenario where machine obj has OOR error already and any other status update event triggers. So if we enqueue just on presence of OOR error ,then orphan logic will be triggered almost all the time

Release note:

orphan collection is also triggered if machine obj is updated with having multiple backing VMs

@gardener-robot gardener-robot added the needs/review Needs review label Dec 28, 2021
@gardener-robot gardener-robot added the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Dec 28, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 28, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 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 Dec 28, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 29, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 29, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 7, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 7, 2022
@himanshu-kun himanshu-kun marked this pull request as ready for review January 10, 2022 12:23
@himanshu-kun himanshu-kun requested a review from a team as a code owner January 10, 2022 12:23
@himanshu-kun
Copy link
Contributor Author

@dkistner could you provide you review as well:)

@himanshu-kun
Copy link
Contributor Author

Merging as a new release is awaited by Core colleagues

@himanshu-kun
Copy link
Contributor Author

@AxiomSamarth could you provide you approving review and merge?

Copy link
Member

@dkistner dkistner left a comment

Choose a reason for hiding this comment

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

Hi @himanshu-kun,

sorry for the delay. From my point of view this looks good.
But I would really like to have @AxiomSamarth opinion also on this.

Copy link
Contributor

@AxiomSamarth AxiomSamarth 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 11, 2022
@himanshu-kun himanshu-kun merged commit 3f73111 into gardener:master Jan 11, 2022
@himanshu-kun himanshu-kun deleted the eventual_consistency_handle branch January 11, 2022 15:43
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/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants