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

optimize scale-up and delete node #2185

Merged
merged 2 commits into from
Apr 28, 2023
Merged

Conversation

wizardlyk
Copy link
Collaborator

@wizardlyk wizardlyk commented Apr 13, 2023

Describe what this PR does / why we need it

scale-up/delete new node will not depend on all existing nodes reachable.

Does this pull request fix one issue?

Fixes #1998

Describe how you did it

Describe how to verify it

Special notes for reviews

@wizardlyk wizardlyk changed the title optimize scale-up and delete new node optimize scale-up and delete node Apr 13, 2023
@wizardlyk wizardlyk force-pushed the fix/scale-up branch 2 times, most recently from 0e88334 to 9f42c20 Compare April 13, 2023 08:42
@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023

Codecov Report

Patch coverage has no change and project coverage change: -6.54 ⚠️

Comparison is base (eeede74) 20.01% compared to head (85974e5) 13.48%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2185      +/-   ##
==========================================
- Coverage   20.01%   13.48%   -6.54%     
==========================================
  Files          98      263     +165     
  Lines        9182    22677   +13495     
==========================================
+ Hits         1838     3057    +1219     
- Misses       7090    19209   +12119     
- Partials      254      411     +157     
Flag Coverage Δ
e2e-tests 9.05% <ø> (?)
unit-tests 19.93% <0.00%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
pkg/cluster-runtime/scale.go 0.00% <0.00%> (ø)
pkg/cluster-runtime/uninstall.go 0.00% <0.00%> (ø)
pkg/cluster-runtime/utils.go 0.00% <0.00%> (ø)

... and 165 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Stevent-fei
Copy link
Collaborator

learned, very strong🐂🍺

Copy link
Collaborator

@Stevent-fei Stevent-fei left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@cubxxw cubxxw left a comment

Choose a reason for hiding this comment

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

LGTM

@VinceCui
Copy link
Collaborator

以下几个问题:configureLvs

@VinceCui
Copy link
Collaborator

以下几个问题:

  1. configureLvs这个方法,在master节点没有新增/减少时,不应该在旧的worker节点去触发,因为没有任何改变,触发是没有意义的徒增复杂性
  2. 在1的基础上,扩容新节点时,不应该依赖旧的节点的密码,也就是说,你不应该忽略密码不对的那个错误,而应该是压根不会用旧节点的密码。可以摸一下扩容新节点时,哪些地方需要ssh到旧节点,列出来我们一起评估一下。

logrus.Infof("failed to ssh host %s, your ssh connet has timed out. skip flush ipvs policy on %s", n, n)
logrus.Debugf("ssh err: %v", err)
return nil
} else if strings.Contains(err.Error(), "unable to authenticate") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里有问题,首先,这里不应该走到ssh旧节点的逻辑,其次,ssh新节点时,如果密码失败,此处应该报错。

@wizardlyk
Copy link
Collaborator Author

  1. configurelvs调用,发生在install、scale-up、delete、upgrade、rollback这些command中;install保持不变,scale-up和delete增加判断 master节点有没有新增/减少,upgrade、rollback则不执行configurelvs。
  2. 扩容时只有configurelvs在ssh旧节点。

@kakaZhou719
Copy link
Member

kakaZhou719 commented Apr 18, 2023

  1. 只有master节点变更的时候,需要在旧的node节点上更新 新的LVS real server endpoint(新master ip),需要连通旧节点。
  2. 只有node节点变更的时候,需要在新的node节点上配置,lvs规则,无需连通旧节点。
  3. 同时有master和node变更的时候,那么新节点,旧节点就同时都需要连通。(目前应该不支持)

@wizardlyk wizardlyk force-pushed the fix/scale-up branch 2 times, most recently from c9f5056 to 8f7b369 Compare April 24, 2023 07:59
Signed-off-by: wb-lyk925458 <wb-lyk925458@alibaba-inc.com>
Copy link
Collaborator

@VinceCui VinceCui left a comment

Choose a reason for hiding this comment

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

LGTM

@VinceCui VinceCui merged commit dd72089 into sealerio:main Apr 28, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants