Look up sonobuoy master pod name by label #787
Conversation
Codecov Report
@@ Coverage Diff @@
## master #787 +/- ##
==========================================
- Coverage 42.9% 42.73% -0.18%
==========================================
Files 71 71
Lines 4165 4182 +17
==========================================
Hits 1787 1787
- Misses 2272 2289 +17
Partials 106 106
Continue to review full report at Codecov.
|
| pod, err := client.CoreV1().Pods(namespace).Get(StatusPodName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "could not retrieve sonobuoy pod") | ||
| return nil, errors.New("could not retrieve sonobuoy pod") |
There was a problem hiding this comment.
Why the change in the error? Seems like that would just obfuscate issues by not returning the actual error to the user.
There was a problem hiding this comment.
To test this error I created a second pod with the 'run:sonobuoy-master' label in the same namespace.
This is the error if we don't make this change:
$ sonobuoy status
ERRO[0001] More than one pod with label 'run:sonobuoy-master'
ERRO[0001] error attempting to run sonobuoy: could not retrieve sonobuoy pod: pods "sonobuoy" not found
This seemed a little confusing to me b/c the user will wonder "why are you looking for a pod called 'sonobuoy' - my pod has a different name?"
This happens because the SetStatusPodName function doesn't set the pod name if there's an error and it reverts back to the default name "sonobuoy". (I'm wondering now if this was wise.)
This is the error output if we do make this change:
$ sonobuoy status
ERRO[0006] More than one pod with label 'run:sonobuoy-master'
ERRO[0006] error attempting to run sonobuoy: could not retrieve sonobuoy pod
With the both errors provided together, I think this is clearer. But open to suggestions.
There was a problem hiding this comment.
Update for clarity: Due to other changes based on other review items...
There is no longer an error if there are multiple pods with the run:sonobuoy-master label. The first pod is selected (with a warning) and the status command runs.
If there are no pods with the label the output is now as follows:
$ sonobuoy status
ERRO[0001] No pods found with label 'run=sonobuoy-master' in namespace heptio-sonobuoy, using default pod name 'sonobuoy'
ERRO[0001] error attempting to run sonobuoy: could not retrieve sonobuoy pod
|
Looks great. I restarted the test run; it failed due to an upstream flakiness (known issue). If you could just rebase onto master and squash down to one commit I'll merge this once it goes green on CI. |
Currently sonobuoy always expects it's pod name to be sonobuoy. So if you run sonobuoy with a CronJob, the controller will create a pod name something like `sonobuoy-1561584180-zrwjf` and sonobuoy will fail to update it's own status. Also the sonobuoy CLI will not find the pod if you check status or try to retrieve results. This change removes the statically-set sonobuoy master pod name and looks up the pod name based on the label given to that pod: 'run=sonobuoy-master`. This assumes that this label is always set and will fail if the label is not present. If there are multiple pods with the label it will log a warning and pick the first pod in the list returned. Signed-off-by: Richard Lander <landerr@vmware.com>
lander2k2
force-pushed
the
pod-name-by-label
branch
from
41d49aa
to
3245c48
Compare
|
Done and done :). Thanks for reviewing! |
|
💯 |
What this PR does / why we need it:
Currently sonobuoy always expects it's pod name to be sonobuoy.
So if you run sonobuoy with a CronJob, the controller will create a pod name
something like
sonobuoy-1561584180-zrwjfand sonobuoy will fail to updateit's own status. Also the sonobuoy CLI will not find the pod if you
check status or try to retrieve results.
This change removes the statically-set sonobuoy master pod name and
looks up the pod name based on the label given to that pod:
'run=sonobuoy-master`.
This assumes that this label is always set and will fail if more than
one pod exists in the namespace with that label.
Which issue(s) this PR fixes
Special notes for your reviewer:
This PR is an alternative implementation to: #779
Release note:
Signed-off-by: Richard Lander landerr@vmware.com