-
Notifications
You must be signed in to change notification settings - Fork 522
fix: get-logs command collects control plane logs even if apiserver is down #3939
Conversation
to @jadarsie for final approval/merge |
checking |
cmd/get_logs.go
Outdated
@@ -362,6 +365,17 @@ func (glc *getLogsCmd) getCloudName() string { | |||
return "" | |||
} | |||
|
|||
func (glc *getLogsCmd) getControlPanelNodes() []v1.Node { |
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.
computeControlPanelNodes
may be slightly better
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.
Updated
cmd/get_logs.go
Outdated
@@ -362,6 +365,17 @@ func (glc *getLogsCmd) getCloudName() string { | |||
return "" | |||
} | |||
|
|||
func (glc *getLogsCmd) getControlPanelNodes() []v1.Node { |
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.
Could you please add a test for this method?
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.
test added
cmd/get_logs.go
Outdated
@@ -362,6 +365,17 @@ func (glc *getLogsCmd) getCloudName() string { | |||
return "" | |||
} | |||
|
|||
func (glc *getLogsCmd) getControlPanelNodes() []v1.Node { | |||
var ControlPanelNodeList []v1.Node |
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.
shorter camel-cased local variable names are more idiomatic in my opinion
ex: i
instead of nodeIndex
or node
instead of controlPlaneNode
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.
updated
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haofan-ms, jadarsie The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…s down (Azure#3939) * fix: get-logs command collects control plane logs even if apiserver is down * update * update message * update * update format * update typos
Reason for Change:
When api server is down, the
kubeClient.ListNodes()
function will not be able to get nodes. In this case we want to get control panel nodes using other configurations and collect logs of these nodes.Issue Fixed:
Credit Where Due:
Does this change contain code from or inspired by another project?
If "Yes," did you notify that project's maintainers and provide attribution?
Requirements:
Notes: