-
Notifications
You must be signed in to change notification settings - Fork 296
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
Recalculate health check params on BC/THC removal #2192
Conversation
Hi @DamianSawicki. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
pkg/healthchecks/healthchecks.go
Outdated
@@ -218,10 +261,13 @@ func (h *HealthChecks) sync(hc *translator.HealthCheck, backendConfigHCConfig *b | |||
return "", err | |||
} | |||
|
|||
// Do not merge the existing settings and perform the full diff in calculateDiff. | |||
var recalculate bool = thcConf.THCOptInOnSvc || h.shouldRecalculateOnBCRemoval(existingHC, backendConfigHCConfig) || h.shouldRecalculateOnTHCRemoval(existingHC, thcConf.THCOptInOnSvc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be called reconfigureHC and why not using ":=" instead of "var"?
extract those ors to new function like shouldReconfigureHC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
why not using ":=" instead of "var"?
No particular reason. Is one preferred over the other?
} | ||
} | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this is commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't remember exactly, but it's possible that I realised that the bug fixed in #2181 existed while writing this. The behaviour described in the commented out test case is incorrect, and I will remove this case. Btw. now we have similar (but correct) cases in TestCustomHealthcheckRemoval
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted.
healthchecker *HealthChecks | ||
hc *translator.HealthCheck | ||
bchcc *backendconfigv1.HealthCheckConfig | ||
want bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does "want" stands for? want recalculation? please rename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does "want" stands for?
It stands for the return value of the tested function, like in the pattern got-want. Don't you think this name makes sense?
I renamed the tested functions and the test function in v2, please let me know if you still recommend renaming want
to something else.
"ILB": translator.DescriptionForDefaultILBHealthChecks, | ||
"NEG": translator.DescriptionForDefaultNEGHealthChecks, | ||
"RP": translator.DescriptionForHealthChecksFromReadinessProbe, | ||
"BC": (&healthcheck.HealthcheckInfo{HealthcheckConfig: healthcheck.BackendConfigHC}).GenerateHealthcheckDescription(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BC is taking priority over THC so THC removal should be NoOp for BC based hc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. This test is probably overly complicated, but it tests a very simple thing: that shouldRecalculateOnTHCRemoval
returns true
only for beforeKey == "THC" && afterKey == "NO-THC"
, and false
otherwise. It does not look at annotations for before, so the case you are describing is beforeKey == BC && afterKey == NO-THC
, in which case false
should be returned. It is correct.
6a1ba74
to
2844d38
Compare
/cc swetharepakula |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, waiting to @swetharepakula to look at.
pkg/healthchecks/healthchecks.go
Outdated
return false | ||
} | ||
// The existing HC is configured via Transparent Health Checks, but the annotation has been removed. | ||
if hcDesc.Config == healthcheck.TransparentHC && !thcOptIn { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can be just return hcDesc.Config == healthcheck.TransparentHC && !thcOptIn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Fixed.
return hc | ||
} | ||
gotHC := filter(*computeHCs[0]) | ||
wantHC := filter(*tc.wantComputeHC) | ||
|
||
if !reflect.DeepEqual(gotHC, wantHC) { | ||
t.Fatalf("Compute healthcheck is:\n%s, want:\n%s", pretty.Sprint(gotHC), pretty.Sprint(wantHC)) | ||
t.Errorf("Compute healthcheck is:\n%s, want:\n%s", pretty.Sprint(gotHC), pretty.Sprint(wantHC)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why changing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it during debugging to be able to see more and remembered to revert it... but then I forgot in the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
042d2f3
to
f51f667
Compare
f51f667
to
b3e3fb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits. Otherwise LGTM
@@ -57,28 +57,28 @@ func calculateDiff(old, new *translator.HealthCheck, c *backendconfigv1.HealthCh | |||
|
|||
// TODO(bowei): why don't we check Port, timeout etc. | |||
|
|||
if c == nil && !thcEnabled { | |||
if c == nil && !fullDiff { | |||
return &changes | |||
} | |||
|
|||
// This code assumes that the changes wrt to `c` has been applied to `new`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make these checks a little simpler can we not check if fullDIff is false and return early?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I also think that calculateDiff is overly complicated and these conditions in practise duplicate the functionality of mergeUserSettings
. Eventually, there should be no early returns, just comparisons old.X != new.X
. Having said that, health checks is a very sensitive issue, and a refactor here should be a separate and well-tested PR, perhaps additionally consulted. For now, I'm just renaming fullDiff -> fullDiffOnRecalculation
to make clear what we do.
return hcDesc.Config == healthcheck.TransparentHC && !thcOptIn | ||
} | ||
|
||
func (h *HealthChecks) shouldRecalculateHC(existingHC *translator.HealthCheck, backendConfigHCConfig *backendconfigv1.HealthCheckConfig, thcConf utils.THCConfiguration) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i would unmarshal the healthcheck once in here and pass it to the various validation functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/healthchecks/healthchecks.go
Outdated
@@ -183,6 +196,37 @@ func (h *HealthChecks) emitTHCEvents(hc *translator.HealthCheck, thcEvents utils | |||
|
|||
} | |||
|
|||
func (h *HealthChecks) isBackendConfigRemoved(hc *translator.HealthCheck, bchcc *backendconfigv1.HealthCheckConfig) bool { | |||
if !h.healthcheckFlags.EnableRecalculationOnBackendConfigRemoval { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: based on the name, this is a little weird that we don't consider the BackendConfig removed based on a flag.
Instead I would think we would check the flag at where this would be called, and only if enabled we would check if the backend config was removed.
Either a rename or slight change in the code to make it match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead I would think we would check the flag at where this would be called, and only if enabled we would check if the backend config was removed.
Done.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DamianSawicki, kl52752 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The purpose of the present PR is to skip the function
mergeUserSettings()
when the removal of a BackendConfig or TransparentHealthCheck annotation is detected (or the removal of a health check specification from a BackendConfig). The rationale is that by removing custom annotations, the user should expect falling back to the default (or Readiness Probe) health check parameters and invokingmergeUserSettings()
effectively preserves the custom setup.