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.

Sign up for GitHub

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

Jump to bottom

Simplify reconciliation #208

Closed
wants to merge 4 commits into from
Closed

Simplify reconciliation #208

wants to merge 4 commits into from

Conversation

amshuman-kr
Copy link
Collaborator

@amshuman-kr amshuman-kr commented Jul 28, 2021 •
edited
Loading

How to categorize this PR?

/area control-plane
/kind technical-debt

What this PR does / why we need it:

Simplified claim logic in reconciliation by replacing selecting existing objects based on a selector with CreateOrUpdate or recreating them if necessary.

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

Special notes for your reviewer:
This PR is based on #203 and ensures that all the test cases as of #203 are still working after the simplification. So, only the commits 67df3c1 and 36a8ed6 are relevant in this PR.

This PR should be merged only after rebasing after #203 is merged.

Release note:

Simplified claim logic in reconciliation by replacing selecting existing objects based on a selector with `CreateOrUpdate` or recreating them if necessary.

@gardener-robot gardener-robot added needs/review Needs review area/control-plane Control plane related kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Jul 28, 2021
@amshuman-kr amshuman-kr changed the title Simplify reconciliation [WIP] Simplify reconciliation Jul 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 Jul 28, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 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 Jul 28, 2021
@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 Jul 28, 2021
@gardener-robot
Copy link

@shreyas-s-rao, @timuthy, @abdasgupta You have pull request review open invite, please check

@gardener-robot-ci-3 gardener-robot-ci-3 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 Aug 23, 2021
@amshuman-kr amshuman-kr changed the title [WIP] Simplify reconciliation Simplify reconciliation Aug 23, 2021
@amshuman-kr amshuman-kr marked this pull request as ready for review August 23, 2021 17:13
@amshuman-kr amshuman-kr requested a review from a team as a code owner August 23, 2021 17:13
@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 Aug 24, 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 Aug 24, 2021
@amshuman-kr
Copy link
Collaborator Author

Rebased on #203 again because it was also rebased.

@gardener-robot
Copy link

@timuthy, @abdasgupta, @shreyas-s-rao You have pull request review open invite, please check

Amshuman K R added 4 commits August 31, 2021 19:36
Migrate unit tests to use gomock.
84d6fb6
Cleanup vendoring
b99d8f1
Simplify claim logic in reconciliation.
67df3c1 
Selecting objects based on selector has been
replaced with CreateOrUpdate or recreate if necessary.
Preserve resources in statefulset.
36a8ed6
@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 Aug 31, 2021
@amshuman-kr
Copy link
Collaborator Author

Resolved conflicts and adapted test cases for #222 and rebased after v0.6.0 release.

@gardener-robot-ci-3 gardener-robot-ci-3 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 Aug 31, 2021
abdasgupta
abdasgupta reviewed Sep 13, 2021
View reviewed changes
Copy link
Contributor

@abdasgupta abdasgupta left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Greatly simplified. I think, we can remove all cron job related logics in this PR. What say?

@timuthy
Copy link
Member

timuthy commented Sep 30, 2021

/hold as we agreed to first merge #235 and adapt this PR accordingly.

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Sep 30, 2021
@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Nov 17, 2021
@gardener-robot
Copy link

@amshuman-kr You need rebase this pull request with latest master branch. Please check.

@timuthy
Copy link
Member

timuthy commented Dec 7, 2021

/close
We will gradually move to component handling as done in #262 and simplify the reconciliation (i.e. drop claiming logic) along the way. (/cc @abdasgupta @shreyas-s-rao @aaronfern @ishan16696)

@timuthy timuthy mentioned this pull request Dec 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abdasgupta abdasgupta abdasgupta left review comments

@timuthy timuthy Awaiting requested review from timuthy

@shreyas-s-rao shreyas-s-rao Awaiting requested review from shreyas-s-rao

Assignees
No one assigned
Labels
area/control-plane Control plane related kind/technical-debt Something that is only solved on the surface, but requires more (re)work to be done properly needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review needs/second-opinion Needs second review by someone else reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Milestone
No milestone
7 participants
@amshuman-kr @gardener-robot @timuthy @abdasgupta @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3