🐛 Fixing vmtoolsd path for/on a flatcar image #2660
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @PatrickLaabs. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2660 +/- ##
==========================================
+ Coverage 64.62% 64.70% +0.08%
==========================================
Files 118 118
Lines 8580 8580
==========================================
+ Hits 5545 5552 +7
+ Misses 2602 2597 -5
+ Partials 433 431 -2 ☔ View full report in Codecov by Sentry. |
|
The change looks good to me. I'm wondering why our ignition e2e test didn't catch this before (we're testing with flatcar-stable-3602.2.3-kube-v1.29.0) Are you using image-builder or are you building your images a different way? (which is totally fine, just trying to understand the difference between our e2e tests and the error you got) |
|
Looking at the CI failure I wonder if the binary is in the PATH in the image-builder flatcar image |
|
Mhh.. weird. Edit: Here is a screenshot from inside my flatcar image, with the above mentioned version: |
|
Ok, next try. I will build another flatcar image with the version you provided and double check. |
|
Ok, now is see the problem. On a flatcar image in version stable-3602.2.3 the binary can be found here: When using flatcar in version stable-3760.2.0 the binary can be found here: @sbueringer Workaround: |
|
🤔 at best we would be compatible with both. To summarise: we have flatcar versions where
We could even pack the whole script here into a bash script and pass it via files 🤔 ExecStart=/usr/bin/bash -cv 'echo "COREOS_CUSTOM_HOSTNAME=$(/usr/bin/vmtoolsd --cmd "info-get guestinfo.metadata" | base64 -d | grep local-hostname | awk {\'print $2\'} | tr -d \'"\')" > $${OUTPUT}'Idea: a command like |
|
Ye, supporting both variants might be the better approach. I will do some testings. |
|
/retitle 🐛 Fixing vmtoolsd path for/on a flatcar image |
k8s-ci-robot
changed the title
k8s-ci-robot
added
size/S
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
size/XS
| @@ -67,7 +67,7 @@ systemd: | |||
| RemainAfterExit=yes | |||
| Environment=OUTPUT=/run/metadata/coreos | |||
| ExecStart=/usr/bin/mkdir --parent /run/metadata | |||
| ExecStart=/usr/bin/bash -cv 'echo "COREOS_CUSTOM_HOSTNAME=$(/usr/share/oem/bin/vmtoolsd --cmd "info-get guestinfo.metadata" | base64 -d | grep local-hostname | awk {\'print $2\'} | tr -d \'"\')" > $${OUTPUT}' | |||
| ExecStart=/usr/bin/bash -cv 'VMTOOLSD_PATH=$(find /usr -iname 'vmtoolsd' -executable); if [[ -n "$vmtoolsd_path" ]]; then echo "COREOS_CUSTOM_HOSTNAME=$("$VMTOOLSD_PATH" --cmd "info-get guestinfo.metadata" | base64 -d | grep local-hostname | awk {\'print $2\'} | tr -d \'"\')" > $${OUTPUT}; else echo "vmtoolsd not found in /usr/share/oem/bin/ or /usr/bin/"; fi' | |||
There was a problem hiding this comment.
Let's add a comment on why we are doing this :-) (at place X for version Y and place a for version B).
What do you think about only using if / else? The path should not change too much right?
| ExecStart=/usr/bin/bash -cv 'VMTOOLSD_PATH=$(find /usr -iname 'vmtoolsd' -executable); if [[ -n "$vmtoolsd_path" ]]; then echo "COREOS_CUSTOM_HOSTNAME=$("$VMTOOLSD_PATH" --cmd "info-get guestinfo.metadata" | base64 -d | grep local-hostname | awk {\'print $2\'} | tr -d \'"\')" > $${OUTPUT}; else echo "vmtoolsd not found in /usr/share/oem/bin/ or /usr/bin/"; fi' | |
| ExecStart=/usr/bin/bash -cv 'echo "COREOS_CUSTOM_HOSTNAME=$("$(find /usr/bin /usr/share/oem -name vmtoolsd -type f -executable 2>/dev/null | head -n 1)" --cmd "info-get guestinfo.metadata" | base64 -d | grep local-hostname | awk {\'print $2\'} | tr -d \'"\')" > $${OUTPUT}' |
There was a problem hiding this comment.
Maybe only use find to gather the command and let it fail if it fails?
we need -type f because directories can also be executable.
Also I think we don't have to search through the whole /usr directory. better to have some well-known paths to search in.
There was a problem hiding this comment.
Thanks for the suggestion!
I will try this one out. 👍
There was a problem hiding this comment.
Hey @chrischdi ,
i tried your suggestion on flatcar version 3602 and 3760.
I was able to successfully spawn the control plane and the worker nodes.
|
/retest |
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
The latest change on finding the vmtoolsd binary on the different pathes has been tested on my vsphere instance with CAPV v1.9.0 on the flatcar version '3602' and '3760'. I was able successfully spawn the controlplane and worker nodes. Lets see, what the CI has to say about it 😄 |
|
/retest |
|
@PatrickLaabs: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
|
Note: failure of |
|
/test pull-cluster-api-provider-vsphere-e2e-blocking-main |
|
The failing test "pull-cluster-api-provider-vsphere-apidiff-main" gets me curious. Did i missed something? 😱 |
|
Nope, that one is fine. It just notices us that a function signature changes. Which is fine in this case, because its only flavorgen code. (Edit: it does not impact merging / tide) |
|
/test help |
|
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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. |
|
/approve /assign sbueringer For lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrischdi 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 |
k8s-ci-robot
added
the
approved
Thanks for the clearification 👍 |
|
Looks good, thx! |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 38dd85ac1cb1898d281448bcc8a6c2577e268ddd
|
What this PR does / why we need it:
With this PR we do change the reference to the binary 'vmtoolsd'.
We removed the absolut path from
to just use the vmtoolsd binary, which is already available inside the PATH:
With this change, the kubeadm is able to do it thing and properly find the binary needed.
Without this change, the operation will fail, because the binary cannot be located at the specified path.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #2657