-
Notifications
You must be signed in to change notification settings - Fork 881
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
vmware: Fall back to vmtoolsd if vmware-rpctool errs #4444
vmware: Fall back to vmtoolsd if vmware-rpctool errs #4444
Conversation
80479ce
to
f9a0ba4
Compare
e306216
to
f1930b8
Compare
@TheRealFalcon I think the integration test is failing for an unrelated reason. I looked into it, and it's a wireguard module test:
|
f1930b8
to
4fb5978
Compare
Okay @TheRealFalcon , it's ready for review. Cc @PengpengSun |
4fb5978
to
cdeb997
Compare
@TheRealFalcon / @PengpengSun, I spent an hour refactoring DataSourceVMware a bit to support better unit testing around the fallback case. All tests pass. |
cdeb997
to
490a255
Compare
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.
@akutz Thanks for this change! I left some inline comments, but good change overall.
6f3ea96
to
a0386f4
Compare
Hi @TheRealFalcon, I am missing something. The tests are passing for me locally, but failing on GH actions. |
From the
Are you missing a mock somewhere? |
Quite possibly? But why would it work locally? |
It looks like it might actually exist on these runners. I don't have it locally and the test passes for me, so I think the missing binary is triggering the normal fallback, but the exit code on the runners is causing us to raise early because of the 255 exit code. |
Huh. I don't have it locally either. I tested for it being missing, not existing and returning 255. Let me think about that. |
4310dbd
to
5cfd96f
Compare
@TheRealFalcon I was able to repro it locally by adding the same command to my |
5cfd96f
to
6f907f3
Compare
6f907f3
to
fc7b92b
Compare
58a0268
to
90a3c28
Compare
This patch udpates the ds-identify script and the VMware datasource to fall back to using the vmtoolsd program if vmware-rpctool errors.
90a3c28
to
964e6ab
Compare
@TheRealFalcon I verified this works on a real system again. Please note the reason I just force pushed one more time is to add a little extra logging. It was not working on the real system, but the reason was not apparent. It ended up being because the current codebase has |
Hi @TheRealFalcon, Please let me know if there is anything else needed to resolve this PR. 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.
Thanks @akutz ! LGTM!
Addresses #4436
Proposed Commit Message
Additional Context
I validated the output and exit code for both commands:
Test Steps
Locally I verified the OVF datasource with:
Locally I verified the VMware datasource with:
Locally I verified
ds-identify
with:I also copied the modified
ds-identify
and datasources to a VM and ran the following:Everything behaved as expected.
I also replaced
/usr/bin/vmware-rpctool
with:And ran:
And
/var/log/cloud-init.log
showed the expected behavior occurred:With
/usr/bin/vmware-rpctool
still replaced, I validatedds-identify
by runningsudo rm /var/run/cloud-init/.ds-identify.result /var/run/cloud-init/ds-identify.log
and then runningsudo /usr/lib/cloud-init/ds-identify
. The resulting log fail showed the VMware datasource was still detected:Checklist: