-
Notifications
You must be signed in to change notification settings - Fork 20
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
Re-init conditions each reconcile #177
Re-init conditions each reconcile #177
Conversation
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.
lgtm
condition.InitReason, | ||
condition.InputReadyInitMessage), | ||
) | ||
instance.Status.Conditions = condition.Conditions{} |
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.
don't we want doing this conditionally, like cinder proposed https://github.com/openstack-k8s-operators/cinder-operator/pull/364/files?diff=split&w=1#diff-b57d545aef75db901bea7faa1e1e6fd8e93b88c0dc1638a8bb27258bf530c57aR169
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.
IMO this will not give us anything (exept that some conditions with status unknown will keep old LastTransitionTime) but we can change this too be consistent
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 agree it seems there is no need for nil checks
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 as far as I see instance.Status.Conditions.Init(&cl)
is smart and it updates existing conditions only if there was status change.
However instance.Status.Conditions = condition.Conditions{}
blows away the previous list of conditions in the Status. So I think I see why cinder did this only once but calls Init unconditionally.
With this PR a later reconcile:
- deletes the previous condition list
- creates a new condition list with unknown state (no last transition time)
- do the reconciliation and set condition states (conditions with updated state get a transition time)
Cinder's solution:
- keeps the previous condition list
- sets all condition state to Unknown in the list. (If the state wasn't unknown before then this updates the last transition time)
- do the reconciliation and set condition states accordingly (conditions with updated state get a transition time)
So basically both solutions make the last transition time of non Unknown conditions useless as both solution will reset that field at each reconciliation.
Also both solutions are noisy at the end as a later reconciliaton overwrites the transition time of the conditions in the API.
I will bring up loosing the last transition time issue on slack.
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.
some minor concern
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.
lgtm
3743fbf
to
bfd0ff8
Compare
/retest |
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.
API def looks good and the the implementation in sync with the rest of the services. We will improve the impl later separately
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gibizer, mrkisaolamb 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 placement-operator-build-deploy-kuttl |
c710647
into
openstack-k8s-operators:main
Jira: OSPRH-5698