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

Improve error handling #144

Merged
merged 8 commits into from
Jul 10, 2019
Merged

Improve error handling #144

merged 8 commits into from
Jul 10, 2019

Conversation

aermakov-zalando
Copy link
Contributor

Instead of just aborting on the first encountered error, try to proceed and reconcile the remaining resources. The only thing that currently causes a hard error is UpdateFromResources(), the rest should be logged, reported as an event and ignored.

StackContainer logic has been adjusted to check other stack resources in IsReady() as well to prevent a situation where traffic is directed to a stack that can't be reconciled.

Signed-off-by: Alexey Ermakov <alexey.ermakov@zalando.de>
Signed-off-by: Alexey Ermakov <alexey.ermakov@zalando.de>
Signed-off-by: Alexey Ermakov <alexey.ermakov@zalando.de>
Signed-off-by: Alexey Ermakov <alexey.ermakov@zalando.de>
Instead of just aborting on the first encountered error, try to proceed
and reconcile the remaining resources. The only thing that currently
causes a hard error is UpdateFromResources(), the rest should be logged,
reported as an event and ignored.

StackContainer logic has been adjusted to check other stack resources in
IsReady() as well to prevent a situation where traffic is directed to a
stack that can't be reconciled.

Signed-off-by: Alexey Ermakov <alexey.ermakov@zalando.de>
@coveralls
Copy link

coveralls commented Jul 9, 2019

Pull Request Test Coverage Report for Build 997

  • 50 of 90 (55.56%) changed or added relevant lines in 6 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 77.79%

Changes Missing Coverage Covered Lines Changed/Added Lines %
controller/stackset.go 22 62 35.48%
Files with Coverage Reduction New Missed Lines %
controller/stackset.go 5 42.18%
Totals Coverage Status
Change from base Build 989: 0.04%
Covered Lines: 1408
Relevant Lines: 1810

💛 - Coveralls

This apparently doesn't work at all for HPA resources for some reason.

Signed-off-by: Alexey Ermakov <alexey.ermakov@zalando.de>
@muaazsaleem
Copy link
Contributor

The only thing that currently causes a hard error is UpdateFromResources()

Could you say why UpdateFromResources() and ReconcileStatuses() cause hard errors?

@muaazsaleem
Copy link
Contributor

It's clear why the container parameter in ReconcileStackSet(container *core.StackSetContainer) is a pointer (cuz of container.MarkExpiredStacks()), why were the parameters of other methods changed to pointers? Maybe there is a golang best practice I don't know about?

@aermakov-zalando
Copy link
Contributor Author

@muaazsaleem ReconcileStatuses() is the last call, so there's no point in trying to do anything. It also shouldn't be possible to break it permanently by a user action.

We can't really proceed if UpdateFromResources() fails because the whole state will be completely broken. Note that currently it only fails if it can't parse traffic information, which can only happen if the users deliberately corrupt the annotations. We could implement a fallback strategy for this, but I'm not sure if it's worth it.

@muaazsaleem
Copy link
Contributor

It's clear why the container parameter in ReconcileStackSet(container *core.StackSetContainer) is a pointer (cuz of container.MarkExpiredStacks()), why were the parameters of other methods changed to pointers? Maybe there is a golang best practice I don't know about?

I think my question is too generic, I'll follow up with more specific ones if I have them

@muaazsaleem
Copy link
Contributor

👍

Signed-off-by: Alexey Ermakov <alexey.ermakov@zalando.de>
@muaazsaleem
Copy link
Contributor

👍

Signed-off-by: Alexey Ermakov <alexey.ermakov@zalando.de>
@muaazsaleem
Copy link
Contributor

👍

2 similar comments
@aermakov-zalando
Copy link
Contributor Author

👍

@mikkeloscar
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants