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

Linux windows hybrid deploy #8504

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

Lyndon-Li
Copy link
Contributor

@Lyndon-Li Lyndon-Li commented Dec 10, 2024

Fix issue #8416, #8417, deploy Velero server and node-agent in linux/Windows hybrid env:

  • Velero server is restricted to run on linux nodes only
  • node-agent is divided into two instances. node-agent instance runs in linux nodes only and node-agent-windows runs in Windows only
  • node-agent daemonset is deployed when --use-node-agent is specified; node-agent-windows daemonset is deployed when --use-node-agent-windows is specified
  • In a pure linux nodes cluster, desiredNumberScheduled for node-agent-windows is 0
  • In a pure Windows nodes cluster, desiredNumberScheduled for node-agent is 0
  • In a hybrid cluster, desiredNumberScheduled for node-agent-windows is the number of Windows nodes; desiredNumberScheduled for node-agent is the number of linux nodes

Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@Lyndon-Li Lyndon-Li force-pushed the linux-windows-hybrid-deploy branch from 05c02d9 to 8168ce2 Compare December 10, 2024 11:04
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 52.33645% with 51 lines in your changes missing coverage. Please review.

Project coverage is 59.19%. Comparing base (010fd1c) to head (4ad9c24).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
pkg/cmd/server/server.go 0.00% 19 Missing ⚠️
pkg/cmd/cli/nodeagent/server.go 0.00% 17 Missing ⚠️
pkg/cmd/cli/install/install.go 0.00% 8 Missing ⚠️
pkg/install/resources.go 53.84% 4 Missing and 2 partials ⚠️
pkg/nodeagent/node_agent.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8504      +/-   ##
==========================================
+ Coverage   59.18%   59.19%   +0.01%     
==========================================
  Files         369      369              
  Lines       39329    39408      +79     
==========================================
+ Hits        23276    23327      +51     
- Misses      14585    14612      +27     
- Partials     1468     1469       +1     

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

@Lyndon-Li Lyndon-Li force-pushed the linux-windows-hybrid-deploy branch from 8168ce2 to 9f196b5 Compare December 10, 2024 11:26
@Lyndon-Li Lyndon-Li force-pushed the linux-windows-hybrid-deploy branch from 9f196b5 to 2ed820d Compare December 10, 2024 11:41
@Lyndon-Li Lyndon-Li marked this pull request as ready for review December 11, 2024 02:28
@Lyndon-Li Lyndon-Li requested a review from reasonerjt December 11, 2024 02:28
Cache: cacheOption,
})

var mgr manager.Manager
Copy link
Contributor Author

@Lyndon-Li Lyndon-Li Dec 11, 2024

Choose a reason for hiding this comment

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

This retry is to resolve the problem that in some Windows env, the pod network is not immediately ready. Without this, the pod may restart for several times

@Lyndon-Li
Copy link
Contributor Author

Lyndon-Li commented Dec 11, 2024

Technically, we don't need to distinguish different OS types for install, node-agent and node-agent-windows ds instance could be installed in both OS all the time.
I don't see any problem about this, but let me know if you have any concern about the same. @sseago @shubham-pampattiwar @kaovilai @reasonerjt @blackpiglet @anshulahuja98

@kaovilai
Copy link
Member

I don't see a problem with 0 scheduled daemonsets. It's not taking up any major resources.

pkg/install/resources.go Outdated Show resolved Hide resolved
@reasonerjt
Copy link
Contributor

reasonerjt commented Dec 16, 2024

Thanks @Lyndon-Li
I have 2 overall comments:

  1. I think we can add the labels/annotations to make sure velero-server is always running on Linux worker-node.
  2. We should add a flag like " --windows-support" and only when this flag is true we deploy the daemonset for Windows so by default everything stays the same.

@Lyndon-Li
Copy link
Contributor Author

Lyndon-Li commented Dec 17, 2024

Thanks @Lyndon-Li I have 2 overall comments:

  1. I think we can add the labels/annotations to make sure velero-server is always running on Linux worker-node.
  2. We should add a flag like " --windows-support" and only when this flag is true we deploy the daemonset for Windows so by default everything stays the same.

As the discussion:
For 1, we will force Velero to run in linux nodes only. We will add a clarification in release notes --- Velero requires at least one linux worker node in order to run with Windows nodes.
For 2, we will add a flag --use-node-agent-windows to install node-agent-windows daemonset.

@Lyndon-Li Lyndon-Li force-pushed the linux-windows-hybrid-deploy branch from 2ed820d to f94b6a7 Compare December 17, 2024 04:59
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@Lyndon-Li Lyndon-Li force-pushed the linux-windows-hybrid-deploy branch from f94b6a7 to 11cd6d9 Compare December 17, 2024 05:05
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@Lyndon-Li Lyndon-Li force-pushed the linux-windows-hybrid-deploy branch from d4e491e to fe0a45e Compare December 17, 2024 05:38
@Lyndon-Li Lyndon-Li requested a review from ywk253100 December 17, 2024 05:57
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@Lyndon-Li Lyndon-Li force-pushed the linux-windows-hybrid-deploy branch from 79ff23d to 4ad9c24 Compare December 18, 2024 02:50
@Lyndon-Li Lyndon-Li merged commit ea93c00 into vmware-tanzu:main Dec 20, 2024
42 of 43 checks passed
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.

4 participants