From d561366ce9030207f7bbab8a263c73f9121d4afa Mon Sep 17 00:00:00 2001 From: Ce Gao Date: Wed, 22 May 2019 12:12:12 +0800 Subject: [PATCH 1/2] refactor: Use manager client to get log for test Signed-off-by: Ce Gao --- .../trial/managerclient/managerclient.go | 18 ++++++- .../v1alpha2/trial/trial_controller.go | 9 ++-- .../v1alpha2/trial/trial_controller_consts.go | 10 ++++ .../v1alpha2/trial/trial_controller_test.go | 4 ++ ...tatus_util.go => trial_controller_util.go} | 53 ++++++------------- .../trial/managerclient/katibmanager.go | 15 ++++++ 6 files changed, 67 insertions(+), 42 deletions(-) create mode 100644 pkg/controller/v1alpha2/trial/trial_controller_consts.go rename pkg/controller/v1alpha2/trial/{util/status_util.go => trial_controller_util.go} (69%) diff --git a/pkg/controller/v1alpha2/trial/managerclient/managerclient.go b/pkg/controller/v1alpha2/trial/managerclient/managerclient.go index 36f1d9ef1c2..45e0f9423fc 100644 --- a/pkg/controller/v1alpha2/trial/managerclient/managerclient.go +++ b/pkg/controller/v1alpha2/trial/managerclient/managerclient.go @@ -14,6 +14,8 @@ type ManagerClient interface { CreateTrialInDB(instance *trialsv1alpha2.Trial) error UpdateTrialStatusInDB(instance *trialsv1alpha2.Trial) error GetTrialObservation(instance *trialsv1alpha2.Trial) error + GetTrialObservationLog( + instance *trialsv1alpha2.Trial) (*api_pb.GetObservationLogReply, error) GetTrialConf(instance *trialsv1alpha2.Trial) *api_pb.Trial } @@ -66,8 +68,22 @@ func (d *DefaultClient) UpdateTrialStatusInDB(instance *trialsv1alpha2.Trial) er return nil } -func (d *DefaultClient) GetTrialObservation(instance *trialsv1alpha2.Trial) error { +func (d *DefaultClient) GetTrialObservationLog( + instance *trialsv1alpha2.Trial) (*api_pb.GetObservationLogReply, error) { + // read GetObservationLog call and update observation field + objectiveMetricName := instance.Spec.Objective.ObjectiveMetricName + request := &api_pb.GetObservationLogRequest{ + TrialName: instance.Name, + MetricName: objectiveMetricName, + } + reply, err := common.GetObservationLog(request) + if err != nil { + return nil, err + } + return reply, nil +} +func (d *DefaultClient) GetTrialObservation(instance *trialsv1alpha2.Trial) error { return nil } diff --git a/pkg/controller/v1alpha2/trial/trial_controller.go b/pkg/controller/v1alpha2/trial/trial_controller.go index ebc8e331086..1a49dca6495 100644 --- a/pkg/controller/v1alpha2/trial/trial_controller.go +++ b/pkg/controller/v1alpha2/trial/trial_controller.go @@ -42,7 +42,6 @@ import ( trialsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/trial/v1alpha2" commonv1alpha2 "github.com/kubeflow/katib/pkg/common/v1alpha2" "github.com/kubeflow/katib/pkg/controller/v1alpha2/trial/managerclient" - trialutil "github.com/kubeflow/katib/pkg/controller/v1alpha2/trial/util" ) var ( @@ -159,7 +158,7 @@ func (r *ReconcileTrial) Reconcile(request reconcile.Request) (reconcile.Result, instance.Status.CompletionTime = &metav1.Time{} } msg := "Trial is created" - instance.MarkTrialStatusCreated(trialutil.TrialCreatedReason, msg) + instance.MarkTrialStatusCreated(TrialCreatedReason, msg) err = r.CreateTrialInDB(instance) if err != nil { logger.Error(err, "Create trial in DB error") @@ -220,11 +219,11 @@ func (r *ReconcileTrial) reconcileTrial(instance *trialsv1alpha2.Trial) error { //Job already exists //TODO Can desired Spec differ from deployedSpec? if deployedJob != nil { - if err = trialutil.UpdateTrialStatusCondition(instance, deployedJob); err != nil { + if err = r.UpdateTrialStatusCondition(instance, deployedJob); err != nil { logger.Error(err, "Update trial status condition error") return err } - if err = trialutil.UpdateTrialStatusObservation(instance, deployedJob); err != nil { + if err = r.UpdateTrialStatusObservation(instance, deployedJob); err != nil { logger.Error(err, "Update trial status observation error") return err } @@ -259,7 +258,7 @@ func (r *ReconcileTrial) reconcileJob(instance *trialsv1alpha2.Trial, desiredJob } msg := "Trial is running" - instance.MarkTrialStatusRunning(trialutil.TrialRunningReason, msg) + instance.MarkTrialStatusRunning(TrialRunningReason, msg) return deployedJob, nil } diff --git a/pkg/controller/v1alpha2/trial/trial_controller_consts.go b/pkg/controller/v1alpha2/trial/trial_controller_consts.go new file mode 100644 index 00000000000..80c08f688f2 --- /dev/null +++ b/pkg/controller/v1alpha2/trial/trial_controller_consts.go @@ -0,0 +1,10 @@ +package trial + +const ( + DefaultJobKind = "Job" + TrialCreatedReason = "TrialCreated" + TrialRunningReason = "TrialRunning" + TrialSucceededReason = "TrialSucceeded" + TrialFailedReason = "TrialFailed" + TrialKilledReason = "TrialKilled" +) diff --git a/pkg/controller/v1alpha2/trial/trial_controller_test.go b/pkg/controller/v1alpha2/trial/trial_controller_test.go index 5670992a6ef..56ad0546f75 100644 --- a/pkg/controller/v1alpha2/trial/trial_controller_test.go +++ b/pkg/controller/v1alpha2/trial/trial_controller_test.go @@ -19,6 +19,7 @@ import ( commonv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/common/v1alpha2" trialsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/trial/v1alpha2" + api_pb "github.com/kubeflow/katib/pkg/api/v1alpha2" managerclientmock "github.com/kubeflow/katib/pkg/mock/v1alpha2/trial/managerclient" ) @@ -96,6 +97,9 @@ func TestReconcileTFJobTrial(t *testing.T) { mc := managerclientmock.NewMockManagerClient(mockCtrl) mc.EXPECT().CreateTrialInDB(gomock.Any()).Return(nil).AnyTimes() mc.EXPECT().UpdateTrialStatusInDB(gomock.Any()).Return(nil).AnyTimes() + mc.EXPECT().GetTrialObservationLog(gomock.Any()).Return(&api_pb.GetObservationLogReply{ + ObservationLog: nil, + }, nil).AnyTimes() // Setup the Manager and Controller. Wrap the Controller Reconcile function so it writes each request to a // channel when it is finished. diff --git a/pkg/controller/v1alpha2/trial/util/status_util.go b/pkg/controller/v1alpha2/trial/trial_controller_util.go similarity index 69% rename from pkg/controller/v1alpha2/trial/util/status_util.go rename to pkg/controller/v1alpha2/trial/trial_controller_util.go index 7d4a49e358b..e8de2610f82 100644 --- a/pkg/controller/v1alpha2/trial/util/status_util.go +++ b/pkg/controller/v1alpha2/trial/trial_controller_util.go @@ -13,7 +13,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package util +package trial import ( "strconv" @@ -22,27 +22,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - logf "sigs.k8s.io/controller-runtime/pkg/runtime/log" commonv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/common/v1alpha2" trialsv1alpha2 "github.com/kubeflow/katib/pkg/api/operators/apis/trial/v1alpha2" api_pb "github.com/kubeflow/katib/pkg/api/v1alpha2" - common "github.com/kubeflow/katib/pkg/common/v1alpha2" commonv1beta2 "github.com/kubeflow/tf-operator/pkg/apis/common/v1beta2" ) -var log = logf.Log.WithName("trial-status-util") - -const ( - DefaultJobKind = "Job" - TrialCreatedReason = "TrialCreated" - TrialRunningReason = "TrialRunning" - TrialSucceededReason = "TrialSucceeded" - TrialFailedReason = "TrialFailed" - TrialKilledReason = "TrialKilled" -) - -func UpdateTrialStatusCondition(instance *trialsv1alpha2.Trial, deployedJob *unstructured.Unstructured) error { +func (r *ReconcileTrial) UpdateTrialStatusCondition(instance *trialsv1alpha2.Trial, deployedJob *unstructured.Unstructured) error { kind := deployedJob.GetKind() status, ok, unerr := unstructured.NestedFieldCopy(deployedJob.Object, "status") @@ -96,29 +83,23 @@ func UpdateTrialStatusCondition(instance *trialsv1alpha2.Trial, deployedJob *uns return nil } -func UpdateTrialStatusObservation(instance *trialsv1alpha2.Trial, deployedJob *unstructured.Unstructured) error { - - // read GetObservationLog call and update observation field +func (r *ReconcileTrial) UpdateTrialStatusObservation(instance *trialsv1alpha2.Trial, deployedJob *unstructured.Unstructured) error { objectiveMetricName := instance.Spec.Objective.ObjectiveMetricName - request := &api_pb.GetObservationLogRequest{ - TrialName: instance.Name, - MetricName: objectiveMetricName, - } - if reply, err := common.GetObservationLog(request); err != nil { + reply, err := r.GetTrialObservationLog(instance) + if err != nil { return err - } else { - if reply.ObservationLog != nil { - bestObjectiveValue := getBestObjectiveMetricValue(reply.ObservationLog.MetricLogs, instance.Spec.Objective.Type) - if bestObjectiveValue != nil { - if instance.Status.Observation == nil { - instance.Status.Observation = &commonv1alpha2.Observation{} - metric := commonv1alpha2.Metric{Name: objectiveMetricName, Value: *bestObjectiveValue} - instance.Status.Observation.Metrics = []commonv1alpha2.Metric{metric} - } else { - for index, metric := range instance.Status.Observation.Metrics { - if metric.Name == objectiveMetricName { - instance.Status.Observation.Metrics[index].Value = *bestObjectiveValue - } + } + if reply.ObservationLog != nil { + bestObjectiveValue := getBestObjectiveMetricValue(reply.ObservationLog.MetricLogs, instance.Spec.Objective.Type) + if bestObjectiveValue != nil { + if instance.Status.Observation == nil { + instance.Status.Observation = &commonv1alpha2.Observation{} + metric := commonv1alpha2.Metric{Name: objectiveMetricName, Value: *bestObjectiveValue} + instance.Status.Observation.Metrics = []commonv1alpha2.Metric{metric} + } else { + for index, metric := range instance.Status.Observation.Metrics { + if metric.Name == objectiveMetricName { + instance.Status.Observation.Metrics[index].Value = *bestObjectiveValue } } } diff --git a/pkg/mock/v1alpha2/trial/managerclient/katibmanager.go b/pkg/mock/v1alpha2/trial/managerclient/katibmanager.go index 26144e5e040..7f834889f95 100644 --- a/pkg/mock/v1alpha2/trial/managerclient/katibmanager.go +++ b/pkg/mock/v1alpha2/trial/managerclient/katibmanager.go @@ -76,6 +76,21 @@ func (mr *MockManagerClientMockRecorder) GetTrialObservation(arg0 interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTrialObservation", reflect.TypeOf((*MockManagerClient)(nil).GetTrialObservation), arg0) } +// GetTrialObservationLog mocks base method +func (m *MockManagerClient) GetTrialObservationLog(arg0 *v1alpha2.Trial) (*v1alpha20.GetObservationLogReply, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetTrialObservationLog", arg0) + ret0, _ := ret[0].(*v1alpha20.GetObservationLogReply) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetTrialObservationLog indicates an expected call of GetTrialObservationLog +func (mr *MockManagerClientMockRecorder) GetTrialObservationLog(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTrialObservationLog", reflect.TypeOf((*MockManagerClient)(nil).GetTrialObservationLog), arg0) +} + // UpdateTrialStatusInDB mocks base method func (m *MockManagerClient) UpdateTrialStatusInDB(arg0 *v1alpha2.Trial) error { m.ctrl.T.Helper() From 939d9937cb6102c32d403f88b6fbb1a96b6aa8a3 Mon Sep 17 00:00:00 2001 From: Ce Gao Date: Thu, 23 May 2019 10:18:47 +0800 Subject: [PATCH 2/2] fix: Add a log Signed-off-by: Ce Gao --- pkg/controller/v1alpha2/trial/trial_controller_util.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/controller/v1alpha2/trial/trial_controller_util.go b/pkg/controller/v1alpha2/trial/trial_controller_util.go index e8de2610f82..9f043abc6eb 100644 --- a/pkg/controller/v1alpha2/trial/trial_controller_util.go +++ b/pkg/controller/v1alpha2/trial/trial_controller_util.go @@ -87,6 +87,7 @@ func (r *ReconcileTrial) UpdateTrialStatusObservation(instance *trialsv1alpha2.T objectiveMetricName := instance.Spec.Objective.ObjectiveMetricName reply, err := r.GetTrialObservationLog(instance) if err != nil { + log.Error(err, "Get trial observation log error") return err } if reply.ObservationLog != nil {