-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add reason to AgentVersionNotSupported condition #7811
Add reason to AgentVersionNotSupported condition #7811
Conversation
Hi @raspbeep. Thanks for your PR. I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
7759202
to
3cd564e
Compare
pkg/virt-handler/vm.go
Outdated
return false | ||
basicCommandsUnsupportedReason := "This guest agent doesn't support required basic commands" | ||
log.Log.V(3).Object(vmi).Info(basicCommandsUnsupportedReason) | ||
return false, basicCommandsUnsupportedReason |
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 we could always return the reason and we could log it in the place of the caller. What do you think?
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.
Moved it to the caller function, thanks.
3cd564e
to
6db1334
Compare
/assign @vladikr |
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.
/approve
/cc @fossedihelm
pkg/virt-handler/vm_test.go
Outdated
Expect(result).To(BeTrue()) | ||
Expect(reason).To(Equal("")) |
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.
nit: You can use BeEmpty
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xpivarc 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 |
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.
Thanks @raspbeep. Looks good overall! Some comments below.
|
||
// For current versions, virt-launcher's supported commands will always contain data. | ||
// For backwards compatibility: during upgrade from a previous version of KubeVirt, | ||
// virt-launcher might not provide any supported commands. If the list of supported | ||
// commands is empty, fall back to previous behavior. | ||
if len(guestInfo.SupportedCommands) > 0 { | ||
supported = isGuestAgentSupported(vmi, guestInfo.SupportedCommands) | ||
supported, reason = isGuestAgentSupported(vmi, guestInfo.SupportedCommands) | ||
log.Log.V(3).Object(vmi).Info(reason) |
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 am not sure about moving the log here. I like that is the function itself that prints out the output of its computation, also for future uses; if some other part will use that function it must log the reason too. You can always get the returned value and using it in the VirtualMachineInstanceCondition
. Also, you can return the reason only and only if the command is not supported. WDYT?
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.
Sorry, I only noticed now that my comment is the opposite of #7811 (comment)
Feel free to adopt the solution you prefer, thanks!
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 went for the the version suggested by @xpivarc. Thanks.
pkg/virt-handler/vm_test.go
Outdated
Expect(result).To(BeTrue()) | ||
Expect(reason).To(Equal("")) |
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.
IIUC, currently, isGuestAgentSupported
should return "This guest agent is supported" when the result
is true
, so I think that this should not work. Or maybe am I missing something?
This is valid for all the occurrences below. Thanks
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.
You are right, fixed. Thanks.
6db1334
to
f916c2c
Compare
Also, deleted some forgotten Println in |
pkg/virt-handler/vm.go
Outdated
} | ||
|
||
log.Log.V(3).Object(vmi).Info("This guest agent is supported") | ||
return true | ||
return true, "" |
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.
Having moved the Log outside of this function you have to return This guest agent is supported
, otherwise you will lose a log. I think that here you have 2 ways:
- leave this return as is, but you have to restore the log inside this function
- return
This guest agent is supported
but you have to adjust the tests properly
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.
Fixed, thanks. I chose to always return some reason even if it is This guest agent is supported
and adjusted the tests.
Signed-off-by: Pavel Kratochvil <pakratoc@redhat.com>
f916c2c
to
28ef53a
Compare
/ok-to-test |
/lgtm |
/retest |
@raspbeep: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest-required |
4 similar comments
/retest-required |
/retest-required |
/retest-required |
/retest-required |
/cherry-pick release-0.53 |
@xpivarc: new pull request created: #7911 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-0.49 |
@fossedihelm: new pull request created: #7912 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
Add reason to AgentVersionNotSupported condition.
Which issue(s) this PR fixes:
Fixes #7433
Release note: