Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Feat: Make pod start time available als logging link template variable in kubeflow plugin #362

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

fg91
Copy link
Member

@fg91 fg91 commented Jun 14, 2023

TL;DR

I'd like to add the filter timestamp >= podStartTime to our stackdriver log link template. This is important because currently our users have to manually extend the time range (default 1h) each time they visit the stackdriver log page. (Unless the job started and finished within the past hour.)

To this aim, I recently made the pod start and finish time available as logging link template variables in RFC3339 time format which is required by google cloud logging (#360).

Currently, the kubeflow plugin doesn't pass the pod start time to the log plugin though.

In this PR I extend the kubeflow plugin to do so.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

NA

Tracking Issue

NA

Follow-up issue

NA

@@ -104,8 +105,11 @@ func GetMPIPhaseInfo(currentCondition commonOp.JobCondition, occurredAt time.Tim
}

// GetLogs will return the logs for kubeflow job
func GetLogs(taskType string, name string, namespace string, hasMaster bool,
func GetLogs(taskType string, objectMeta meta_v1.ObjectMeta, hasMaster bool,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By passing in objectMeta as opposed to individually passing in the name, namespace and in the future also the creation timestamp, we can retrieve the creation timestamp only a single time here instead of doing so in pytorch.go, tensorflow.go, as well as mpi.go.

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #362 (d21d9bd) into master (53e63bc) will increase coverage by 1.37%.
The diff coverage is 100.00%.

❗ Current head d21d9bd differs from pull request most recent head 19a4cca. Consider uploading reports for the commit 19a4cca to get more accurate results

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
+ Coverage   62.67%   64.04%   +1.37%     
==========================================
  Files         152      152              
  Lines       12806    10389    -2417     
==========================================
- Hits         8026     6654    -1372     
+ Misses       4168     3123    -1045     
  Partials      612      612              
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../plugins/k8s/kfoperators/common/common_operator.go 69.07% <100.00%> (+8.83%) ⬆️
go/tasks/plugins/k8s/kfoperators/mpi/mpi.go 75.00% <100.00%> (+2.42%) ⬆️
...o/tasks/plugins/k8s/kfoperators/pytorch/pytorch.go 78.08% <100.00%> (+1.54%) ⬆️
...s/plugins/k8s/kfoperators/tensorflow/tensorflow.go 77.77% <100.00%> (+2.05%) ⬆️

... and 129 files with indirect coverage changes

@fg91 fg91 marked this pull request as ready for review June 14, 2023 22:16
@fg91 fg91 requested a review from hamersaw June 14, 2023 22:16
@hamersaw hamersaw merged commit dfdf6f9 into master Jun 15, 2023
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants