-
Notifications
You must be signed in to change notification settings - Fork 191
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
Bug 1905100: Add "ingress.operator.openshift.io/hard-stop-after" annotation #522
Bug 1905100: Add "ingress.operator.openshift.io/hard-stop-after" annotation #522
Conversation
@frobware: This pull request references Bugzilla bug 1905100, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
/hold Will push some e2e tests. |
07f5f0a
to
d33069a
Compare
9bc4ee7
to
d2d7144
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.
Just some very minor suggestions Overall, it looks good.
Annotating either an ingresscontroller or the ingress config with this new annoation will redeploy the router and that will configure haproxy to emit the haproxy "hard-stop-after" global option. An ingresscontroller with a valid annotation set will override ingresses.config/cluster (if set). Examples: Annotating the ingress config: $ oc annotate ingresses.config/cluster ingress.operator.openshift.io/hard-stop-after=1h Annotating the "default" ingresscontroller: $ oc -n openshift-ingress-operator annotate ingresscontrollers/default ingress.operator.openshift.io/hard-stop-after=30m
d2d7144
to
7b7327f
Compare
/hold cancel I pushed some tests. This is ready for a review. |
8f657e8
to
af74ad7
Compare
@@ -623,6 +649,12 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController | |||
env = append(env, corev1.EnvVar{Name: RouterDisableHTTP2EnvName, Value: "true"}) | |||
} | |||
|
|||
if enabled, value := HardStopAfterIsEnabled(ci, ingressConfig); enabled { | |||
if _, err := time.ParseDuration(value); err != nil { | |||
env = append(env, corev1.EnvVar{Name: RouterHardStopAfterEnvName, Value: value}) |
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.
time.ParseDuration
can't parse things that specify days (ie 1d
). We don't expect users to be setting values that high, right? We don't expect folks to set a value using days do we? (In the past this was an issue with the route timeout annotation).
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.
Also if we fail to parse the annotation should we log 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 think the logic is inverted here: We should use value
only if time.ParseDuration(value)
returns a nil error value.
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.
Perhaps HardStopAfterIsEnabledByAnnotation
should perform the validation. What should happen if ingresses.config/cluster
has a valid annotation and then the user adds an invalid annotation to the ingresscontroller? I think the first annotation should remain in effect, but with the current logic, the invalid annotation on the ingresscontroller causes the valid annotation on the ingress.config to be ignored.
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 and added a test; though I will add an additional test that explicitly sets "" as a timeout value - just need to rearrange the test a bit now.
af74ad7
to
4386845
Compare
c16ac2f
to
3e1c30d
Compare
test/e2e/hard_stop_after_test.go
Outdated
} | ||
|
||
// Cleanup | ||
if err := clearHardStopAfterDurationForIngressConfig(t, kclient, hardStopAfterRetryTimeout, ingressConfig); err != nil { |
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.
These cleanups should probably be wrapped in a defer block and moved above any t.Fatalf
cases, right?
I think this applies to multiple test cases in this file.
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.
This doesn't hold true in terms of moving them above any t.Fatal calls. It's only at the places that say // cleanup
that you could make them into a defer block so I don't know what we would gain. I will remove the comments. If you want these to be rock solid and have no effect on any other test then the better thing to do would be to test against their own controller, config, deployment - but they will take longer to run.
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.
So if test assertions fail, then we don't need to revert the default ingress controller back to it's original state?
ie https://github.com/openshift/cluster-ingress-operator/pull/522/files#diff-62e23c6a05e53db8113412f4b1533e9f9d4479ac083164f750ded87106d4a78aR59
Just trying to understand how cleanups work for failing tests in the hard stop e2e tests.
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.
http2 tests seem to follow a similar pattern to this PR, so there's precedence. So maybe we should double check any possible defer improvements in a follow up for all e2e tests in general.
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.
defer improvements i
It's not clear to me what a defer improvement would look like. The test has failed. CI will eventually fail, so there's nothing that would aid getting a green run. Mutating the same objects in multiple tests doesn't help things.
Should defer look like:
defer func() {
if err := setHardStopAfterDurationForIngressController(t, kclient, hardStopAfterRetryTimeout, "", ic); err != nil {
t.Fatalf("failed to clear hard-stop-after on ingresscontroller: %v", err)
}
if err := waitForHardStopAfterUnsetInAllComponents(kclient, hardStopAfterRetryTimeout, ic, ingressConfig, routerDeployment); err != nil {
t.Fatalf("some component still has hard-stop-after set: %v", err)
}
}()
if err := hardStopAfterTestIngressController(t, kclient, ic, routerDeployment, (300 * time.Minute).String()); err != nil {
t.Fatalf("test assertions failed: %v", err)
}
Or?
defer func() {
if err := waitForHardStopAfterUnsetInAllComponents(kclient, hardStopAfterRetryTimeout, ic, ingressConfig, routerDeployment); err != nil {
t.Fatalf("some component still has hard-stop-after set: %v", err)
}
}()
defer func() {
if err := setHardStopAfterDurationForIngressController(t, kclient, hardStopAfterRetryTimeout, "", ic); err != nil {
t.Fatalf("failed to clear hard-stop-after on ingresscontroller: %v", err)
}
}()
if err := hardStopAfterTestIngressController(t, kclient, ic, routerDeployment, (300 * time.Minute).String()); err != nil {
t.Fatalf("test assertions failed: %v", err)
}
given that defer runs in LIFO.
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.
FTR, TIL that fatal arranges for all defers to be called. Thank you @sgreene570 .
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 the former (grouping all cleanup code into one defer block) would make the most sense (if we decide we want to add any defers at all).
/retest |
7c92728
to
f1a580d
Compare
test/e2e/hard_stop_after_test.go
Outdated
t.Fatalf("failed to clear hard-stop-after on ingresscontroller: %v", err) | ||
} | ||
|
||
// Deployment should reveet back to config value |
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: reveet -> revert
e2e-aws-operator passed |
/hold Need to redo |
f1a580d
to
31ee8ab
Compare
/hold cancel @sgreene570 I addressed #522 (comment). PTAL. |
Looks good. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware, sgreene570 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 |
|
/test e2e-aws |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@frobware: All pull requests linked via external trackers have merged: Bugzilla bug 1905100 has been moved to the MODIFIED state. In response to this:
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. |
/cherry-pick release-4.6 |
@frobware: #522 failed to apply on top of branch "release-4.6":
In response to this:
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. |
Manual backport here: #535 |
Annotating either an ingresscontroller or the ingress config with this
new annoation will redeploy the router and that will configure haproxy
to emit the haproxy "hard-stop-after" global option.
An ingresscontroller with a valid annotation set will override
ingresses.config/cluster (if set).
Examples:
Annotating the ingress config:
Annotating the "default" ingresscontroller: