From cdc9b44a4beee649eb04a1673d90bd4fca81c082 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Wed, 26 Oct 2016 16:54:23 +0200 Subject: [PATCH 1/4] job: parse Conflicts option in a more extensive way So far when parsing values defined in the Conflicts option, it hasn't taken multiple values into account. So let's allow j.Conflicts() to parse multiple values, splitting with white-space delimiters into a slice of each string. --- job/job.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/job/job.go b/job/job.go index 1ed001f14..a8dbf8cb1 100644 --- a/job/job.go +++ b/job/job.go @@ -220,8 +220,13 @@ func (j *Job) ValidateRequirements() error { // machine as this Job. func (j *Job) Conflicts() []string { conflicts := make([]string, 0) - conflicts = append(conflicts, j.requirements()[deprecatedXPrefix+fleetConflicts]...) - conflicts = append(conflicts, j.requirements()[fleetConflicts]...) + + ldConflicts := splitCombine(j.requirements()[deprecatedXPrefix+fleetConflicts]) + conflicts = append(conflicts, ldConflicts...) + + dConflicts := splitCombine(j.requirements()[fleetConflicts]) + conflicts = append(conflicts, dConflicts...) + return conflicts } @@ -332,3 +337,14 @@ func isTruthyValue(s string) bool { chl := strings.ToLower(s) return chl == "true" || chl == "yes" || chl == "1" || chl == "on" || chl == "t" } + +// splitCombine retrieves each word from an input string slice, to put each +// one again into a single slice. +func splitCombine(inStrs []string) []string { + outStrs := make([]string, 0) + for _, str := range inStrs { + inStrs := strings.Fields(str) + outStrs = append(outStrs, inStrs...) + } + return outStrs +} From f4aec0742493088fef9775dc385dfb84e754fde9 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Thu, 27 Oct 2016 12:02:01 +0200 Subject: [PATCH 2/4] agent: allow HasConflict() to handle multiple values defined in Conflicts In AgentState.HasConflict(), split a common part for checking if a unit belongs to a Conflicts list, into a new helper hasStringInSlice(). Also return a slice of conflicted strings, instead of a single string. Doing that, HasConflict() can be more readable. --- agent/state.go | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/agent/state.go b/agent/state.go index c254147fa..7562ab199 100644 --- a/agent/state.go +++ b/agent/state.go @@ -39,31 +39,42 @@ func (as *AgentState) unitScheduled(name string) bool { return as.Units[name] != nil } +func hasStringInSlice(inSlice []string, unitName string) bool { + for _, elem := range inSlice { + if globMatches(elem, unitName) { + return true + } + } + return false +} + // HasConflict determines whether there are any known conflicts with the given Unit -func (as *AgentState) HasConflict(pUnitName string, pConflicts []string) (found bool, conflict string) { +func (as *AgentState) HasConflict(pUnitName string, pConflicts []string) (bool, []string) { + found := false + conflicts := []string{} + for _, eUnit := range as.Units { if pUnitName == eUnit.Name { continue } - for _, pConflict := range pConflicts { - if globMatches(pConflict, eUnit.Name) { - found = true - conflict = eUnit.Name - return - } + if hasStringInSlice(eUnit.Conflicts(), pUnitName) { + conflicts = append(conflicts, pUnitName) + found = true + break } - - for _, eConflict := range eUnit.Conflicts() { - if globMatches(eConflict, pUnitName) { - found = true - conflict = eUnit.Name - return - } + if hasStringInSlice(pConflicts, eUnit.Name) { + conflicts = append(conflicts, eUnit.Name) + found = true + break } } - return + if !found { + return false, []string{} + } + + return true, conflicts } // hasReplace determines whether there are any known replaces with the given Unit From ab82f762b4c352e19c0e082393d4bd3e20178104 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Mon, 7 Nov 2016 12:39:25 +0100 Subject: [PATCH 3/4] agent,job: add new test cases for multiple Conflicts values Now that it's possible to parse multiple values defined in Conflicts, we should add new test cases for unit tests, TestAbleToRun(), TestHasConflicts(), and TestJobConflicts(), e.g.: Conflicts=pong.service Conflicts=ping.service and Conflicts=pong.service ping.service --- agent/reconcile_test.go | 24 +++++++++++++ agent/state_test.go | 80 +++++++++++++++++++++++++++++++++++++++++ job/job_test.go | 15 ++++++++ 3 files changed, 119 insertions(+) diff --git a/agent/reconcile_test.go b/agent/reconcile_test.go index 9d4fb38cc..e20e6b633 100644 --- a/agent/reconcile_test.go +++ b/agent/reconcile_test.go @@ -387,6 +387,30 @@ func TestAbleToRun(t *testing.T) { want: job.JobActionUnschedule, }, + // conflicts found + { + dState: &AgentState{ + MState: &machine.MachineState{ID: "123"}, + Units: map[string]*job.Unit{ + "ping.service": &job.Unit{Name: "ping.service"}, + }, + }, + job: newTestJobWithXFleetValues(t, "Conflicts=pong.service\nConflicts=ping.service"), + want: job.JobActionUnschedule, + }, + + // conflicts found + { + dState: &AgentState{ + MState: &machine.MachineState{ID: "123"}, + Units: map[string]*job.Unit{ + "ping.service": &job.Unit{Name: "ping.service"}, + }, + }, + job: newTestJobWithXFleetValues(t, "Conflicts=pong.service ping.service"), + want: job.JobActionUnschedule, + }, + // no replaces found { dState: &AgentState{ diff --git a/agent/state_test.go b/agent/state_test.go index e259949db..f4be7a7d9 100644 --- a/agent/state_test.go +++ b/agent/state_test.go @@ -82,6 +82,86 @@ func TestHasConflicts(t *testing.T) { want: true, conflict: "bar.service", }, + + // existing job has conflict with new job, + // one of multiple conflicts defined in a single line + { + cState: &AgentState{ + MState: &machine.MachineState{ID: "XXX"}, + Units: map[string]*job.Unit{ + "bar.service": &job.Unit{ + Name: "bar.service", + Unit: fleetUnit(t, "Conflicts=foo.service bar.service"), + }, + }, + }, + job: &job.Job{ + Name: "foo.service", + Unit: unit.UnitFile{}, + }, + want: true, + conflict: "bar.service", + }, + + // existing job has conflict with new job, + // one of multiple conflicts over multiple lines + { + cState: &AgentState{ + MState: &machine.MachineState{ID: "XXX"}, + Units: map[string]*job.Unit{ + "bar.service": &job.Unit{ + Name: "bar.service", + Unit: fleetUnit(t, "Conflicts=foo.service\nConflicts=bar.service"), + }, + }, + }, + job: &job.Job{ + Name: "foo.service", + Unit: unit.UnitFile{}, + }, + want: true, + conflict: "bar.service", + }, + + // new job has conflict with existing job, + // one of multiple conflicts defined in a single line + { + cState: &AgentState{ + MState: &machine.MachineState{ID: "XXX"}, + Units: map[string]*job.Unit{ + "bar.service": &job.Unit{ + Name: "bar.service", + Unit: unit.UnitFile{}, + }, + }, + }, + job: &job.Job{ + Name: "foo.service", + Unit: fleetUnit(t, "Conflicts=foo.service bar.service"), + }, + want: true, + conflict: "bar.service", + }, + + // new job has conflict with existing job, + // one of multiple conflicts over multiple lines + { + cState: &AgentState{ + MState: &machine.MachineState{ID: "XXX"}, + Units: map[string]*job.Unit{ + "bar.service": &job.Unit{ + Name: "bar.service", + Unit: unit.UnitFile{}, + }, + }, + }, + job: &job.Job{ + Name: "foo.service", + Unit: fleetUnit(t, "Conflicts=foo.service\nConflicts=bar.service"), + }, + want: true, + conflict: "bar.service", + }, } for i, tt := range tests { diff --git a/job/job_test.go b/job/job_test.go index 94c4be163..ba54b2829 100644 --- a/job/job_test.go +++ b/job/job_test.go @@ -77,6 +77,21 @@ Description=Testing [X-Fleet] Conflicts=*bar* `, []string{"*bar*"}}, + {``, []string{}}, + {`[Unit] +Description=Testing + +[X-Fleet] +Conflicts=*bar* *foo* +`, []string{"*bar*", "*foo*"}}, + {``, []string{}}, + {`[Unit] +Description=Testing + +[X-Fleet] +Conflicts=*bar* +Conflicts=*foo* +`, []string{"*bar*", "*foo*"}}, } for i, tt := range testCases { j := NewJob("echo.service", *newUnit(t, tt.contents)) From e06ce1d660ed827e1787cf018456198b94f656c2 Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Mon, 7 Nov 2016 12:39:32 +0100 Subject: [PATCH 4/4] functional: new functional test TestScheduleMultipleConflicts To test the new feature of having multiple values in Conflicts, introduce a functional test TestScheduleMultipleConflicts(). --- .../units/conflict.multiple.0.service | 8 +++ .../units/conflict.multiple.1.service | 8 +++ .../units/conflict.multiple.2.service | 10 +++ functional/scheduling_test.go | 68 +++++++++++++++++++ 4 files changed, 94 insertions(+) create mode 100644 functional/fixtures/units/conflict.multiple.0.service create mode 100644 functional/fixtures/units/conflict.multiple.1.service create mode 100644 functional/fixtures/units/conflict.multiple.2.service diff --git a/functional/fixtures/units/conflict.multiple.0.service b/functional/fixtures/units/conflict.multiple.0.service new file mode 100644 index 000000000..4b30e8fc6 --- /dev/null +++ b/functional/fixtures/units/conflict.multiple.0.service @@ -0,0 +1,8 @@ +[Unit] +Description=Test Unit + +[Service] +ExecStart=/bin/bash -c "while true; do echo Hello, World!; sleep 1; done" + +[X-Fleet] +Conflicts=conflict.2.service conflict.1.service conflict.0.service diff --git a/functional/fixtures/units/conflict.multiple.1.service b/functional/fixtures/units/conflict.multiple.1.service new file mode 100644 index 000000000..4b30e8fc6 --- /dev/null +++ b/functional/fixtures/units/conflict.multiple.1.service @@ -0,0 +1,8 @@ +[Unit] +Description=Test Unit + +[Service] +ExecStart=/bin/bash -c "while true; do echo Hello, World!; sleep 1; done" + +[X-Fleet] +Conflicts=conflict.2.service conflict.1.service conflict.0.service diff --git a/functional/fixtures/units/conflict.multiple.2.service b/functional/fixtures/units/conflict.multiple.2.service new file mode 100644 index 000000000..5a93342da --- /dev/null +++ b/functional/fixtures/units/conflict.multiple.2.service @@ -0,0 +1,10 @@ +[Unit] +Description=Test Unit + +[Service] +ExecStart=/bin/bash -c "while true; do echo Hello, World!; sleep 1; done" + +[X-Fleet] +Conflicts=conflict.2.service +Conflicts=conflict.1.service +Conflicts=conflict.0.service diff --git a/functional/scheduling_test.go b/functional/scheduling_test.go index d29066484..6771df2ee 100644 --- a/functional/scheduling_test.go +++ b/functional/scheduling_test.go @@ -333,6 +333,74 @@ func TestScheduleOneWayConflict(t *testing.T) { } +// Start 3 services that conflict with one another. +func TestScheduleMultipleConflicts(t *testing.T) { + cluster, err := platform.NewNspawnCluster("smoke") + if err != nil { + t.Fatal(err) + } + defer cluster.Destroy(t) + + // Start with a simple three-node cluster + members, err := platform.CreateNClusterMembers(cluster, 3) + if err != nil { + t.Fatal(err) + } + m0 := members[0] + machines, err := cluster.WaitForNMachines(m0, 3) + if err != nil { + t.Fatal(err) + } + + // Ensure we can SSH into each machine using fleetctl + for _, machine := range machines { + if stdout, stderr, err := cluster.Fleetctl(m0, "--strict-host-key-checking=false", "ssh", machine, "uptime"); err != nil { + t.Errorf("Unable to SSH into fleet machine: \nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) + } + } + + for i := 0; i < 3; i++ { + unit := fmt.Sprintf("fixtures/units/conflict.multiple.%d.service", i) + stdout, stderr, err := cluster.Fleetctl(m0, "start", "--no-block", unit) + if err != nil { + t.Errorf("Failed starting unit %s: \nstdout: %s\nstderr: %s\nerr: %v", unit, stdout, stderr, err) + } + } + + // All 3 services should be visible immediately and 3 should become + // ACTIVE shortly thereafter + stdout, stderr, err := cluster.Fleetctl(m0, "list-unit-files", "--no-legend") + if err != nil { + t.Fatalf("Failed to run list-unit-files:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) + } + units := strings.Split(strings.TrimSpace(stdout), "\n") + if len(units) != 3 { + t.Fatalf("Did not find five units in cluster: \n%s", stdout) + } + active, err := cluster.WaitForNActiveUnits(m0, 3) + if err != nil { + t.Fatal(err) + } + states, err := util.ActiveToSingleStates(active) + if err != nil { + t.Fatal(err) + } + + machineSet := make(map[string]bool) + + for unit, unitState := range states { + if len(unitState.Machine) == 0 { + t.Errorf("Unit %s is not reporting machine", unit) + } + + machineSet[unitState.Machine] = true + } + + if len(machineSet) != 3 { + t.Errorf("3 active units not running on 3 unique machines") + } +} + // TestScheduleReplace starts 3 units, followed by starting another unit // that replaces the 1st unit. Then it verifies that the original unit // got rescheduled on a different machine.