Permalink
Browse files

fix bug occurring when colocating jobs with new and old style properties

[fixes #47304677]
  • Loading branch information...
1 parent 7ef64ff commit afd5a849982e075d6eb122475a8ee650f25fa7dd Amit Gupta and Chris Brown committed Apr 3, 2013
View
62 director/lib/director/deployment_plan/job.rb
@@ -227,17 +227,15 @@ def parse_release
@release = @deployment.releases.first
else
raise JobMissingRelease,
- "Cannot tell what release job `#{@name}' " +
- "supposed to use, please reference an existing release"
+ "Cannot tell what release job `#{@name}' supposed to use, please reference an existing release"
end
else
@release = @deployment.release(release_name)
end
if @release.nil?
raise JobUnknownRelease,
- "Job `#{@name}' references " +
- "an unknown release `#{release_name}'"
+ "Job `#{@name}' references an unknown release `#{release_name}'"
end
end
@@ -264,31 +262,27 @@ def parse_template
end
def parse_disk
- @persistent_disk = safe_property(@job_spec, "persistent_disk",
- :class => Integer, :default => 0)
+ @persistent_disk = safe_property(@job_spec, "persistent_disk", :class => Integer, :default => 0)
end
def parse_properties
# Manifest can contain global and per-job properties section
- job_properties = safe_property(
- @job_spec, "properties", :class => Hash, :optional => true)
+ job_properties = safe_property(@job_spec, "properties", :class => Hash, :optional => true)
@all_properties = deployment.properties._deep_copy
if job_properties
@all_properties.recursive_merge!(job_properties)
end
- mappings = safe_property(
- @job_spec, "property_mappings", :class => Hash, :default => {})
+ mappings = safe_property(@job_spec, "property_mappings", :class => Hash, :default => {})
mappings.each_pair do |to, from|
resolved = lookup_property(@all_properties, from)
if resolved.nil?
raise JobInvalidPropertyMapping,
- "Cannot satisfy property mapping `#{to}: #{from}', " +
- "as `#{from}' is not in deployment properties"
+ "Cannot satisfy property mapping `#{to}: #{from}', as `#{from}' is not in deployment properties"
end
@all_properties[to] = resolved
@@ -301,8 +295,7 @@ def parse_resource_pool
@resource_pool = deployment.resource_pool(resource_pool_name)
if @resource_pool.nil?
raise JobUnknownResourcePool,
- "Job `#{@name}' references " +
- "an unknown resource pool `#{resource_pool_name}'"
+ "Job `#{@name}' references an unknown resource pool `#{resource_pool_name}'"
end
end
@@ -337,16 +330,14 @@ def parse_instances
end
unless VALID_JOB_STATES.include?(state)
raise JobInvalidInstanceState,
- "Invalid state `#{state}' for `#{@name}/#{index}', " +
- "valid states are: #{VALID_JOB_STATES.join(", ")}"
+ "Invalid state `#{state}' for `#{@name}/#{index}', valid states are: #{VALID_JOB_STATES.join(", ")}"
end
@instance_states[index] = state
end
if @state && !VALID_JOB_STATES.include?(@state)
raise JobInvalidJobState,
- "Invalid state `#{@state}' for `#{@name}', " +
- "valid states are: #{VALID_JOB_STATES.join(", ")}"
+ "Invalid state `#{@state}' for `#{@name}', valid states are: #{VALID_JOB_STATES.join(", ")}"
end
job_size.times do |index|
@@ -370,8 +361,7 @@ def parse_networks
network = @deployment.network(network_name)
if network.nil?
raise JobUnknownNetwork,
- "Job `#{@name}' references " +
- "an unknown network `#{network_name}'"
+ "Job `#{@name}' references an unknown network `#{network_name}'"
end
static_ips = nil
@@ -382,8 +372,7 @@ def parse_networks
end
if static_ips.size != @instances.size
raise JobNetworkInstanceIpMismatch,
- "Job `#{@name}' has #{@instances.size} " +
- "instances but was allocated #{static_ips.size} static IPs"
+ "Job `#{@name}' has #{@instances.size} instances but was allocated #{static_ips.size} static IPs"
end
end
@@ -393,16 +382,13 @@ def parse_networks
default_network.each do |property|
unless Network::VALID_DEFAULTS.include?(property)
raise JobNetworkInvalidDefault,
- "Job `#{@name}' specified " +
- "an invalid default network property `#{property}', " +
- "valid properties are: " +
- Network::VALID_DEFAULTS.join(", ")
+ "Job `#{@name}' specified an invalid default network property `#{property}', " +
+ "valid properties are: " + Network::VALID_DEFAULTS.join(", ")
end
if @default_network[property]
raise JobNetworkMultipleDefaults,
- "Job `#{@name}' specified more than one " +
- "network to contain default #{property}"
+ "Job `#{@name}' specified more than one network to contain default #{property}"
else
@default_network[property] = network_name
end
@@ -429,8 +415,7 @@ def parse_networks
unless missing_default_properties.empty?
raise JobNetworkMissingDefault,
"Job `#{@name}' must specify which network is default for " +
- missing_default_properties.sort.join(", ") +
- ", since it has more than one network configured"
+ missing_default_properties.sort.join(", ") + ", since it has more than one network configured"
end
else
# Set the default network to the one and only available network
@@ -456,17 +441,17 @@ def bind_properties
# @return [Hash] Properties required by templates included in this job
def filter_properties(collection)
if @templates.empty?
- raise DirectorError,
- "Can't extract job properties before " +
- "parsing job templates"
+ raise DirectorError, "Can't extract job properties before parsing job templates"
end
- @templates.each do |template|
- # If at least one template doesn't have properties defined, we
- # need all properties to be available to job (backward-compatibility)
- return collection if template.properties.nil?
- end
+ return collection if @templates.all? { |template| template.properties.nil? }
+ return extract_template_properties(collection) if @templates.none? { |template| template.properties.nil? }
+ raise JobIncompatibleSpecs, "Job `#{name}' has specs with conflicting property definition styles between" +
+ " its job spec templates. This may occur if colocating jobs, one of which has a spec file including" +
+ " `properties' and one which doesn't."
+ end
+ def extract_template_properties(collection)
result = {}
@templates.each do |template|
@@ -477,7 +462,6 @@ def filter_properties(collection)
result
end
-
end
end
end
View
1 director/lib/director/errors.rb
@@ -103,6 +103,7 @@ def self.err(error_code, response_code = BAD_REQUEST)
JobTemplateUnpackFailed = err(80007)
JobInvalidPropertySpec = err(80008)
JobInvalidPropertyMapping = err(80009)
+ JobIncompatibleSpecs = err(80010)
ResourceError = err(100001)
ResourceNotFound = err(100002, NOT_FOUND)
View
313 director/spec/unit/deployment_plan/job_spec.rb
@@ -6,10 +6,10 @@
def make_spec
{
- "name" => "foobar",
- "template" => "foo",
- "release" => "appcloud",
- "resource_pool" => "dea"
+ "name" => "foobar",
+ "template" => "foo",
+ "release" => "appcloud",
+ "resource_pool" => "dea"
}
end
@@ -21,10 +21,6 @@ def make_plan(deployment)
mock(BD::DeploymentPlan, :model => deployment)
end
- def make(deployment, job_spec)
- BD::DeploymentPlan::Job.new(deployment, job_spec)
- end
-
describe "parsing job spec" do
before(:each) do
@@ -34,29 +30,29 @@ def make(deployment, job_spec)
end
it "parses name" do
- job = make(@plan, @spec)
+ job = described_class.new(@plan, @spec)
job.parse_name
job.name.should == "foobar"
end
it "parses release" do
- job = make(@plan, @spec)
+ job = described_class.new(@plan, @spec)
release = mock(BD::DeploymentPlan::Release)
@plan.should_receive(:release).with("appcloud").and_return(release)
job.parse_release
job.release.should == release
end
it "complains about unknown release" do
- job = make(@plan, @spec)
+ job = described_class.new(@plan, @spec)
@plan.should_receive(:release).with("appcloud").and_return(nil)
expect {
job.parse_release
}.to raise_error(BD::JobUnknownRelease)
end
it "parses a single template" do
- job = make(@plan, @spec)
+ job = described_class.new(@plan, @spec)
release = mock(BD::DeploymentPlan::Release)
template = mock(BD::DeploymentPlan::Template)
@@ -71,7 +67,7 @@ def make(deployment, job_spec)
it "parses multiple templates" do
@spec["template"] = %w(foo bar)
- job = make(@plan, @spec)
+ job = described_class.new(@plan, @spec)
release = mock(BD::DeploymentPlan::Release)
foo_template = mock(BD::DeploymentPlan::Template)
bar_template = mock(BD::DeploymentPlan::Template)
@@ -91,19 +87,19 @@ def make(deployment, job_spec)
it "parses persistent disk if present" do
@spec["persistent_disk"] = 300
- job = make(@plan, @spec)
+ job = described_class.new(@plan, @spec)
job.parse_disk
job.persistent_disk.should == 300
end
it "uses 0 for persistent disk if not present" do
- job = make(@plan, @spec)
+ job = described_class.new(@plan, @spec)
job.parse_disk
job.persistent_disk.should == 0
end
it "parses resource pool" do
- job = make(@plan, @spec)
+ job = described_class.new(@plan, @spec)
resource_pool = mock(BD::DeploymentPlan::ResourcePool)
@plan.should_receive(:resource_pool).with("dea").and_return(resource_pool)
@@ -113,7 +109,7 @@ def make(deployment, job_spec)
end
it "complains about unknown resource pool" do
- job = make(@plan, @spec)
+ job = described_class.new(@plan, @spec)
@plan.should_receive(:resource_pool).with("dea").and_return(nil)
@@ -122,172 +118,163 @@ def make(deployment, job_spec)
}.to raise_error(BD::JobUnknownResourcePool)
end
- it "uses all deployment properties if at least one template model " +
- "has no properties defined" do
- props = {
- "cc" => {
- "token" => "deadbeef",
- "max_users" => 1024
- },
- "dea_max_memory" => 2048
- }
-
- @spec["properties"] = props
- @spec["template"] = %w(foo bar)
+ describe "binding properties" do
+ let(:props) do
+ {
+ "cc_url" => "www.cc.com",
+ "deep_property" => {
+ "unneeded" => "abc",
+ "dont_override" => "def"
+ },
+ "dea_max_memory" => 1024
+ }
+ end
+ let(:foo_properties) do
+ {
+ "dea_min_memory" => {"default" => 512},
+ "deep_property.dont_override" => {"default" => "ghi"},
+ "deep_property.new_property" => {"default" => "jkl"}
+ }
+ end
+ let(:bar_properties) do
+ {"dea_max_memory" => {"default" => 2048}}
+ end
+ let(:job) { described_class.new(@plan, @spec) }
- bar_p = {
- "dea_max_memory" => {"default" => 1024}
- }
+ before do
+ @spec["properties"] = props
+ @spec["template"] = %w(foo bar)
- release = mock(BD::DeploymentPlan::Release)
- foo_template = mock(BD::DeploymentPlan::Template, :properties => nil)
- bar_template = mock(BD::DeploymentPlan::Template, :properties => bar_p)
+ release = mock(BD::DeploymentPlan::Release)
- @plan.stub!(:properties).and_return(props)
- @plan.should_receive(:release).with("appcloud").and_return(release)
+ @plan.stub(:properties).and_return(props)
+ @plan.should_receive(:release).with("appcloud").and_return(release)
- release.should_receive(:use_template_named).with("foo")
- release.should_receive(:use_template_named).with("bar")
-
- release.should_receive(:template).with("foo").and_return(foo_template)
- release.should_receive(:template).with("bar").and_return(bar_template)
+ release.should_receive(:use_template_named).with("foo")
+ release.should_receive(:use_template_named).with("bar")
- job = make(@plan, @spec)
+ release.should_receive(:template).with("foo").and_return(foo_template)
+ release.should_receive(:template).with("bar").and_return(bar_template)
- job.parse_release
- job.parse_template
- job.parse_properties
- job.bind_properties
-
- job.properties.should == props
+ job.parse_name
+ job.parse_release
+ job.parse_template
+ job.parse_properties
+ end
+
+ context "when all the job specs (aka templates) specify properties" do
+ let(:foo_template) { mock(BD::DeploymentPlan::Template, :properties => foo_properties) }
+ let(:bar_template) { mock(BD::DeploymentPlan::Template, :properties => bar_properties) }
+
+ before do
+ job.bind_properties
+ end
+
+ it "should drop deployment manifest properties not specified in the job spec properties" do
+ job.properties.should_not have_key "cc"
+ job.properties["deep_property"].should_not have_key "unneeded"
+ end
+
+ it "should include properties that are in the job spec properties but not in the deployment manifest" do
+ job.properties["dea_min_memory"].should == 512
+ job.properties["deep_property"]["new_property"].should == "jkl"
+ end
+
+ it "should not override deployment manifest properties" do
+ job.properties["dea_max_memory"].should == 1024
+ job.properties["deep_property"]["dont_override"].should == "def"
+ end
+ end
+
+ context "when none of the job specs (aka templates) specify properties" do
+ let(:foo_template) { mock(BD::DeploymentPlan::Template, :properties => nil) }
+ let(:bar_template) { mock(BD::DeploymentPlan::Template, :properties => nil) }
+
+ before do
+ job.bind_properties
+ end
+
+ it "should use the properties specified throughout the deployment manifest" do
+ job.properties.should == props
+ end
+ end
+
+ context "when some job specs (aka templates) specify properties and some don't" do
+ let(:foo_template) { mock(BD::DeploymentPlan::Template, :properties => nil) }
+ let(:bar_template) { mock(BD::DeploymentPlan::Template, :properties => bar_properties) }
+
+ it "should raise an error" do
+ expect {
+ job.bind_properties
+ }.to raise_error(BD::JobIncompatibleSpecs, "Job `foobar' has specs with conflicting property definition styles" +
+ " between its job spec templates. This may occur if colocating jobs, one of which has a spec file" +
+ " including `properties' and one which doesn't.")
+ end
+ end
end
- it "only copies properties needed by templates into job properties" do
- props = {
- "cc" => {
- "token" => "deadbeef",
- "max_users" => 1024
- },
- "dea_max_memory" => 2048,
- "foo" => {
- "bar" => "baz",
- "baz" => "zazzle"
- },
- "test_hash" => {"a" => "b", "c" => "d"}
- }
-
- @spec["properties"] = props
- @spec["template"] = %w(foo bar)
-
- foo_p = {
- "cc.token" => {},
- "test.long.property.name" => {"default" => 33},
- "test_hash" => {}
- }
-
- bar_p = {
- "dea_max_memory" => {"default" => 1024},
- "big_bad_wolf" => {"default" => "foo"}
- }
-
- release = mock(BD::DeploymentPlan::Release)
- foo_template = mock(BD::DeploymentPlan::Template, :properties => foo_p)
- bar_template = mock(BD::DeploymentPlan::Template, :properties => bar_p)
-
- @plan.stub!(:properties).and_return(props)
- @plan.should_receive(:release).with("appcloud").and_return(release)
-
- release.should_receive(:use_template_named).with("foo")
- release.should_receive(:use_template_named).with("bar")
-
- release.should_receive(:template).with("foo").and_return(foo_template)
- release.should_receive(:template).with("bar").and_return(bar_template)
-
- job = make(@plan, @spec)
-
- job.parse_release
- job.parse_template
- job.parse_properties
- job.bind_properties
-
- job.properties.should == {
- "cc" => {
- "token" => "deadbeef"
- },
- "dea_max_memory" => 2048,
- "big_bad_wolf" => "foo",
- "test" => {
- "long" => {
- "property" => {
- "name" => 33
+ describe "property mappings" do
+ it "supports property mappings" do
+ props = {
+ "ccdb" => {
+ "user" => "admin",
+ "password" => "12321",
+ "unused" => "yada yada"
+ },
+ "dea" => {
+ "max_memory" => 2048
}
- }
- },
- "test_hash" => {"a" => "b", "c" => "d"}
- }
- end
-
- it "supports property mappings" do
- props = {
- "ccdb" => {
- "user" => "admin",
- "password" => "12321",
- "unused" => "yada yada"
- },
- "dea" => {
- "max_memory" => 2048
}
- }
- @spec["properties"] = props
- @spec["property_mappings"] = {"db" => "ccdb", "mem" => "dea.max_memory"}
- @spec["template"] = "foo"
+ @spec["properties"] = props
+ @spec["property_mappings"] = {"db" => "ccdb", "mem" => "dea.max_memory"}
+ @spec["template"] = "foo"
- foo_p = {
- "db.user" => {"default" => "root"},
- "db.password" => {},
- "db.host" => {"default" => "localhost"},
- "mem" => {"default" => 256}
- }
+ foo_p = {
+ "db.user" => {"default" => "root"},
+ "db.password" => {},
+ "db.host" => {"default" => "localhost"},
+ "mem" => {"default" => 256}
+ }
- release = mock(BD::DeploymentPlan::Release)
- foo_template = mock(BD::DeploymentPlan::Template, :properties => foo_p)
+ release = mock(BD::DeploymentPlan::Release)
+ foo_template = mock(BD::DeploymentPlan::Template, :properties => foo_p)
- @plan.stub!(:properties).and_return(props)
- @plan.should_receive(:release).with("appcloud").and_return(release)
+ @plan.stub!(:properties).and_return(props)
+ @plan.should_receive(:release).with("appcloud").and_return(release)
- release.should_receive(:template).with("foo").and_return(foo_template)
- release.should_receive(:use_template_named).with("foo")
+ release.should_receive(:template).with("foo").and_return(foo_template)
+ release.should_receive(:use_template_named).with("foo")
- job = make(@plan, @spec)
- job.parse_release
- job.parse_template
- job.parse_properties
- job.bind_properties
-
- job.properties.should == {
- "db" => {
- "user" => "admin",
- "password" => "12321",
- "host" => "localhost"
- },
- "mem" => 2048,
- }
- end
+ job = described_class.new(@plan, @spec)
+ job.parse_release
+ job.parse_template
+ job.parse_properties
+ job.bind_properties
+
+ job.properties.should == {
+ "db" => {
+ "user" => "admin",
+ "password" => "12321",
+ "host" => "localhost"
+ },
+ "mem" => 2048,
+ }
+ end
- it "complains about unsatisfiable property mappings" do
- props = {"foo" => "bar"}
+ it "complains about unsatisfiable property mappings" do
+ props = {"foo" => "bar"}
- @spec["properties"] = props
- @spec["property_mappings"] = {"db" => "ccdb"}
+ @spec["properties"] = props
+ @spec["property_mappings"] = {"db" => "ccdb"}
- @plan.stub!(:properties).and_return(props)
+ @plan.stub!(:properties).and_return(props)
- job = make(@plan, @spec)
- expect {
- job.parse_properties
- }.to raise_error(BD::JobInvalidPropertyMapping)
+ job = described_class.new(@plan, @spec)
+ expect {
+ job.parse_properties
+ }.to raise_error(BD::JobInvalidPropertyMapping)
+ end
end
-
end
end

0 comments on commit afd5a84

Please sign in to comment.