Fix HealthCheck log destination, count, and size defaults #25520
Conversation
openshift-ci
bot
added
release-note
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Honny1 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
github-actions
bot
added
the
kind/api-change
Honny1
force-pushed
the
fix-hc-inf-log
branch
2 times, most recently
from
0096bde to
a2718bf
Compare
pkg/specgen/specgen.go
Outdated
| HealthLogDestination *string `json:"healthLogDestination,omitempty"` | ||
| // HealthMaxLogCount is maximum number of attempts in the HealthCheck log file. | ||
| // ('0' value means an infinite number of attempts in the log file) | ||
| HealthMaxLogCount uint `json:"healthMaxLogCount,omitempty"` | ||
| // ('0' value means an infinite number of attempts in the log file). | ||
| // Nil value means the default value (5). | ||
| HealthMaxLogCount *uint `json:"healthMaxLogCount,omitempty"` | ||
| // HealthMaxLogSize is the maximum length in characters of stored HealthCheck log | ||
| // ("0" value means an infinite log length) | ||
| HealthMaxLogSize uint `json:"healthMaxLogSize,omitempty"` | ||
| // ("0" value means an infinite log length). | ||
| // Nil value means the default value (500). | ||
| HealthMaxLogSize *uint `json:"healthMaxLogSize,omitempty"` |
There was a problem hiding this comment.
Note you cannot change them, specgen is part of our stable API via pkg/bindings.
The pointer change I mentioned is only for the libpod container config as it doesn't have to be stable.
There was a problem hiding this comment.
Can I at least remove the omitempty?
There was a problem hiding this comment.
yes omitempty should be fine to remove (and I guess is needed to make --health-max-log-count 0 work via remote, otherwise the client would not add it top the json and the server thus using the 5 as default)
There was a problem hiding this comment.
Yes, it is not serialized to httpBody, but parsing from SpecGenerator.ContainerCreateCommand is also an option. You can also add it to the httpBody manually. Which is the better way? @Luap99
There was a problem hiding this comment.
Don't parse ContainerCreateCommand, it should not be trusted for these things.
I think removing the omitempty is the best, that means we always pass what the user sets or the default. I hvae done that kind of work around in the past and it worked good enough. Now if we add containers.conf fields for these then this becomes an issue.
There was a problem hiding this comment.
Okay, that simplifies the problem. I guess it depends on what priority containers.conf should have.
Honny1
force-pushed
the
fix-hc-inf-log
branch
2 times, most recently
from
3ec3b8d to
db63dde
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| HealthMaxLogCount uint `json:"healthMaxLogCount,omitempty"` | ||
| // ('0' value means an infinite number of attempts in the log file). | ||
| // Nil value means the default value (5). | ||
| HealthMaxLogCount uint `json:"healthMaxLogCount"` |
There was a problem hiding this comment.
also likely want to add a comment that omitempty should not be added and these should be converted to a pointer on the next major release
There was a problem hiding this comment.
Do we have something like this for employee tracking for the next major release, or is it just TODO?
There was a problem hiding this comment.
My personal syntax I use is // TODO (6.0): ...
Honny1
force-pushed
the
fix-hc-inf-log
branch
3 times, most recently
from
0c1277b to
e098270
Compare
GoLang sets unset values to the default value of the type. This means that the destination of the log is an empty string and the count and size are set to 0. However, this means that size and count are unbounded, and this is not the default behavior. Fixes: containers#25473 Fixes: https://issues.redhat.com/browse/RHEL-83262 Signed-off-by: Jan Rodák <[email protected]>
This PR changes
HealthLogDestination,HealthMaxLogCount,HealthMaxLogSizeofContainerMiscConfigto pointers. And it removes omitempty fromHealthLogDestination,HealthMaxLogCount,HealthMaxLogSizeofSpecGenerator.ContainerHealthCheckConfig.GoLang sets unset values to the default value of the type. This means that the destination of the log is an empty string and the count and size are set to 0. However, this means that size and count are unbounded, and this is not the default behavior.
Fixes: #25473
Fixes: https://issues.redhat.com/browse/RHEL-83262
Does this PR introduce a user-facing change?