This repository has been archived by the owner on May 3, 2022. It is now read-only.
ReleaseController application picks the tip of the release chain as contender #166
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@kanatohodets @juliogreff Hey folks! Mind dropping a line on this one as we all had concerns regarding this change, it would be great to keep it tracked. Thanks! |
ksurent
previously requested changes
Aug 30, 2019
juliogreff
reviewed
Sep 2, 2019
|
My original worry on this change was actually unfounded (I took a code comment to its word, but that has since been addressed). I'm actually quite comfortable with the spirit of the change now that my misunderstanding has been cleared up. There is a quite unlikely scenario that might be worth considering though. Imagine that a healthy release Honestly I can't think of a good way around this (maybe preventing new releases when there's an incomplete contender?), so this is just putting it out there. As I said earlier, the idea behind the current state of this PR is sound to me now. |
osdrv
pushed a commit
that referenced
this pull request
Sep 3, 2019
This change addresses the concern raised in #166 (comment) and enforces release equality check by their name, not pointer-based. Signed-off-by: Oleg Sidorov <[email protected]>
juliogreff
reviewed
Sep 3, 2019
isutton
reviewed
Sep 4, 2019
osdrv
force-pushed
the
olegs/fix-get-release-pair
branch
from
416cb0e to
6f30009
Compare
osdrv
force-pushed
the
olegs/fix-get-release-pair
branch
3 times, most recently
from
e8bec08 to
a580c1a
Compare
isutton
reviewed
Sep 5, 2019
osdrv
force-pushed
the
olegs/fix-get-release-pair
branch
2 times, most recently
from
3897928 to
66b122d
Compare
juliogreff
reviewed
Sep 12, 2019
juliogreff
reviewed
Sep 12, 2019
juliogreff
previously approved these changes
Sep 12, 2019
osdrv
force-pushed
the
olegs/fix-get-release-pair
branch
from
66b122d to
d1d5d79
Compare
The current implementation has a slightly relaxed algorithm of finding contender and incumbent in the chain of releases: in particular, the latest (sorted by generation) scheduled release is identified as a contender and the latest complete one is our incumbent. This commit changes the approach and forces release controller to start using releaseutil methods for finding incumbent and contender. The incumbent finding is still very naive as it's relying on release status to figure out whether it's complete or not. On the improvement side, we got rid of using the most dangerous condition in release object: scheduled criteria. From now on, we always pick the tip of the release chain. Some extra fixes in release controller and chart repo: * Chart repo returns ChartFetchFailureError if fetch failed * Release controller: moved away from 2-step cluster selection and scheduling: 1 pass now * ChartRepoInternalError is a recognised error now * Application utils: incumbent equality to contender is name-based Signed-off-by: Oleg Sidorov <[email protected]>
osdrv
force-pushed
the
olegs/fix-get-release-pair
branch
from
d1d5d79 to
3ef70e6
Compare
juliogreff
approved these changes
Sep 12, 2019
ghost
approved these changes
Sep 13, 2019
juliogreff
added a commit
that referenced
this pull request
Oct 1, 2019
This may or may not make conceptual sense (it does to me, but I could not find out why choosing clusters and scheduling a release was two separate steps in the first place, although after #166[1] I'm fairly convinced this was just a technical artifact), but it sure is convenient: we move all the error handling during the scheduling step to a single chunk of code. This fixes an issue where errors in ChooseClusters were not reflected in any condition, making the Release object not change during the sync, and therefore not triggering any events, being essentially invisible to users. As a bonus, I restored the actual testing part of this in the unit tests. We were previously just checking that ChooseClusters didn't trigger any updates, without actually checking if it was doing the right thing (choosing clusters). [1] https://github.com/bookingcom/shipper/pull/166/files#diff-caffe52421149f1f8d77a0e7c749867dR327-R341
juliogreff
added a commit
that referenced
this pull request
Oct 1, 2019
This may or may not make conceptual sense (it does to me, but I could not find out why choosing clusters and scheduling a release was two separate steps in the first place, although after #166[1] I'm fairly convinced this was just a technical artifact), but it sure is convenient: we move all the error handling during the scheduling step to a single chunk of code. This fixes an issue where errors in ChooseClusters were not reflected in any condition, making the Release object not change during the sync, and therefore not triggering any events, being essentially invisible to users. As a bonus, I restored the actual testing part of this in the unit tests. We were previously just checking that ChooseClusters didn't trigger any updates, without actually checking if it was doing the right thing (choosing clusters). [1] https://github.com/bookingcom/shipper/pull/166/files#diff-caffe52421149f1f8d77a0e7c749867dR327-R341
juliogreff
added a commit
that referenced
this pull request
Oct 4, 2019
This may or may not make conceptual sense (it does to me, but I could not find out why choosing clusters and scheduling a release was two separate steps in the first place, although after #166[1] I'm fairly convinced this was just a technical artifact), but it sure is convenient: we move all the error handling during the scheduling step to a single chunk of code. This fixes an issue where errors in ChooseClusters were not reflected in any condition, making the Release object not change during the sync, and therefore not triggering any events, being essentially invisible to users. As a bonus, I restored the actual testing part of this in the unit tests. We were previously just checking that ChooseClusters didn't trigger any updates, without actually checking if it was doing the right thing (choosing clusters). [1] https://github.com/bookingcom/shipper/pull/166/files#diff-caffe52421149f1f8d77a0e7c749867dR327-R341
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementation has a slightly relaxed algorithm of finding
contender and incumbent in the chain of releases: in particular, the
latest (sorted by generation) scheduled release is identified as a
contender and the latest complete one is our incumbent.
This commit changes the approach and forces release controller to start
using releaseutil methods for finding incumbent and contender. The
incumbent finding is still very naive as it's relying on release status
to figure out whether it's complete or not. On the improvement side, we
got rid of using the most dangerous condition in release object:
scheduled criteria. From now on, we always pick the tip of the release
chain.
Signed-off-by: Oleg Sidorov [email protected]