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

Feat: Expose nodeID to container #388

Closed
wants to merge 2 commits into from

Conversation

fellhorn
Copy link

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.

@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

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

Comparison is base (03dc9db) 63.01% compared to head (524b7cf) 64.15%.
Report is 15 commits behind head on master.

❗ Current head 524b7cf differs from pull request most recent head 352b5d6. Consider uploading reports for the commit 352b5d6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   63.01%   64.15%   +1.13%     
==========================================
  Files         154      156       +2     
  Lines       13080    10712    -2368     
==========================================
- Hits         8243     6872    -1371     
+ Misses       4220     3208    -1012     
- Partials      617      632      +15     
Flag Coverage Δ
unittests ?

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

Files Coverage Δ
go/tasks/pluginmachinery/core/phase.go 21.50% <ø> (-1.27%) ⬇️
go/tasks/pluginmachinery/flytek8s/config/config.go 50.00% <ø> (ø)
...machinery/flytek8s/config/k8spluginconfig_flags.go 51.28% <100.00%> (+3.78%) ⬆️
go/tasks/pluginmachinery/flytek8s/pod_helper.go 71.07% <100.00%> (-3.03%) ⬇️
go/tasks/pluginmachinery/internal/webapi/cache.go 85.71% <100.00%> (+5.91%) ⬆️
go/tasks/pluginmachinery/internal/webapi/state.go 100.00% <ø> (ø)
go/tasks/plugins/array/k8s/subtask_exec_context.go 83.18% <100.00%> (+1.36%) ⬆️
go/tasks/plugins/hive/execution_state.go 71.92% <100.00%> (+1.51%) ⬆️
.../plugins/k8s/kfoperators/common/common_operator.go 68.45% <100.00%> (+4.13%) ⬆️
go/tasks/plugins/k8s/ray/config_flags.go 38.70% <100.00%> (+2.34%) ⬆️
... and 18 more

... and 113 files with indirect coverage changes

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

Signed-off-by: Dennis Keck <[email protected]>
@@ -31,6 +31,15 @@ func GetContextEnvVars(ownerCtx context.Context) []v1.EnvVar {
},
)
}

if nodeID := contextutils.Value(ownerCtx, contextutils.NodeIDKey); nodeID != "" {
Copy link
Contributor

@hamersaw hamersaw Aug 15, 2023

Choose a reason for hiding this comment

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

It looks like this ID will be a parent / child hierarchy separated by '/' (here). Do we instead want the nodeID to be identical to that displayed in the UI? We could use the NodeExecutionID.NodeID and inject here using something like id.GetID().NodeExecutionId.NodeId. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @hamersaw
For our case we need the entire hierarchy, similar to how is is displayed here in the UI:
image

(ideally without it being shortened to a specific length)

id.GetID().NodeExecutionId.NodeId only gives me the child nodeID, e.g. dn0, which might occur multiple times in a workflow. Instead we need the entire hierarchy to uniquely identify a single node in the graph (e.g. n0/dn0/dn0/dn0/dn0)

Copy link
Contributor

Choose a reason for hiding this comment

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

If you use something like:

currentNodeUniqueID, err := common.GenerateUniqueID(nCtx.ExecutionContext().GetParentInfo(), nodeExecutionID.NodeId)

then it should prepend the node ID with the parent info which should give you the n0-0-dn0 / n0-0-dn0-0-dn0 being separated by - rather than /. I think this would be cleaner?

@eapolinario
Copy link
Contributor

eapolinario commented Oct 3, 2023

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to . Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

edit: Re-opening these PRs as the automation failed.

@eapolinario eapolinario closed this Oct 3, 2023
@eapolinario eapolinario reopened this Oct 3, 2023
@eapolinario
Copy link
Contributor

Hi, we are moving all Flyte development to a monorepo. In order to help the transition period, we're moving open PRs to monorepo automatically and your PR was moved to flyteorg/flyte#4135. Notice that if there are any conflicts in the resulting PR they most likely happen due to the change in the import path of the flyte components.

@eapolinario eapolinario closed this Oct 3, 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.

3 participants