Skip to content
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

Moving from flyteplugins - Feat: Expose nodeID to container #4135

Closed
wants to merge 3 commits into from

Conversation

eapolinario
Copy link
Contributor

TL;DR

Exposes the NodeId to the kubernetes Pod as environment variable.

- name: FLYTE_INTERNAL_NODE_ID
  value: n0/dn0/dn0/dn0/dn0/dn0/dn0/dn0/dn0/dn0

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

Currently the environment variables do not provide enough information to know the exact task being executed.

For example we intended to create a unique ID for all retries of a task. Without the nodeID this is not possible as tasks might be reused.

One can retrieve this information also from the hostname but this fails for long names due to k8s pod name length limits.

fellhorn and others added 3 commits August 14, 2023 16:15
Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>
Signed-off-by: Dennis Keck <26092524+fellhorn@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (d9586b0) 58.98% compared to head (0758cc5) 59.30%.

❗ Current head 0758cc5 differs from pull request most recent head 6c24344. Consider uploading reports for the commit 6c24344 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4135      +/-   ##
==========================================
+ Coverage   58.98%   59.30%   +0.32%     
==========================================
  Files         619      550      -69     
  Lines       52804    39700   -13104     
==========================================
- Hits        31146    23545    -7601     
+ Misses      19174    13834    -5340     
+ Partials     2484     2321     -163     
Flag Coverage Δ
unittests ?

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

Files Coverage Δ
...asks/pluginmachinery/flytek8s/k8s_resource_adds.go 91.00% <0.00%> (-6.68%) ⬇️

... and 555 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fg91
Copy link
Member

fg91 commented Oct 4, 2023

Original PR was #4135. Closing for now as we discussed with @hamersaw that a different approach should be chosen.

@fg91 fg91 closed this Oct 4, 2023
@fellhorn
Copy link
Contributor

fellhorn commented Oct 4, 2023

Will open a new PR once I were able to refactor it in such a way that the ID is the same as displayed in the UI (without truncation)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants