-
Notifications
You must be signed in to change notification settings - Fork 170
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
[ARO-5368] Try and log the VM info + console log on failure #3629
Conversation
/azp run ci, e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
#3630 splits out the log changes |
"github.com/Azure/ARO-RP/pkg/util/stringutils" | ||
) | ||
|
||
func (m *manager) LogAzureInformation(ctx context.Context) (interface{}, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want some doc comments here about what this func does and what information it gets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
pkg/cluster/gatherlogs.go
Outdated
{f: m.logNodes, isJSON: true}, | ||
{f: m.logClusterOperators, isJSON: true}, | ||
{f: m.logIngressControllers, isJSON: true}, | ||
{f: d.LogAzureInformation, isJSON: false}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about not adding LogAzureInformation
in gatherFailureLogs
but adding a new method?
It would be simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feedback doesn't make a lot of sense to me. What's the need for adding another function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean because we added d.LogAzureInformation
in gatherFailureLogs
, we need isJSON
field and some conditional branches in here.
If we make a new method instead of gatherFailureLogs
, and call d.LogAzureInformation
there not in gatherFailureLogs
, we can keep this method simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the existing process of dumping out YAML/JSON is a bit unfriendly for SREs, especially when it comes to the ClusterOperators output. I think it would be better to move some of these to more formatted output, as well as have the ability to automatically analyse the logs (e.g. the serial console logs) and output formatted results there. I'm not sure if wrapping this func in another will make it overall more simple, but I do think another pass at this whole area would be a welcome change in a separate PR.
Please rebase pull request. |
80fa07b
to
00ca0d7
Compare
d65328a
to
c2e5526
Compare
/azp run ci, e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
c2e5526
to
a91be7c
Compare
/azp run ci, e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run e2e |
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes: issues.redhat.com/browse/ARO-5368
Dumps the VM info + console logs on failure so that we don't need to run the Geneva Action or have the control plane still around to get it.
See #3287