availableupdates: do not reset lastTransitionTime on unchanged status #964
Conversation
This file contains hidden or 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
openshift-ci
bot
added
the
approved
petr-muller
force-pushed
the
do-not-reset-last-transition-time
branch
from
f3d24ee to
db205e7
Compare
Member
Author
|
/retest |
This was referenced Sep 7, 2023
Member
Author
|
/test e2e-agnostic-ovn-upgrade-out-of-change |
DavidHurta
suggested changes
Sep 7, 2023
Contributor
DavidHurta
left a comment
DavidHurta
left a comment
There was a problem hiding this comment.
Overall looks good and functional!
My only concern is the used comparison when searching for an update in updates.
Other than that only nitpicks.
(Note: Only tested locally using the go test subcommand)
Member
Author
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
petr-muller
force-pushed
the
do-not-reset-last-transition-time
branch
from
db205e7 to
5606ae9
Compare
Member
Author
openshift-ci
bot
removed
the
do-not-merge/hold
DavidHurta
approved these changes
Sep 8, 2023
openshift-ci
bot
added
do-not-merge/hold
DavidHurta
reviewed
Sep 8, 2023
The code in `evaluateConditionalUpdates` correctly uses `SetStatusCondition` to set conditions, which only updates the `LastTransitionTime` field when `Status` differs between the original and updated state. Previously though, the original state always contained empty conditions, because conditional updates are always obtained from OSUS and the fresh structure was never updated with existing conditions from the in-cluster status.
petr-muller
force-pushed
the
do-not-reset-last-transition-time
branch
from
5606ae9 to
0375904
Compare
Contributor
|
/lgtm |
Contributor
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
DavidHurta
suggested changes
Sep 9, 2023
Contributor
DavidHurta
left a comment
DavidHurta
left a comment
There was a problem hiding this comment.
I apologize for the late re-review after accepting the changes.
/hold
Comment on lines
170
to
179
| Type: "PromQL", | ||
| PromQL: &configv1.PromQLClusterCondition{PromQL: string(evalToYes)}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Conditions: []metav1.Condition{ | ||
| { | ||
| Type: "Recommended", | ||
| Status: metav1.ConditionFalse, |
Contributor
There was a problem hiding this comment.
Here we hard-code the field Type to the value of "PromQL", the field PromQL to the value of &configv1.PromQLClusterCondition{PromQL: string(evalToYes)}, and the field Status to the value of metav1.ConditionFalse even though they should be set by the function's parameters ruleType, promql, and by the expected value for the Status depending on the Match result.
Member
There was a problem hiding this comment.
I think Petr has things the way he does so he can excercise "what we already know about a conditional update for a particular target, and the new update service response comes in with new information for that same image?". So maybe we move from PromQL to Always like openshift/cincinnati-graph-data#3590. The test suite should confirm that lastTransitionTime is being preserved if we transition from "outgoing PromQL matched and so does the incoming Always" and that the transition time resets if we transition from "outgoing PromQL did not match but the incoming Always does".
Contributor
There was a problem hiding this comment.
I think Petr has things the way he does so he can excercise "what we already know about a conditional update for a particular target, and the new update service response comes in with new information for that same image?".
Oh, I didn't think of it like that, good point! Any of the options seems reasonable.
Member
Author
There was a problem hiding this comment.
Unfortunately I cannot claim the intent that Trevor assumes. The truth is that I finished the testing code knowing it is probably a bit rough but I called it day thinking it's not worth the effort to polish it entirely. David is right that this method pretends to be a reusable configurable helper that can prepare fixtures for different situations but in reality it does not. I'll spend some time looking at how I could clean it up even further, maybe taking other Trevor's review items into account as well.
Thanks for being thorough!
Member
Author
There was a problem hiding this comment.
I reworked the tests using the existing mock that Trevor suggested, which made some of the code above obsolete. I have dropped osusWithSingleConditionalEdge's illusion of being reusable, hardcoded its content (the value of this helper is that is it creates the complicated but consistent fixture data). When someone wants to add tests they can generalize the code following their future use case.
openshift-ci
bot
added
do-not-merge/hold
wking
reviewed
Sep 9, 2023
wking
reviewed
Sep 9, 2023
| } | ||
|
|
||
| func osusWithSingleConditionalEdge(from, to string, ruleType clusterConditionRuleType, promql clusterConditionRuleFakePromql) ([]configv1.ConditionalUpdate, *httptest.Server) { | ||
| osus := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
Member
There was a problem hiding this comment.
nit: we could possibly simplify this test harness a bit if we pull this out into a new function, because the upstream defaulting, available-updates throttling, and proxy transport lookup don't seem as interesting to cover in tests. You could create a dummy client to pass through to calculateAvailableUpdatesStatus like:
type MockRoundTripper func(r *http.Request) *http.Response
func (f MockRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
return f(r), nil
}and:
httpClient := &http.Client{
Transport: MockRoundTripper(func(r *http.Request) *http.Response {
return &http.Response{
StatusCode: http.StatusOK,
Body: io.NopCloser(strings.NewReader(fmt.Sprintf(`{
"nodes": [{"version": "%s", "payload": "payload/%s"}, {"version": "%s", "payload": "payload/%s"}],
"conditionalEdges": [
{
"edges": [{"from": "%s", "to": "%s"}],
"risks": [
{
"url": "https://example.com/%s",
"name": "FourFiveSix",
"message": "Four Five Five is just fine",
"matchingRules": [{"type": "%s", "promql": { "promql": "%s"}}]
}
]
}
]
}
`, from, from, to, to, from, to, to, ruleType, promql
))),
},
},
}or some such without needing to set up an HTTP server that needs to be closed later.
I'm not sure if it's worth the effort to pivot, now that you've already figured out all the plumbing to get this up to the existing level, but it might be worth poking at to see if the pivoting-this-pull cost seems like it might be worth reducing the onboarding-the-next-dev-who-needs-to-understand-this-test-suite cost.
Member
Author
There was a problem hiding this comment.
I have considered doing something like that, but the tested functionality felt like deserving a higher-level integration test so I thought test HTTP servers are cheap in Go with httptest. The real complexity is in preparing all the data and expected fixtures.
But I'll reconsider, with further development and feedback the code now is different than when started. Thanks for the suggestion!
Member
Author
There was a problem hiding this comment.
I gave the pivot a quick shot and I think we're not really saving that much complexity, the annoying bits with "set up the input/output" would need to stay, we'd just need different plumbing.
But I actually discovered that I like using httptest here. It allows testing the full method and it quite explicitly communicates that the important input comes from a server. The plumbing needed is basically identical to mocking it in the client transport. I liked that I could get rid of the annoying queue stub though :)
Member
Author
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
wking
reviewed
Sep 11, 2023
Member
Author
looks unrelated |
Member
Author
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
Member
Author
Looks unrelated, I'll override next unrelated failure :P |
Member
Author
/override ci/prow/e2e-agnostic-ovn-upgrade-into-change |
Contributor
|
@petr-muller: Overrode contexts on behalf of petr-muller: ci/prow/e2e-agnostic-ovn-upgrade-into-change DetailsIn 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. |
Contributor
|
Pre-merge test using the dummy cincy json: Prior to the change, the // The lastTransitionTime of 3 conditionalUpdates was // After 5 minutes, the lastTransitionTime of the 3 conditionalUpdates was changed to // After 5 minutes, the lastTransitionTime of the 3 conditionalUpdates was changed to After the change, // The // After around 10 minutes, the status of the last condition was changed from // The The test result looks good to me. Petr, please let me know if there is additional test against it needed |
Member
Author
|
@shellyyang1989 this looks good, thanks for testing! |
Contributor
|
Thanks for confirming, then /label qe-approved |
openshift-ci
bot
added
the
qe-approved
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: petr-muller, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Contributor
Contributor
|
@petr-muller: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
wking
pushed a commit
to wking/cluster-version-operator
that referenced
this pull request
Sep 25, 2023
…openshift#964) * availableupdates: do not reset lastTransitionTime on unchanged status The code in `evaluateConditionalUpdates` correctly uses `SetStatusCondition` to set conditions, which only updates the `LastTransitionTime` field when `Status` differs between the original and updated state. Previously though, the original state always contained empty conditions, because conditional updates are always obtained from OSUS and the fresh structure was never updated with existing conditions from the in-cluster status. * review: use existing mock condition instead of new code * review: use real queue instead of a mock
openshift-cherrypick-robot
pushed a commit
to openshift-cherrypick-robot/cluster-version-operator
that referenced
this pull request
Sep 27, 2023
…openshift#964) * availableupdates: do not reset lastTransitionTime on unchanged status The code in `evaluateConditionalUpdates` correctly uses `SetStatusCondition` to set conditions, which only updates the `LastTransitionTime` field when `Status` differs between the original and updated state. Previously though, the original state always contained empty conditions, because conditional updates are always obtained from OSUS and the fresh structure was never updated with existing conditions from the in-cluster status. * review: use existing mock condition instead of new code * review: use real queue instead of a mock
wking
pushed a commit
to wking/cluster-version-operator
that referenced
this pull request
Oct 25, 2023
…openshift#964) * availableupdates: do not reset lastTransitionTime on unchanged status The code in `evaluateConditionalUpdates` correctly uses `SetStatusCondition` to set conditions, which only updates the `LastTransitionTime` field when `Status` differs between the original and updated state. Previously though, the original state always contained empty conditions, because conditional updates are always obtained from OSUS and the fresh structure was never updated with existing conditions from the in-cluster status. * review: use existing mock condition instead of new code * review: use real queue instead of a mock
wking
pushed a commit
to wking/cluster-version-operator
that referenced
this pull request
Nov 2, 2023
…openshift#964) * availableupdates: do not reset lastTransitionTime on unchanged status The code in `evaluateConditionalUpdates` correctly uses `SetStatusCondition` to set conditions, which only updates the `LastTransitionTime` field when `Status` differs between the original and updated state. Previously though, the original state always contained empty conditions, because conditional updates are always obtained from OSUS and the fresh structure was never updated with existing conditions from the in-cluster status. * review: use existing mock condition instead of new code * review: use real queue instead of a mock
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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 code in
evaluateConditionalUpdatescorrectly usesSetStatusConditionto set conditions, which only updates theLastTransitionTimefield whenStatusdiffers between the original and updated state. Previously though, the original state always contained empty conditions, because conditional updates are always obtained from OSUS and the fresh structure was never updated with existing conditions from the in-cluster status.