buildSmbMetricsContainer: pass --port to smbmetrics container#371
Open
BenEstrabaud wants to merge 1 commit intosamba-in-kubernetes:masterfrom
Open
buildSmbMetricsContainer: pass --port to smbmetrics container#371BenEstrabaud wants to merge 1 commit intosamba-in-kubernetes:masterfrom
BenEstrabaud wants to merge 1 commit intosamba-in-kubernetes:masterfrom
Conversation
This fix is about buildSmbMetricsContainer in internal/resources/metrics.go: when the smbmetrics sidecar container is enabled, the samba share pod always crashes (CrashLoopBackoff). buildSmbMetricsContainer() configures liveness and readiness probes on port 8080 but does not pass --port to the smbmetrics binary, which defaults to port 9922. The probes therefore always fail, putting the samba-metrics sidecar into a CrashLoopBackOff immediately after startup. Fixed by passing --port= as an argument so the binary listens on the same port the probes expect. Signed-off-by: Ben Estrabaud <benestrabaud@gmail.com>
Collaborator
|
The CI checks are rejecting long lines in the commit description. In addition there was a failure due to an older Go version in the dockerfile. I have filed #372 to fix that issue. |
synarete
reviewed
Feb 22, 2026
Collaborator
synarete
left a comment
synarete
left a comment
There was a problem hiding this comment.
The fix itself is in-place but the commit message does not follow the projects conversions:
- Split long lines
- Single white-line between commit title and body
- Title should be shorter and correlate to actual change. Something like: "resources: pass '--port' option to smbmetrics container"
Comment on lines
+16
to
+23
| t.Run("name", func(t *testing.T) { | ||
| assert.Equal(t, "samba-metrics", ctr.Name) | ||
| }) | ||
|
|
||
| t.Run("image", func(t *testing.T) { | ||
| assert.Equal(t, "quay.io/samba.org/samba-metrics:latest", ctr.Image) | ||
| }) | ||
|
|
Collaborator
There was a problem hiding this comment.
Using t.Run() for such checks is too much. A simple assert.Equal is good enough here.
Collaborator
|
@BenEstrabaud While addressing the review comments, please ensure that the branch is rebased onto the latest master to incorporate the fix from #372. |
Author
|
No problem, I won't be able to do it for a few days but I'll incorporate all the suggestions when I get to it. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
This fix is about buildSmbMetricsContainer in internal/resources/metrics.go: when the smbmetrics sidecar container is enabled, the samba share pod always crashes (CrashLoopBackoff).
buildSmbMetricsContainer() configures liveness and readiness probes on port 8080 but does not pass --port to the smbmetrics binary, which defaults to port 9922. The probes therefore always fail, putting the samba-metrics sidecar into a CrashLoopBackOff immediately after startup.
Fixed by passing --port= as an argument so the binary listens on the same port the probes expect.
This fix was tested on a live k3s cluster: