-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: log podspec [DET-9861] #8899
Conversation
✅ Deploy Preview for determined-ui canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a minor question / thought
@@ -480,6 +481,7 @@ func (p *pod) createPodSpec(scheduler string) error { | |||
|
|||
p.pod = p.configurePodSpec( | |||
volumes, initContainer, container, sidecars, (*k8sV1.Pod)(env.PodSpec()), scheduler) | |||
p.insertLog(time.Now().UTC(), fmt.Sprintf("Pod Spec Configured:\n%v\n", p.pod.Spec)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the message obnoxiously large? Like do you think it could be annoying for some users?
Maybe we could add the log at a debug level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that if we log it at the debug level it might not be useful enough in a lot of situations. I personally don't think it's too obnoxious, but what do you think? (I'll add a screenshot)
imo it's like how in master logs we print the master spec at startup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d1c3e74
to
ae60716
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8899 +/- ##
==========================================
- Coverage 47.53% 44.22% -3.31%
==========================================
Files 1066 706 -360
Lines 170248 133397 -36851
Branches 2235 2235
==========================================
- Hits 80919 59000 -21919
+ Misses 89171 74239 -14932
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more. |
ae60716
to
eaecd42
Compare
Description
Simply prints out the podspec before the task is run. The message occurs at the start of the task and doesn't interfere with the task logs.
Test Plan
Manually verify: run a task in kubernetes and check that the logs include a string of the podspec.
Commentary (optional)
Checklist
docs/release-notes/
.See Release Note for details.
Ticket