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

Updating "Automatic" log message #184

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

mshitrit
Copy link
Member

@mshitrit mshitrit commented Jan 30, 2024

ECOPROJECT-1862

  • Considering a use case where the finalizer is placed by the agent and than we loose connectivity after reboot, missing this log message.
  • Also fixing a flaky UT in config

… than we loose connectivity after reboot, missing this log.

Signed-off-by: Michael Shitrit <[email protected]>
Copy link
Contributor

openshift-ci bot commented Jan 30, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: Michael Shitrit <[email protected]>
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea: we could put the runtime strategy into an annotation, so it's visible on the CR, and we don't need to spam the logs with it in each reconcile?

Can also be done in a follow up...

/hold

Copy link
Contributor

openshift-ci bot commented Jan 30, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mshitrit, slintes

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mshitrit
Copy link
Member Author

Idea: we could put the runtime strategy into an annotation, so it's visible on the CR, and we don't need to spam the logs with it in each reconcile?

Maybe, TBH I am not even so sure that we should even print this log to begin with.
I think it comes down to how visible we want to be with the inner working of "Automatic".

I'm not sure what's the proper balance from a user perspective between:
Automatic: I choose that because I trust you to do it best without involving me in the details
to
Automatic: here is how it works we selected X because Y

I'm also a bit worried from followup bugs (when we chance the Automatic selection/ or when run on different ocp version) such as the runtime annotation used to be X and now it's Y

@slintes
Copy link
Member

slintes commented Jan 30, 2024

Idea: we could put the runtime strategy into an annotation, so it's visible on the CR, and we don't need to spam the logs with it in each reconcile?

Maybe, TBH I am not even so sure that we should even print this log to begin with. I think it comes down to how visible we want to be with the inner working of "Automatic".

I'm not sure what's the proper balance from a user perspective between: Automatic: I choose that because I trust you to do it best without involving me in the details to Automatic: here is how it works we selected X because Y

Maybe it's fair to discuss how much info to reveal to the user. But we also need it ourself for debugging, not?

I'm also a bit worried from followup bugs (when we chance the Automatic selection/ or when run on different ocp version) such as the runtime annotation used to be X and now it's Y

Actually I see this as an pro argument for the annotation. In case SNR or the cluster updates during remediation, it would be bad to switch from one strategy to another one, not? The cleanup would not match anymore. With the annotation we can ensure that we use the same strategy for the whole remediation process.

@mshitrit
Copy link
Member Author

/test 4.14-openshift-e2e

@mshitrit
Copy link
Member Author

Maybe it's fair to discuss how much info to reveal to the user. But we also need it ourself for debugging, not?

Sure SGTM.
In any case I consider logs as something less visible than annotations/events and targeted for more technical users.

it would be bad to switch from one strategy to another one, not?

I'm not sure.
In case the remediation still exist in this use case there is a very good chance that we weren't able to fix it for awhile, might not be such a bad idea to try with a different strategy - it might even be the purpose of a change in the Automatic strategy.

@mshitrit mshitrit changed the title [WIP] Updating "Automatic" log message Updating "Automatic" log message Jan 31, 2024
@mshitrit mshitrit marked this pull request as ready for review January 31, 2024 09:04
@openshift-ci openshift-ci bot requested review from clobrano and razo7 January 31, 2024 09:04
@mshitrit
Copy link
Member Author

/test 4.13-test

@mshitrit
Copy link
Member Author

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit f38da78 into medik8s:main Jan 31, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants