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

Add upgrade test for spiderpool #3323

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Conversation

ty-dc
Copy link
Collaborator

@ty-dc ty-dc commented Mar 19, 2024

Thanks for contributing!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@ty-dc ty-dc requested a review from weizhoublue as a code owner March 19, 2024 10:06
@ty-dc ty-dc added the pr/not-ready not ready for merging label Mar 19, 2024
@ty-dc ty-dc force-pushed the ci/upgrade_test branch 11 times, most recently from 9d54e5d to 6ddefba Compare March 21, 2024 02:12
@ty-dc
Copy link
Collaborator Author

ty-dc commented Mar 29, 2024

wait #3322

@weizhoublue weizhoublue closed this Apr 7, 2024
@weizhoublue weizhoublue reopened this Apr 7, 2024
@ty-dc ty-dc force-pushed the ci/upgrade_test branch 14 times, most recently from e94cb76 to c3f2fd7 Compare April 12, 2024 02:14
@ty-dc ty-dc force-pushed the ci/upgrade_test branch 4 times, most recently from 7b23b35 to 9312d4d Compare April 12, 2024 07:10
@ty-dc ty-dc added release/none no release note and removed pr/not-ready not ready for merging labels Apr 12, 2024
@ty-dc ty-dc force-pushed the ci/upgrade_test branch 2 times, most recently from 39215c6 to 87d2a8b Compare April 12, 2024 08:01

workflow_dispatch:
inputs:
ref:
Copy link
Collaborator

Choose a reason for hiding this comment

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

如果是手动触发的,那么应该是 手动指定某个老版本、或者默认上一个版本

KUBECONFIG_PATH: /home/runner/work/spiderpool/spiderpool/test/.cluster
on:
schedule:
- cron: "0 20 * * *"
Copy link
Collaborator

@weizhoublue weizhoublue Apr 15, 2024

Choose a reason for hiding this comment

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

如果是自动触发的,我认为 可以是 当前 main 分支的版本的 上一个 最新版本,即可

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

fail-fast: false
matrix:
# Synchronise with the latest releases of each version
version: [release-v0.7, release-v0.8]
Copy link
Collaborator

Choose a reason for hiding this comment

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

我认为不需要 手动维护 这个,也不需要覆盖很多老的,只要测试到 前后两个版本能顺利升级

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

维护的是 release-v0.7、release-0.8、release-0.9 是否应该考虑这几个版本都能升级到最新版本?

Copy link
Collaborator

@weizhoublue weizhoublue Apr 16, 2024

Choose a reason for hiding this comment

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

我不认为 这里需要人工维护是个号注意,如果CI 本身证明了 7-8 顺利,8-9 顺利,可能就意味着 7-9顺利 或者有 7-8-9的升级通道
即使是社区的大型项目也不保证 跨多个版本能升级

Copy link
Collaborator

@weizhoublue weizhoublue Apr 16, 2024

Choose a reason for hiding this comment

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

并且这里 没有覆盖 v0.7.0 - v0.7.1 , v0.7.5 - v0.8.0 这些升级情况
所以我认为 考虑 当前代码分支的 上一个最新发布,是最基本的诉求,当前这个诉求还没满足

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

并且这里 没有覆盖 v0.7.0 - v0.7.1 , v0.7.5 - v0.8.0 这些升级情况 所以我认为 考虑 当前代码分支的 上一个最新发布,是最基本的诉求,当前这个诉求还没满足

1

ref: ${{ needs.get_ref.outputs.ref }}
fetch-depth: 0

- name: Move kubeconfig to branch ${{ needs.get_ref.outputs.ref }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里可以替换 新老版本的 变量名,便于 代码阅读
oldVersion
upgradeVersion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

.PHONY: helm_upgrade_spiderpool
helm_upgrade_spiderpool:
@echo -e "\033[35m [helm upgrade spiderpool] \033[0m"
kubectl delete po -n $(RELEASE_NAMESPACE) spiderpool-init --kubeconfig $(E2E_KUBECONFIG) || true ;\
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个是升级的必须动作么 ? 有体现在 文档中 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

文档里面有。

Copy link
Collaborator

@weizhoublue weizhoublue Apr 16, 2024

Choose a reason for hiding this comment

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

文档里说 “删除 spiderpool-init Pod(可选)-- 如果您的环境有 spiderpool-init Pod (默认其不是一定有么) ---- 将其删除,否则在 helm upgrade 时,会失败 (没解释为什么 有 会导致失败) ”
那用户如何判断 是否 可选

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1

@echo -e "\033[35m [helm upgrade spiderpool] \033[0m"
kubectl delete po -n $(RELEASE_NAMESPACE) spiderpool-init --kubeconfig $(E2E_KUBECONFIG) || true ;\
HELM_OPTION="";\
HELM_OPTION+=" --set spiderpoolController.replicas=1 " ; \
Copy link
Collaborator

Choose a reason for hiding this comment

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

既然是升级,为什么不是标准的 --resuse-values ,还需要设置这么多

Copy link
Collaborator Author

@ty-dc ty-dc Apr 15, 2024

Choose a reason for hiding this comment

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

之前考虑的跨版本升级

有些新增 crd ,所以要删下 spiderpool-init

其次,如果默认是 --resuse-values ,很多情况 Pod 起不来。
如:make setup 设置的 controller pod 是两个副本,如果 --resuse-values ,升级后 controller Pod 还是两个副本,那么会出现 端口占用,新的 controller pod 无法运行。

Copy link
Collaborator

Choose a reason for hiding this comment

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

我认为这部分代码就是和标准的升级文档是一致的
(1)有些新增 crd ,所以要删下 spiderpool-init ---- 如果是标准流程,升级文档有体现么
(2)--resuse-values ,很多情况 Pod 起不来 --- 那客户怎么办 ? 升级文档如何体现

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@ty-dc ty-dc force-pushed the ci/upgrade_test branch 2 times, most recently from d347280 to 4b3bf28 Compare April 18, 2024 08:10
@ty-dc ty-dc requested a review from windsonsea as a code owner April 18, 2024 08:10
@ty-dc ty-dc force-pushed the ci/upgrade_test branch 2 times, most recently from bcfd18b to aa60c6a Compare April 18, 2024 10:23
@ty-dc ty-dc force-pushed the ci/upgrade_test branch 2 times, most recently from ca16cb0 to 2b2b103 Compare April 23, 2024 07:19
Signed-off-by: tao.yang <tao.yang@daocloud.io>
@weizhoublue weizhoublue merged commit 7330e6a into spidernet-io:main Apr 25, 2024
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants