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.

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

consul: add support for service weight #24186

Conversation

mvegter
Copy link
Contributor

@mvegter mvegter commented Oct 13, 2024

Manual verification

No `weights`
$ curl -s http://127.0.0.1:8500/v1/agent/services  | jq '.[] | select(.Service=="global-redis-check") | .Weights'
{
  "Passing": 1,
  "Warning": 1
}

$ dig @127.0.0.1 -p 8600 global-redis-check.service.consul SRV +short
1 1 27982 7f000001.addr.dc1.consul.

weights {}
$ ./bin/nomad job plan ./job.hcl
+/- Job: "redis"
+/- Task Group: "cache" (1 in-place update)
  +/- Task: "redis" (forces in-place update)
    +/- Service {
        Address:           ""
        ...
      + Weights {
        + Passing: "1"
        + Warning: "1"
        }
        }
$ curl -s http://127.0.0.1:8500/v1/agent/services  | jq '.[] | select(.Service=="global-redis-check") | .Weights'
{
  "Passing": 1,
  "Warning": 1
}	

weights {
  passing = 5
}
$ ./bin/nomad job plan ./job.hcl
+/- Job: "redis"
+/- Task Group: "cache" (1 in-place update)
  +/- Task: "redis" (forces in-place update)
    +/- Service {
          Address:           ""
          ...
      +/- Weights {
        +/- Passing: "1" => "5"
            Warning: "1"
          }
        }
$ curl -s http://127.0.0.1:8500/v1/agent/services  | jq '.[] | select(.Service=="global-redis-check") | .Weights'
{
  "Passing": 5,
  "Warning": 1
}

$ dig @127.0.0.1 -p 8600 global-redis-check.service.consul SRV +short
1 5 30957 7f000001.addr.dc1.consul.

@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch from 3fe74b5 to 3694def Compare October 13, 2024 19:37
@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch from 3694def to dd348c6 Compare October 13, 2024 19:39
@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch from dd348c6 to c711f74 Compare October 14, 2024 08:33
@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch 2 times, most recently from 631031b to 8c93c30 Compare October 14, 2024 08:54
@mvegter mvegter marked this pull request as ready for review October 14, 2024 08:56
@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch from 8c93c30 to 5576cf8 Compare October 14, 2024 09:11
@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch from 5576cf8 to 3b34341 Compare October 15, 2024 07:57
@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch from 3b34341 to f17d4b5 Compare October 15, 2024 14:08
@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch from f17d4b5 to f6472a6 Compare October 16, 2024 13:41
@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch from f6472a6 to 3c0001a Compare October 16, 2024 14:11
@mvegter mvegter force-pushed the mvegter-add-support-for-consul-service-weight-registration branch from 3c0001a to 11b7934 Compare October 16, 2024 14:28
@tgross
Copy link
Member

tgross commented Oct 16, 2024

Hi @mvegter! Just wanted to let you know we do see this PR and appreciate that you're trying to get tests green. We're a little swamped with some post-1.9.0 mop-up but we'll review this as soon as feasible. Thanks!

@mvegter
Copy link
Contributor Author

mvegter commented Oct 16, 2024

hey @tgross , thanks for the heads up. After some more testing I noticed a small issue where the agentServiceUpdateRequired returned false when going from having the weights stanza to unsetting it. It should now be fixed and added some more tests to cover this as well, I see the last failing pipelines turning green as we speak. Looking forward to your feedback !

@Juanadelacuesta
Copy link
Member

Hello @mvegter thank you for putting more work into your PR, @philrenaud can give you a hand on the Vercel problems.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mvegter! This is looking really good. I've left some comments but once those are resolved I think we'll be ready to merge.

api/tasks.go Outdated Show resolved Hide resolved
command/agent/consul/connect.go Outdated Show resolved Hide resolved
command/agent/job_endpoint.go Outdated Show resolved Hide resolved
nomad/structs/services.go Outdated Show resolved Hide resolved
website/content/docs/job-specification/service.mdx Outdated Show resolved Hide resolved
nomad/structs/services.go Outdated Show resolved Hide resolved
command/agent/consul/unit_test.go Outdated Show resolved Hide resolved
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Once CI has finished we'll get this merged and it'll ship in the next release of Nomad. Thanks @mvegter!

@tgross tgross added the backport/1.9.x backport to 1.9.x release line label Oct 25, 2024
@tgross tgross merged commit 6236f35 into hashicorp:main Oct 25, 2024
26 checks passed
@mvegter
Copy link
Contributor Author

mvegter commented Oct 25, 2024

Thanks @tgross , looking forward to the next release of 1.9.x !

@mvegter mvegter deleted the mvegter-add-support-for-consul-service-weight-registration branch October 25, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

3 participants