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

logs: select manager container when multiple exist #2844

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Jun 15, 2022

This sets the container to manager which is used by all Flux
controllers as the container name.

The other options I thought about were selecting the first, or doing
something with image detection. But both can be sensitive to either
users adding their patch as a first entry, or e.g. mirroring the image
to a different name.

Fixes #2843

@hiddeco hiddeco added bug Something isn't working area/UX labels Jun 15, 2022
@hiddeco hiddeco force-pushed the fix-logs-multiple-containers branch from 48a2aa6 to 34654dd Compare June 15, 2022 11:29
@ebourgeois
Copy link

Any unit tests?

@hiddeco
Copy link
Member Author

hiddeco commented Jun 15, 2022

@ebourgeois not within reach of the time I have available now, but could be added by someone else.

Copy link

@ebourgeois ebourgeois left a comment

Choose a reason for hiding this comment

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

Looks good for us

@darkowlzz
Copy link
Contributor

darkowlzz commented Jun 16, 2022

Tested this manually with a patched flux deployment with nginx pods. Without this change, it results in:

$ flux logs -A
✗ a container name must be specified for pod helm-controller-f6b8574b6-4zvp5, choose one of: [nginx manager]

With this change:

$ flux logs -A
2022-06-16T18:49:24.764Z info  - Metrics server is starting to listen
2022-06-16T18:49:24.764Z info  - starting manager
2022-06-16T18:49:24.764Z info  - Starting server
2022-06-16T18:49:24.764Z info  - Starting server
2022-06-16T18:49:24.870Z info HelmRelease - Starting EventSource
2022-06-16T18:49:24.870Z info HelmRelease - Starting EventSource
2022-06-16T18:49:24.870Z info HelmRelease - Starting Controller
...

Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

This sets the container to `manager` which is used by all Flux
controllers as the container name.

The other options I thought about were selecting the first, or doing
something with image detection. But both can be sensitive to either
users adding their patch as a first entry, or e.g. mirroring the image
to a different name.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco force-pushed the fix-logs-multiple-containers branch from 34654dd to d12e697 Compare June 23, 2022 11:52
@hiddeco hiddeco merged commit 769e204 into main Jun 23, 2022
@hiddeco hiddeco deleted the fix-logs-multiple-containers branch June 23, 2022 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flux Logs don't work as expected with multiple containers in Flux controller deployments
3 participants