diff --git a/agent/reconcile_test.go b/agent/reconcile_test.go index 8ee79ac45..20a0b6533 100644 --- a/agent/reconcile_test.go +++ b/agent/reconcile_test.go @@ -388,6 +388,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.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 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/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. 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 +} 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))