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
Closed

Feat: Expose nodeID to container #388

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc10b27
Feat: Expose nodeID to container
fellhorn Aug 14, 2023
352b5d6
Linting
fellhorn Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,15 @@
},
)
}

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

@hamersaw hamersaw Aug 15, 2023 •
edited
Loading

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?

envVars = append(envVars,
v1.EnvVar{
Name: "FLYTE_INTERNAL_NODE_ID",
Value: nodeID,
},
)
}

Check warning on line 42 in go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go

View check run for this annotation

Codecov / codecov/patch

go/tasks/pluginmachinery/flytek8s/k8s_resource_adds.go#L36-L42 

Added lines #L36 - L42 were not covered by tests
return envVars
}

Expand Down
Loading