Skip to content
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.

Sign up for GitHub

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

Jump to bottom

plan diff missing arguments for script checks #10564

Closed
tgross opened this issue May 11, 2021 · 4 comments
Closed

plan diff missing arguments for script checks #10564

tgross opened this issue May 11, 2021 · 4 comments
Labels
theme/plan type/bug

Comments

@tgross
Copy link
Member

tgross commented May 11, 2021

The nomad plan command doesn't include the diff for the arguments for script checks, found at group.service.check.args. I discovered this while doing some testing for liveness/readiness checks, using the following job spec:

jobspec 
job "example" {
  datacenters = ["dc1"]

  group "web" {

    network {
      mode = "bridge"
      port "www" {
        to = 8001
      }
    }

    service {
      name = "leader"
      port = "www"

      check {
        name      = "readiness-leader"
        type      = "script"
        interval  = "10s"
        timeout   = "5s"
        task      = "httpd"
        on_update = "ignore"
        command = "/bin/sh"
        args = [
          "-c",
          "echo \"leader check: $NOMAD_ALLOC_INDEX\"; exit $NOMAD_ALLOC_INDEX",
        ]
      }
    }

    task "httpd" {
      driver = "docker"

      config {
        image   = "busybox:1"
        command = "httpd"
        args    = ["-v", "-f", "-p", "8001", "-h", "/local"]
        ports   = ["www"]
      }

      template {
        data        = "<html>hello, world</html>"
        destination = "local/index.html"
      }

      resources {
        cpu    = 128
        memory = 128
      }
    }
  }
}

If I run that jobspec and then modify it as follows:

<           "echo \"leader check: $NOMAD_ALLOC_INDEX\"; exit $NOMAD_ALLOC_INDEX",
---
>           "echo \"leader check: $NOMAD_ALLOC_INDEX\"; exit 1",

Then the resulting plan will show that there is a diff to the check, but not show the contents of that diff:

$ nomad plan ./example.nomad
+/- Job: "example"
+/- Task Group: "web" (1 in-place update)
  +/- Service {
        AddressMode:       "auto"
        EnableTagOverride: "false"
        Name:              "leader"
        Namespace:         "default"
        OnUpdate:          "require_healthy"
        PortLabel:         "www"
        TaskName:          ""
    +/- Check {
      AddressMode:            ""
      Body:                   ""
      Command:                "/bin/sh"
      Expose:                 "false"
      FailuresBeforeCritical: "0"
      GRPCService:            ""
      GRPCUseTLS:             "false"
      InitialStatus:          ""
      Interval:               "10000000000"
      Method:                 ""
      Name:                   "readiness-leader"
      OnUpdate:               "ignore"
      Path:                   ""
      PortLabel:              ""
      Protocol:               ""
      SuccessBeforePassing:   "0"
      TLSSkipVerify:          "false"
      TaskName:               "httpd"
      Timeout:                "5000000000"
      Type:                   "script"
        }
      }
      Task: "httpd"

Scheduler dry-run:
- All tasks successfully allocated.

Job Modify Index: 10
To submit the job with version verification run:

nomad job run -check-index 10 ./example.nomad

When running the job with the check-index flag, the job will only be run if the
job modify index given matches the server-side version. If the index has
changed, another user has modified the job and the plan's results are
potentially invalid.

The serviceCheckDiff function looks like a likely spot where we might be missing the logic here, because arrays need special casing.
@tgross
Copy link
Member Author

tgross commented Nov 8, 2021

Probably related #11255

@tgross tgross mentioned this issue Nov 8, 2021
Open
@apollo13
Copy link
Contributor

apollo13 commented Jan 5, 2022

@tgross Mind closing this one (even though it is older) because the other has parts of a patch available so people don't have to dig in?

@tgross
Copy link
Member Author

tgross commented Jan 5, 2022

Good call.

@tgross tgross closed this as completed Jan 5, 2022
@github-actions
Copy link

I'm going to lock this issue because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
theme/plan type/bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@apollo13 @tgross