Capture instance name #116

zackmay · 2024-07-17T14:01:39Z

added a safety net if folks forget to add varnish instance name.

gquintard · 2024-07-17T16:03:52Z

@hermunn , I could use your opinion on this.
On the one hand, I like how straightforward the code is, it makes a low-effort, high probability attempt at fixing the initial command.
On the other hand, I feel the amount of magic and lack of transparency disturbing.

My knee-jerk reaction would be to run varnishadm ping (possibly with the user-provided -n argument) and warn if it fails ("is varnish running, do you have the right permissions, etc."), but that's actually less useful than the current proposal

hermunn · 2024-07-18T12:41:00Z

I think @gquintard has a good point. I think maybe a combination of the ideas would be the following:

If no -n argument is given, you check if varnishadm ping (with the -n argument) gives a good answer. If it does not, you use this PR's idea to suggest what the users should do: You need to / should consider to specify the instance using -n, and the following values seem to be reasonable: and then a list of the observed varnishd instances which have a -n argument. If there are no varnishd running, you finish this script, for the sake of other information gathering.
If there is a -n argument, but varnishadm ping -n does not work, report that and suggest other -n values that might be better, but run the rest of the script.

In short, I think that the idea of figuring out the right -n is very good, but maybe it can make even better than what is in the gather. I would also be fine with merging this or something very similar to what we have, and expand on it later.

gquintard · 2024-09-05T19:09:51Z

@zackmay, bump

…comment in github

zackmay · 2024-10-04T12:03:47Z

I believe I addressed the concerns. I've tested it on my test box, I am not sure on one piece. Which is. If someone provide the incorrect name, should we allow that which would result in the incorrect name represented in the file output. I went ahead and fixed that to use the valid name, I think that is best. Can you guy please check my work?
Thanks

varnishgather

gquintard · 2024-10-05T00:59:24Z

varnishgather

@@ -393,6 +401,10 @@ _EOF_
 	exit $1
 }

