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

Add support for consul EnableTagOverride #5775

Conversation

bogdanalbei
Copy link

At the moment the Consul's EnableTagOverride flag not being set by Nomad, so the default(false) is always used. In certain cases it is useful for services outside Nomad to update the tags of the Consul services, however for that to happen EnableTagOverride has to be set to true.
This change enables setting EnableTagOverride at a service level.
An older request describing this can be found here #2057

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 4, 2019

CLA assistant check
All committers have signed the CLA.

@bogdanalbei bogdanalbei changed the title Consul enable tag override master rebase Add support for consul EnableTagOverride Jun 4, 2019
@preetapan
Copy link
Contributor

@bogdanalbei please fix linting errors using gofmt - https://travis-ci.org/hashicorp/nomad/jobs/541187845

@endocrimes
Copy link
Contributor

endocrimes commented Jun 4, 2019

func agentServiceUpdateRequired(reg *api.AgentServiceRegistration, svc *api.AgentService) bool {
return !(reg.Kind == svc.Kind &&
reg.ID == svc.ID &&
reg.Port == svc.Port &&
reg.Address == svc.Address &&
reg.Name == svc.Service &&
reflect.DeepEqual(reg.Tags, svc.Tags))
}
will also need updating to ensure that the AgentService tags are a superset of the service registration definition - otherwise Nomad will revert them every time it reconciles with Consul.

There may also be potential issues if other state causes the service to be re-registered (that could drop unmanaged tags), such as when promoting canary deployments.

@bogdanalbei
Copy link
Author

bogdanalbei commented Jun 5, 2019

you're right @endocrimes, I updated the code such that if EnableTagOverride is set, then the Consul tags are being kept and not overridden. I tested the change end to end and it works as expected.
Below are the test steps:

  1. POST http://localhost:4646/v1/jobs
    "Job": {
        "ID": "example",
        "Name": "example",
        "Type": "service",
        "Priority": 50,
        "Datacenters": [
            "dc1"
        ],
        "TaskGroups": [{
            "Name": "cache",
            "Count": 1,
            "Tasks": [{
                "Name": "redis",
                "Driver": "docker",
                "User": "",
                "Config": {
                    "image": "redis:3.2",
                    "port_map": [{
                        "db": 6379
                    }]
                },
                "Services": [{
                    "Id": "",
                    "Name": "redis-cache",
                    "Tags": [
                        "global",
                        "cache"
                    ],
                    "PortLabel": "db",
                    "AddressMode": "",
                    "EnableTagOverride": true,
                    "Checks": [{
                        "Id": "",
                        "Name": "alive",
                        "Type": "tcp",
                        "Command": "",
                        "Args": null,
                        "Path": "",
                        "Protocol": "",
                        "PortLabel": "",
                        "Interval": 10000000000,
                        "Timeout": 2000000000,
                        "InitialStatus": "",
                        "TLSSkipVerify": false
                    }]
                }],
                "Resources": {
                    "CPU": 500,
                    "MemoryMB": 256,
                    "Networks": [{
                        "Device": "",
                        "CIDR": "",
                        "IP": "",
                        "MBits": 10,
                        "DynamicPorts": [{
                            "Label": "db",
                            "Value": 0
                        }]
                    }]
                },
                "Leader": false
            }],
            "RestartPolicy": {
                "Interval": 300000000000,
                "Attempts": 10,
                "Delay": 25000000000,
                "Mode": "delay"
            },
            "ReschedulePolicy": {
                "Attempts": 10,
                "Delay": 30000000000,
                "DelayFunction": "exponential",
                "Interval": 36000000000000,
                "MaxDelay": 3600000000000,
                "Unlimited": false
            },
            "EphemeralDisk": {
                "SizeMB": 300
            }
        }],
        "Update": {
            "MaxParallel": 1,
            "MinHealthyTime": 10000000000,
            "HealthyDeadline": 180000000000,
            "AutoRevert": false,
            "Canary": 0
        }
    }
}
  1. GET http://localhost:8500/v1/catalog/service/redis-cache

  2. PUT httplocalhost:8500/v1/agent/service/register

  "ID": "_nomad-task-c3d9c00e-0f06-de15-b53a-2e5e31ab67f8-redis-redis-cache-true",
  "Name": "redis-cache",
  "Tags": [
	"updated",
        "tags"
  ]
}
  1. GET http://localhost:8500/v1/catalog/service/redis-cache - the updated tags shouldn't be reverted by Nomad

@dcarbone
Copy link

dcarbone commented Aug 26, 2019

It would be really great if this could be merged in.

I have an application that needs to dynamically add and remove fabio urlprefix tags from its definition during runtime. At the moment this means I have to manage it in-process / implement some weird double-registration nonsense.

Would be great if I could offload the service lifecycle to nomad, and merely perform the updates in process.

edit - jira markup has ruined me.

@endocrimes endocrimes added this to the 0.10.0 milestone Sep 2, 2019
@endocrimes
Copy link
Contributor

@bogdanalbei I've targetted this for ~0.10.0, but I'd like to hold off merging till after #6197 merges. If you could rebase after that I'll take a look again 👍

@nickethier nickethier modified the milestones: 0.10.0, 0.10.1 Sep 3, 2019
@schmichael schmichael modified the milestones: 0.10.1, 0.10.2 Nov 5, 2019
@preetapan preetapan modified the milestones: 0.10.2, 0.10.3 Nov 20, 2019
@schmichael schmichael removed this from the 0.10.4 milestone Feb 5, 2020
@shoenig
Copy link
Member

shoenig commented Feb 13, 2020

Thanks for the PR, @bogdanalbei! I started looking into this and realized there's some non-obvious complexity around the stateful house-keeping between Nomad's view of a service definition and what is maintained by each Consul agent. In short, we need to keep track of why a service is being synchronized with the agent, so that we can switch on that knowledge and make the right changes accordingly. There's a more detailed explanation in #7106

I'll go ahead and close this out; the ability to configure service.enable_tag_override will be shipping with v0.11

@shoenig shoenig closed this Feb 13, 2020
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, 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 Jan 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants