Merge pull request #1695 from endocode/dongsu/agent-conflict-multi-units

agent: allow HasConflict() to handle multiple values defined in Conflicts

Author: Dongsu Park

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{

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

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 {

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

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

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

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.

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
+}

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))