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

[YUNIKORN-2606] Modular sidebar with remote components #198

Closed
wants to merge 2 commits into from

Conversation

dcoric
Copy link
Contributor

@dcoric dcoric commented Aug 7, 2024

Implemented Module Federation for the Applications Drawer component Implemented and fixed all tests.
Added copyright info

What is this PR for?

This PR introduces Module Federation for loading components remotely.
This was intended to easily connect Yunikorn History Server with YuniKorn. Components that should consume data from the Yunikorn History Server will be served from the YHS itself and loaded in the YuniKorn with configuring env parameters on build time.

This PR includes adjustments to the Applications page (apps-view component). The allocations drawer is extracted to a separate component (allocations-drawer component). The apps-view component is loaded dynamically on init in the initializeSidebarComponent method. Depending on the env configuration, that component is loaded locally, or remotely from the YHS server.

With component separation and page updates, the design is slightly tweaked to match more closely to match the design linked in the YUNIKORN-2603

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Add info to the documentation on how to configure the remote module

What is the Jira issue?

YUNIKORN-2606

How should this be tested?

Steps for testing this feature:

  1. Check out the (YuniKorn History Server)[https://github.com/G-Research/yunikorn-history-server]
  2. Go to the web folder and run npm install and npm start. This will start the application with module federation in remote mode that will generate a remoteEntry.js file.
  3. In YuniKorn envconfig.json file set these values:
      "moduleFederationRemoteEntry": "http://localhost:3100/remoteEntry.js",
      "allocationsDrawerRemoteComponent": "./AllocationsDrawerComponent"
    
  4. Start YuniKorn as usual. You will notice that the Allocations Sidebar is a different component that is loaded over the network (remoteEntry.js will be visible in the network). The rest of the application will not be affected in any way.

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.77%. Comparing base (602ca5e) to head (ca02ba4).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #198   +/-   ##
=======================================
  Coverage   38.77%   38.77%           
=======================================
  Files           2        2           
  Lines          49       49           
=======================================
  Hits           19       19           
  Misses         27       27           
  Partials        3        3           

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

@chenyulin0719
Copy link
Contributor

chenyulin0719 commented Aug 11, 2024

@dcoric Thanks for the cool feature.

However, I followed the steps but looks like the Module Federation doesn't work in my environment.

{
  "localSchedulerWebAddress": "http://localhost:9889",
  "moduleFederationRemoteEntry": "http://localhost:3100/remoteEntry.js",
  "allocationsDrawerRemoteComponent": "./AllocationsDrawerComponent"
}
  • After added some logs, I'm sure the initializeSidebarComponent() is loaded and remoteComponentConfig!==null

If possible, could you provide screenshot so I can validate the result. Thanks.

My UI screenshot:
image
My remoteEntry.js screenshot:
image

@chenyulin0719
Copy link
Contributor

I'm not familiar with Module Federation, has anyone else successfully loaded the remote component?

@ryankert01
Copy link
Contributor

I suggest perhaps we can have a video demo?(or screenshots) That way, we can grasp the idea quickly.

@dcoric
Copy link
Contributor Author

dcoric commented Aug 11, 2024

I will recheck it in the new environment in case I have missed something. Video is probably the best option to show all steps

@dcoric
Copy link
Contributor Author

dcoric commented Aug 22, 2024

I had to verify on different systems and environments if this is functioning, and I can confirm that there was no issue with loading remote modules.
Here is a video that shows all the steps for setting up the environment:
https://youtu.be/5QnX6Tv315Y (The video was too big for direct upload to GitHub)

@chenyulin0719
Copy link
Contributor

@dcoric Thanks for providing the demo, that's helpful. After updated the node version I can see the "Log Link" column now.
If I click the 'Logs' link, it will open a new page "localhost:4200/element['applicationId'] ". (YHS code link)

I guess It's what YHS have now, the YHS log display function is under development, am I correct? Or I should see anything else?

@dcoric
Copy link
Contributor Author

dcoric commented Aug 23, 2024

@chenyulin0719 That feature will be added in the future to YHS. I used that as a demo to easily differentiate a remote component from the local one.

Copy link
Contributor

@chenyulin0719 chenyulin0719 left a comment

Choose a reason for hiding this comment

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

@dcoric Overall LGTM, the functionality work as expected. Below new files are missing license.

  1. webpack.config.js
  2. webpack.prod.config.js

The license-check command Makefile is not checking js files, could you add it too?

  • yunikorn-web/Makefile

    Lines 197 to 200 in 3922c51

    ifeq (darwin,$(OS))
    $(shell mkdir -p "$(OUTPUT)" && find -E . -not \( -path './.git*' -prune \) -not \( -path ./coverage -prune \) -not \( -path ./node_modules -prune \) -not \( -path ./build -prune \) -not \( -path ./tools -prune \) -not -path ./pnpm-lock.yaml -regex ".*\.(go|sh|md|conf|yaml|yml|html|mod)" -exec grep -L "Licensed to the Apache Software Foundation" {} \; > "$(OUTPUT)/license-check.txt")
    else
    $(shell mkdir -p "$(OUTPUT)" && find . -not \( -path './.git*' -prune \) -not \( -path ./coverage -prune \) -not \( -path ./node_modules -prune \) -not \( -path ./build -prune \) -not \( -path ./tools -prune \) -not -path ./pnpm-lock.yaml -regex ".*\.\(go\|sh\|md\|conf\|yaml\|yml\|html\|mod\)" -exec grep -L "Licensed to the Apache Software Foundation" {} \; > "$(OUTPUT)/license-check.txt")

@chenyulin0719
Copy link
Contributor

Hi @wilfred-s, Could you take a look at the Module Federation feature? I don't have concern for it. The remote component is only connected if users config yunikorn/yunikorn-web/src/assets/config/envconfig.json

Implemented Module Federation for the Applications Drawer component
Implemented and fixed all tests.
Added copyright info
@dcoric
Copy link
Contributor Author

dcoric commented Sep 10, 2024

@dcoric Overall LGTM, the functionality work as expected. Below new files are missing license.

  1. webpack.config.js
  2. webpack.prod.config.js

The license-check command Makefile is not checking js files, could you add it too?

  • yunikorn-web/Makefile

    Lines 197 to 200 in 3922c51

    ifeq (darwin,$(OS))
    $(shell mkdir -p "$(OUTPUT)" && find -E . -not \( -path './.git*' -prune \) -not \( -path ./coverage -prune \) -not \( -path ./node_modules -prune \) -not \( -path ./build -prune \) -not \( -path ./tools -prune \) -not -path ./pnpm-lock.yaml -regex ".*\.(go|sh|md|conf|yaml|yml|html|mod)" -exec grep -L "Licensed to the Apache Software Foundation" {} \; > "$(OUTPUT)/license-check.txt")
    else
    $(shell mkdir -p "$(OUTPUT)" && find . -not \( -path './.git*' -prune \) -not \( -path ./coverage -prune \) -not \( -path ./node_modules -prune \) -not \( -path ./build -prune \) -not \( -path ./tools -prune \) -not -path ./pnpm-lock.yaml -regex ".*\.\(go\|sh\|md\|conf\|yaml\|yml\|html\|mod\)" -exec grep -L "Licensed to the Apache Software Foundation" {} \; > "$(OUTPUT)/license-check.txt")

I have added missing licenses and updated Makefile to check *.js files as well (with exception to /dist folder)

Copy link
Contributor

@chenyulin0719 chenyulin0719 left a comment

Choose a reason for hiding this comment

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

LGTM

@chenyulin0719
Copy link
Contributor

chenyulin0719 commented Sep 12, 2024

I can foresee some other changes required for 1.6.0 (ex: hide v2 queue).
Will merge it after 1.6.0 released.

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.

4 participants