Dynamic targets and cross-task vars

[odyfey]

Hi!

I'm having a few issues while trying to run this playbook:

targets:
  default:
    hosts: [{host: "pghost1"}, {host: "pghost2"}, {host: "pghost3"}]

tasks:
  - name: check-recovery
    targets: ["default"]
    commands:
      - name: check recovery mode and set leader
        script: |
          is_recovery=$(psql -U ${pguser} -p ${pgport} -A -t -c "select pg_is_in_recovery();")
          if [ "$is_recovery" = "f" ]; then
            leader=$(hostname -I)
          fi
        register: [leader]

  - name: run on host
    targets: ["$leader"]
    commands: 
      - name: do something on leader
        script: |
          echo "doing something on $leader"

The playbook performs a check-recovery task on each host to discover the leader. Then, in the next task, some queries are executed on the leader host.

1. Is it correct to use a variable from the previous task as a dynamic target?
2. The playbook is successful only if the last element in the default targets (pghost3) is the leader, and as a result, the res.Vars variable has a value of map[leader:pghost3]. If you change the order in default targets, the last assigned value in the res.Vars map will be [leader:] and spot will error out on the last task:

[]  ! failed to dial: dial tcp :22: connect: connection refused
2024/05/24 17:36:29.424 [ERROR] can't run task "run on host" for target "": 1 error(s) occurred: [0] {failed to dial: dial tcp :22: connect: connection refused}

3. Based on this comment, I suggest that the scope of res.Vars should be limited to one task, but when using register, this variable is used to set targets in another task. Is this correct?
4. If the previous statements are correct, is it possible to change the behavior of spot so that once a non-empty value is written to map, it is not overwritten by the next empty value? I'm talking about code like this:

// changes in this line: https://github.com/umputun/spot/blob/master/pkg/runner/runner.go#L113
if rv, ok := allVars[k]; ok && rv != "" {
	continue
}

allVars[k] = v

[umputun]

You don't need to register target vars to make them available across tasks, this is done automatically. See 2695910 for more details. I'm not sure what will happen if you (re)set the empty var this way, as I have never checked this scenario, but it looks like you won't even reach this point as you set the leaders conditionaly. Probably dropping register: [leader] and changing leader=$(hostname -I) to export leader=$(hostname -I) will achieve what you want. I have not checked it myself, but I believe it will do.

Generally speaking, the idea of disallowing setting empty register variables may break some use cases completely unrelated to yours, as I think people may need to set variables to empty values for whatever reason. If we really want to address the dynamic target issue, the change likely should be in the PlayBook.UpdateTasksTargets function only, checking for empty variables and preventing them from being added to the targets slice. If you want to make a pull request for this, I will accept it.

[odyfey]

Probably dropping register: [leader] and changing leader=$(hostname -I) to export leader=$(hostname -I) will achieve what you want. I have not checked it myself, but I believe it will do.

Same result

If we really want to address the dynamic target issue, the change likely should be in the PlayBook.UpdateTasksTargets function only, checking for empty variables and preventing them from being added to the targets slice.

This won't have the desired effect because the map that is passed to PlayBook.UpdateTaskTargets is already empty.

What do you think about adding a new Targets field to the runner.ProcResp structure, which would be filled during the execution of the runner.Run function? Submitting a pull request isn't a problem, but first I'd like to align our understanding of the issue.

[umputun]

I was able to reproduce the issue with a simple local config like this

targets:
  dev:
    hosts:
      - {name: "dev1", host: "dev1.umputun.dev", port: 22, user: umputun}
      - {name: "dev2", host: "dev2.umputun.dev", port: 22, user: umputun}

  - name: pick a target
    targets: ["dev"]
    commands:
      - name: pick dev1
        script: |
          echo $(hostname)
          if [ "$(hostname)" == "dev1.umputun.dev" ]; then
            export hhh=$(hostname)
            echo "picked: $hhh"
          fi

  - name: run on dynamic target
    targets: ["$hhh"]
    commands:
      - name: show hostname
        script: |
          echo "target set to $SPOT_TARGET"
          echo "hostname is $(hostname)"

And the reason is the last line here (you can see it with --dbg):

[dev2 dev2.umputun.dev:22] script: /tmp/.spot-1905336985178479104/spot-script3262086133
[dev2 dev2.umputun.dev:22]  + #!/bin/sh
[dev2 dev2.umputun.dev:22]  + set -e
[dev2 dev2.umputun.dev:22]  + echo $(hostname)
[dev2 dev2.umputun.dev:22]  + if [ "$(hostname)" == "dev1.umputun.dev" ]; then
[dev2 dev2.umputun.dev:22]  +   export hhh=$(hostname)
[dev2 dev2.umputun.dev:22]  +   echo "picked: $hhh"
[dev2 dev2.umputun.dev:22]  + fi
[dev2 dev2.umputun.dev:22]  + echo setvar hhh=${hhh}

Essentially, the spot scans for all the exported variables and adds setvar(s) to populate them at runtime. Wrapping the generated setvar with "if non-empty" condition is probably a more direct (or at least less confusing) solution than the one proposed in the top message. However, the concern remains the same - this way, we suppress the current ability to reset variables intentionally, unless we force users to do unset in some way.

This won't have the desired effect because the map that is passed to PlayBook.UpdateTaskTargets is already empty.

You are correct about PlayBook.UpdateTaskTargets is too late as it already emptied setvar

What do you think about adding a new Targets field to the runner.ProcResp structure, which would be filled during the execution of the runner.Run function

This sounds a little odd to me, as the runtime resolution is currently handled by the script's code generation. Unless I missed the point, I'm unsure how your suggestion fits here.

The issue is real, but I don't see (yet) a backward-compatible way to address it without introducing major complexity. I don't think this should be a special case just for dynamic targets, as setting (either exporting or registering) a variable with non-dynamic targets has the same issue - the lack of understanding of whether setver part should even be generated.

[umputun]

I have created an issue from this discussion: #207

[odyfey]

And the reason is the last line here (you can see it with --dbg):

Now I understand the root cause of the issue. I observed a similar situation with the setvar setting in the logs, but I didn't give it much importance, as you mentioned earlier "people may need to set variables to empty values for whatever reason" 🙂

This sounds a little odd to me, as the runtime resolution is currently handled by the script's code generation. Unless I missed the point, I'm unsure how your suggestion fits here.

I agree, my suggestion doesn't fit the problem.

Found a workaround using cond:

  - name: check recovery mode and set leader
    script: export leader=$(hostname -I)
    cond: |         
      is_recovery=$(psql -U ${pguser} -p ${pgport} -A -t -c "select pg_is_in_recovery();")
      if [ "$is_recovery" = "f" ]; then exit 0; else exit 1; fi

It doesn't solve the issue with the correct setvar generation, but in my specific case it helped. Thank you for such a detailed review of my request.