+suggest_n_values() {


this looks like an unnecessary level of indirection? If you want to use get_varnishd_instances, just use get_varnishd_instances

gquintard · 2024-10-05T01:05:39Z

varnishgather

@@ -428,6 +440,46 @@ done

 shift $((OPTIND - 1))

+if [ -z "$NAME" ]; then
+    VARNISHD_CMD=$(ps -o cmd= -p $(pgrep varnishd))


this would generate multiple values when multiple varnishd processes are running, wouldn't it?

also, you wrote get_varnishd_instances, it feels like a shame to not use it?

I changed this entire block to tighten up sloppy work on my part.

gquintard · 2024-10-05T01:09:51Z

varnishgather

+    fi
+fi
+
+if [ -n "$NAME" ] && [ -z "$RELDIR" ]; then


this block feels overkill for me, I have the impression that you just need to do

RELDIR="$RELDIR-$NEWLYFIGUREDOUTNAME" ID="$ID-$NEWLYFIGUREDOUTNAME"

if NAME wasn't specified
if

I moved this into the block if block 439-452 after thinking about it I just reused INSTANCE_NAME set from get_varnishd_instance

gquintard · 2024-10-05T01:10:37Z

varnishgather

+    if [ -n "$INSTANCE_NAME" ]; then
+        NAME="-n $INSTANCE_NAME"
+    else
+        INSTANCE_NAME=$(suggest_n_values)


why not start with that?

Covered in the previous two updates I hope.

sorry I'm old, and old dogs + new tricks. :) Co-authored-by: guillaume quintard <[email protected]>

gquintard · 2024-10-08T23:46:42Z

varnishgather

+if [ -n "$NAME" ] && ! validate_instance; then
+    INSTANCE_NAME=$(get_varnishd_instance)
+    [ -n "$INSTANCE_NAME" ] && NAME="-n $INSTANCE_NAME" || NAME=""
+fi


Pål wrote:

If there is a -n argument, but varnishadm ping -n does not work, report that and suggest other -n values that might be better, but run the rest of the script.

I don't think this code is doing that

you are right, I did not go back to your original comments I focused on fixing my laziness before updating. I'll work make sure to add a varnishadm ping check in here, my bad

after re-reading your original statement. I completely skipped over what you wanted. I focused on just ensuring the name was included in the script execution with or without the client adding it, or even if they add the wrong name it picks the proper name. A new day and a revisit to your comment Pal has me feeling you would rather have the following.

script executed, check provided -n value (if correct it goes on about it's marry way)

script executed, no -n provided but there is a name, so we spit out to the client they should rerun the script with -n $propername (exit script)
3.. script executed with -n and wrong name, return the potential proper names we gather and present that to the client with the NAME's we found with echo statements to run the script properly but then exit script.

I went about this task thinking I should just do the -n work for them. There is a potential for compromise here. I could go do the -n capture and then also, print out to the client that we had to do this because they failed to provide the information. Even warning we chose the first -n process we found, therefore if they meant X/Y/Z then they should re-run the script with the proper name?

to be completely exhaustive, I think this is the proper logic:

-n given

varnishadm ping works

proceed as usual

varnishadm ping doesn't work

warn that the name may be wrong, print out a list of candidates if any, then proceed

no -n

varnishadm ping works

proceed as usual

if there's exactly one candidate

warn that we defaulted to that auto-selected name, proceed if varnishadm ping works, error out otherwise

there is 0 candidate

warn that varnishd may not be running

there are more than 1 candidate

list the candidates, error out

@zackmay , @hermunn , how does it look?

gquintard · 2024-10-08T23:52:02Z

varnishgather

+    [ -n "$INSTANCE_NAME" ] && NAME="-n $INSTANCE_NAME" || NAME=""
+fi
+
+ID="$(cat /etc/hostname)-$(date +'%Y%m%d-%H%M%S')${INSTANCE_NAME:+-$(n_opt "$INSTANCE_NAME")}"


isn't that just?

ID="$(cat /etc/hostname)-$(date +'%Y%m%d-%H%M%S')$(n_opt "$INSTANCE_NAME")"

since n_opt will be a noop if INSTANCE_NAME is empty?

The reason I have it the way it is. When using just ID="$(cat /etc/hostname)-$(date +'%Y%m%d-%H%M%S')$(n_opt "$INSTANCE_NAME")"
the results from a named instance output look like below.
Please submit the file: varnishgather.varnish-wrk-20241101-184253varnish1.tar.gz

what I'm saying is that $(n_opt "$INSTANCE_NAME")" is a shorter equivalent to ${INSTANCE_NAME:+-$(n_opt "$INSTANCE_NAME")}"

…ents on github

zackmay · 2024-11-01T18:52:44Z

I've pushed the latest adoption to the script to address your concerns.

-n + actual name proceed and output file
-n + improper name + single instance = warning + corrected name
-n + multiple named instances = produced list and exit script
no -n captures the unnamed varnishd instance and produces resulting output file.

Tests I ran
3 varnish instances, 2 named, one unnamed, all three running

tested script with no -n captured the unnamed instance and produced the tar.gz file that instance
tested script with -n wrong name, produced list of available names
1 named and one unnamed instance running, reran test 1, then ran with -n wrong name, received warning, and output was the properly named file

To note, there was no varnishping processing in the original script, which is why I proceeded in this direction of capturing the unnamed instance as primary if -n was not provided. If -n, new logic is processed.

I hope that addresses your concerns. I also commented above on your n_opt usage.

gquintard · 2024-11-01T20:24:14Z

varnishgather

+if [ -n "$NAME" ]; then
+    INSTANCE_NAME="${NAME#-n }"
+
+    if ! echo "$INSTANCE_NAMES" | grep -qx "$INSTANCE_NAME"; then


this is sketchy because an unnamed instance can still be pinged using -n, and you can use an absolute -n to ping a relatively named instance. As long as the workdir can be fine it work.

However, I don't foresee users doing twisted things like that, this is fine

One Karnaugh map later and I realized that #116 (comment) could have been expressed more simply. If `varnishadm ping` is successful with the current NAME argument, no question asked, go for it: ``` Info: Working directory: /tmp/varnishgather.Z1LTcYva/varnishgather-flamp-20241106-214646 Info: Complete varnishadm command line deduced to: Task: 1: varnishd -V ... ``` If pinging failed, there are a few cases: If NAME was given, or if NAME wasn't given but there's no varnishd detected, issue a warning, pause a bit, then proceed: ``` Info: Working directory: /tmp/varnishgather.BBE414gf/varnishgather-flamp-20241106-221201 using: varnishadm ping, We couldn't ping varnishd, Is it running and is ./varnishgather running with the right permissions? Info: Complete varnishadm command line deduced to: <PAUSE 10s> Task: 1: varnishd -V ... ``` if no NAME was given but there's exactly one candidate, and it's pingable, override NAME, warn about it, pause then proceed: ``` Info: Working directory: /tmp/varnishgather.ZeUDcfg7/varnishgather-flamp-20241106-222151 using: varnishadm ping, We couldn't ping varnishd, Is it running and is ./varnishgather running with the right permissions? There's only one varnishd running and it's pingable. Overriding -n argument to /tmp/beepboop <PAUSE 10s> Info: Complete varnishadm command line deduced to: -n /tmp/beepboop Task: 1: varnishd -V ... ``` Finally, if there are no given NAME, and we have more than one candidate, we just propose a list of candidates and error out: ``` Info: Working directory: /tmp/varnishgather.dkGgfqHe/varnishgather-flamp-20241106-223210 using: varnishadm ping, We couldn't ping varnishd, Is it running and is ./varnishgather running with the right permissions? We've also detected one or more varnishd that could possibly be pinged with these -n arguments: -n /tmp/beepboop -n /tmp/beepboop2 <EXIT> ```

One Karnaugh map later and I realized that #116 (comment) could have been expressed more simply. If `varnishadm ping` is successful with the current NAME argument, no question asked, go for it: ``` Info: Working directory: /tmp/varnishgather.Z1LTcYva/varnishgather-flamp-20241106-214646 Info: Complete varnishadm command line deduced to: Task: 1: varnishd -V ... ``` If pinging failed, there are a few cases: If NAME was given, or if NAME wasn't given but there's no varnishd detected, issue a warning, pause a bit, then proceed: ``` Info: Working directory: /tmp/varnishgather.BBE414gf/varnishgather-flamp-20241106-221201 using: varnishadm ping, We couldn't ping varnishd, Is it running and is ./varnishgather running with the right permissions? Info: Complete varnishadm command line deduced to: <PAUSE 10s> Task: 1: varnishd -V ... ``` if no NAME was given but there's exactly one candidate, and it's pingable, override NAME, warn about it, pause then proceed: ``` Info: Working directory: /tmp/varnishgather.ZeUDcfg7/varnishgather-flamp-20241106-222151 using: varnishadm ping, We couldn't ping varnishd, Is it running and is ./varnishgather running with the right permissions? There's only one varnishd running and it's pingable. Overriding -n argument to /tmp/beepboop <PAUSE 10s> Info: Complete varnishadm command line deduced to: -n /tmp/beepboop Task: 1: varnishd -V ... ``` If no NAME is given, and there's only one candidate but it's not pingable, just error out: ``` Info: Working directory: /tmp/varnishgather.eZSa78VU/varnishgather-flamp-20241106-224856 using: varnishadm ping, We couldn't ping varnishd, Is it running and is ./varnishgather running with the right permissions? <EXIT> ``` Finally, if there are no given NAME, and we have more than one candidate, we just propose a list of candidates and error out: ``` Info: Working directory: /tmp/varnishgather.dkGgfqHe/varnishgather-flamp-20241106-223210 using: varnishadm ping, We couldn't ping varnishd, Is it running and is ./varnishgather running with the right permissions? We've also detected one or more varnishd that could possibly be pinged with these -n arguments: -n /tmp/beepboop -n /tmp/beepboop2 <EXIT> ```

zackmay added 2 commits July 10, 2024 13:58

issue #115 instance name safety net

aaf705c

modified code to tighten it up and reduce lines

db37aa2

zackmay requested a review from gquintard July 17, 2024 14:01

zackmay self-assigned this Jul 17, 2024

zackmay linked an issue Jul 17, 2024 that may be closed by this pull request

safety net for instance name #115

Open

addressing issues pointed out by G and Pal, more details provided in …

c8e0a74

…comment in github

zackmay requested a review from hermunn October 4, 2024 15:15

gquintard reviewed Oct 5, 2024

View reviewed changes

varnishgather Outdated Show resolved Hide resolved

gquintard reviewed Oct 5, 2024

View reviewed changes

zackmay and others added 2 commits October 7, 2024 05:47

Update varnishgather

4ad7164

sorry I'm old, and old dogs + new tricks. :) Co-authored-by: guillaume quintard <[email protected]>

consolidated my code into something much more streamline (I hope)

5098d43

gquintard reviewed Oct 8, 2024

View reviewed changes

rewrote the Instance name management block to address G's input, comm…

e44867e

…ents on github

gquintard reviewed Nov 1, 2024

View reviewed changes

zackmay closed this Nov 1, 2024

gquintard mentioned this pull request Nov 7, 2024

detect Varnish instances and try to be helpful #120

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capture instance name #116

Capture instance name #116

zackmay commented Jul 17, 2024

gquintard commented Jul 17, 2024

hermunn commented Jul 18, 2024

gquintard commented Sep 5, 2024

zackmay commented Oct 4, 2024 •

edited

Loading

gquintard Oct 5, 2024

gquintard Oct 5, 2024

zackmay Oct 8, 2024

gquintard Oct 5, 2024

zackmay Oct 8, 2024

gquintard Oct 5, 2024

zackmay Oct 8, 2024

gquintard Oct 8, 2024

zackmay Oct 9, 2024

zackmay Oct 9, 2024

gquintard Oct 21, 2024

gquintard Oct 8, 2024

zackmay Nov 1, 2024

gquintard Nov 1, 2024

zackmay commented Nov 1, 2024

gquintard Nov 1, 2024

Capture instance name #116

Capture instance name #116

Conversation

zackmay commented Jul 17, 2024

gquintard commented Jul 17, 2024

hermunn commented Jul 18, 2024

gquintard commented Sep 5, 2024

zackmay commented Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackmay commented Nov 1, 2024

Choose a reason for hiding this comment

zackmay commented Oct 4, 2024 •

edited

Loading