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

Support topology v2 #2598

Merged

Conversation

andrewballantyne
Copy link
Member

@andrewballantyne andrewballantyne commented Mar 13, 2024

https://issues.redhat.com/browse/RHOAIENG-2297

Description

Basically this will bring back the horizontal topology using DSPv2 data. It refactors a lot of what was in the sidebar and has some edge cases left. We may need to merge this as first implementation (after some cleanup) and then address reworking the artifact nodes to properly show up.

Screenshots

Pipeline
image

Run screen
Screenshot 2024-03-14 at 1 49 14 AM
Screenshot 2024-03-14 at 1 49 25 AM
Screenshot 2024-03-14 at 1 56 19 AM

Group nodes, not supported today (or for v1=>v2 parity)
Screenshot 2024-03-14 at 1 56 37 AM
Screenshot 2024-03-14 at 1 56 43 AM

Schedules
Screenshot 2024-03-14 at 1 57 02 AM
Screenshot 2024-03-14 at 1 57 12 AM

Gif

Notes:

  • [Bug] Seems the logs are not properly found -- will need to look into this (seems to be an issue more with auto pruning)
  • We don't have a good way to see what's going on -- maybe the above bug will lead to a better status update
Screen.Recording.2024-03-14.at.2.02.48.AM.mov

How Has This Been Tested?

You should be able to view any:

  • Pipeline Details
  • Run Details
  • Schedule Details

And get a topology graph. Statuses should reflect executions but have some quirks to them (see description).

Test Impact

WIP

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Mar 13, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Mar 13, 2024
@andrewballantyne andrewballantyne force-pushed the support-topology-v2 branch 4 times, most recently from 6d24e9e to 55287af Compare March 14, 2024 05:57
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Mar 14, 2024
@andrewballantyne
Copy link
Member Author

@yannnz bit of a work in progress still -- but this is where we are at with Topology. Group Nodes is probably the biggest gap. I know we are probably not showing artifact sidebars quite perfectly -- we can iterate on these.

@andrewballantyne andrewballantyne requested review from Gkrumbach07 and jpuzz0 and removed request for lucferbux and manaswinidas March 14, 2024 06:05
@andrewballantyne
Copy link
Member Author

@Gkrumbach07 @jpuzz0 feel free to start reviewing this... we may need to fix up some of it next week. I'll try to get more of it updated tomorrow.

Copy link
Member Author

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

State of things...

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO

Copy link
Member Author

Choose a reason for hiding this comment

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

probably should delete all Tekton items -- but we may need it later for Edge -- so I'm torn tbh.

frontend/src/pages/ApplicationsPage.tsx Outdated Show resolved Hide resolved
@andrewballantyne andrewballantyne force-pushed the support-topology-v2 branch 2 times, most recently from d426c6c to b80be31 Compare March 14, 2024 20:45
@andrewballantyne
Copy link
Member Author

Seems pods are having some issues getting proper logs -- so I've started a thread with the backend team.

We should be down to a minor number of known issues. @Gkrumbach07 please take a look when you get a moment.

// [PipelineRunNodeTabs.VISUALIZATIONS]: <>TBD 2</>,
[PipelineRunNodeTabs.DETAILS]: <SelectedNodeDetailsTab task={task} />,
[PipelineRunNodeTabs.VOLUMES]: <SelectedNodeVolumeMountsTab task={task} />,
// [PipelineRunNodeTabs.VOLUMES]: <SelectedNodeVolumeMountsTab task={task} />,
Copy link
Member

Choose a reason for hiding this comment

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

why is volumes removed

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not found a way to show them, if you have a Pipeline for them, let me know!

Copy link
Member Author

Choose a reason for hiding this comment

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

Found one, will implement again

Copy link
Member Author

Choose a reason for hiding this comment

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

#2598 (comment) yes, I am aware, fixing. They do weird spec things when it has volumes.


const { task_details: taskDetails } = runDetails;

const thisTaskDetail = taskDetails.find(({ display_name: name }) => name === `${taskId}-driver`);
Copy link
Member

Choose a reason for hiding this comment

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

you only look at driver. I cannot get an error node to apear then. For some reason if a node has an error, it is not setting the state for the driver task, just the children ones

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, seems I might need to find a bunch of them -- I didn't at the start, but it doesn't show active states if I don't.

@Gkrumbach07
Copy link
Member

ran into this when looking at a run with volumes
image

@andrewballantyne andrewballantyne force-pushed the support-topology-v2 branch 3 times, most recently from 500a2f5 to 760c02b Compare March 15, 2024 18:49
@Gkrumbach07
Copy link
Member

i get this type of run graph
image
result yaml: https://gist.github.com/Gkrumbach07/c953f2175e5c97c05fc51391144cde27

@andrewballantyne
Copy link
Member Author

andrewballantyne commented Mar 15, 2024

/retest

Seems like Cypress is having some issues.

(got a TS error locally, fixed it -- not sure why I didn't see that on the cluster before Cypress)

@andrewballantyne andrewballantyne changed the title [WIP] Support topology v2 Support topology v2 Mar 15, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Mar 15, 2024
@andrewballantyne
Copy link
Member Author

andrewballantyne commented Mar 15, 2024

Known gaps (cc @Gkrumbach07):

  • Missing tests for the topology utilities
  • Pipelines are still a little weird at run time, but work
  • Volumes are missing

@Gkrumbach07
Copy link
Member

This looks good to me. The known gaps are fine for now until we get the new topology rendering in.

/lgtm

@andrewballantyne
Copy link
Member Author

/approve

Copy link
Contributor

openshift-ci bot commented Mar 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit ac47429 into opendatahub-io:main Mar 15, 2024
6 checks passed
@andrewballantyne andrewballantyne deleted the support-topology-v2 branch June 26, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants