-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Validate and document 0.12 mbits/network deprecations #8743
Conversation
Note that the homepage still uses |
I changed the mock Job to reflect the task network changes and ended up playing quite a bit of whack-a-test as a result. I also updated the init command to render jobs with network and service definitions in the task group with links back to docs site. |
8922a38
to
b0acc33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good - a couple of questions nitpicking about versions.
if len(tgNetworks) > 0 { | ||
ports := tgNetworks[0].PortLabels() | ||
for portLabel := range ports { | ||
portLabels[portLabel] = struct{}{} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a backward compatibility concern here? If an old job that specifies ports and labels in task resources, will they continue to be validated/processed the same way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking here again - if the answer is no, let's merge then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@notnoop I've re-added the removed block, appending task level port labels to the set of port labels to validate against.
I'm pretty sure this test failure is relevant, I'll dig into it.
|
The updated documented examples seem to work as expected:
A warning when setting
A warning when setting
|
The port validation in There's this difference in behavior between master and this PR using this job file: job "example" {
datacenters = ["dc1"]
group "cache" {
task "redis" {
service {
name = "rservice"
port = "db"
}
driver = "docker"
config {
image = "redis:3.2"
port_map {
db = 6379
}
}
resources {
cpu = 500
memory = 256
network {
mbits = 10
port "db" {}
}
}
}
}
} on master)
on PR)
|
cf3af87
to
d5a422e
Compare
Enable users to submit jobs that still make use of the deprecated task.resources.network stanza. Such jobs can be submitted, but will emit a warning.
Added a fix for the above backwards incompatibility, now the job is submissible with warnings as expected
|
I'll add a few e2e tests around this, but in a separate PR. |
@shoenig - I think the above workaround might be misunderstanding the problem. The problem is that services can be defined at the group OR the task level, but network resources defined at the task level are deprecated. If a port is defined at the group level, it can't seem to be used at the task level - hence:
For some context - I want to define my service at the task level in a "count > 1" situation so that I can apply a consul service tag to differentiate the different instances (using NOMAD_ALLOC_INDEX) - this yields DNS entries like |
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. |
This PR adds two warnings to job validation including a check for task network resources and mbits in the group network.
In addition a notice is added to the upgrade docs about the deprecation.
fixes #8497 #8681 #8746 #8780