KPO write_logs does not need to use complicated pod logs reader#36228
KPO write_logs does not need to use complicated pod logs reader#36228dstandish wants to merge 1 commit into
Conversation
dstandish
requested review from
hussein-awala and
jedcunningham
as code owners
boring-cyborg
Bot
added
area:providers
provider:cncf-kubernetes
There was a problem hiding this comment.
is this considered a breaking change?
There was a problem hiding this comment.
I don't think we need to consider it breaking. This is a method that realistically no one would ever override. It's used in one place. I think of it as "bugfix" that it wasn't marked private.
Really this also connects to something we've discussed before. That our operators should not be published as "base classes" to be extended. I think they should be end products whose behavior is governed by semver but not whose structure is governed by semver. Personally I think we ought to update our "public interface" doc to reflect this.
There was a problem hiding this comment.
Checked the discussion in #36767, seems about right to me that no one "should" ever override the write_logs method, it is something that should not be changed. Seems like a non breaking change
dstandish
force-pushed
the
simplify-write-logs-logic
branch
from
886123e to
e26679d
Compare
amoghrajesh
left a comment
amoghrajesh
left a comment
There was a problem hiding this comment.
Looks good, one nit/ask, rest is fine 👍🏽
There was a problem hiding this comment.
Checked the discussion in #36767, seems about right to me that no one "should" ever override the write_logs method, it is something that should not be changed. Seems like a non breaking change
| "Set log level to DEBUG for traceback.", | ||
| e, | ||
| ) | ||
| self.log.warning("Reading of logs interrupted with error %r;", e) |
There was a problem hiding this comment.
Does it retry? We should mention in the warning in that case.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
github-actions
Bot
added
the
stale
3 participants
Method
write_logswas using pod manager methodread_pod_logswhich has some rather complicated logic that it doesn't need, because it does not "follow" the logs and because by the time we come out of deferral, the pod is already done.By reducing usage of
read_pod_logs, it makes it easier to hopefully refactor and simplify some of our KPO log fetching logic.I also make the metheod explicitly private which it should have been anyway.