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

Fixes a issue with multus and webhook not working when the manifest file is restored during a helm update #4361

Closed

Conversation

cyclinder
Copy link
Collaborator

@cyclinder cyclinder commented Dec 4, 2024

Thanks for contributing!

Notice:

What issue(s) does this PR fix:

Fixes #4357
Fixes #4344

Special notes for your reviewer:

When the admin executes the helm upgrade for spiderpool, the configMap spiderpool-multus, webhookconfig and secrets these manifest files are restored. which can raises issues: #4357 and #4344.

To solve these issues, we add helm hook annotations("helm.sh/hook": pre-install) to these manifests, when helm upgrade spiderpool, these files wouldn't be updated.

执行 Helm 更新时,一些资源如 configMap, webhookconfig, secret 会被 helm 还原为初始配置,但这些资源都会在 spiderpool 运行时动态更新,所以当 helm 更新后,配置被还原这导致 pod webhook 和 multus 无法工作。

所以在这些资源清单文件上添加 helm hook 的 annotation: helm.sh/hook": pre-install, 这期望这些资源只会在安装时下发,而不会在 helm update 时重新下发。

但这里会有一个其他的问题:如果老版本没有如上的 annotation,更新上来后存量的 configMap, webhookconfig, secret 会被删除(这是helm的机制,如果老版本有如上的 annotations,问题不存在)。所以为了避免该情况下,这些资源被 helm 删除,加上另外一个 annotation: helm.sh/resource-type: keep, 但这会导致这些资源残留。所以在 delete-hook 中清理这些资源。

@cyclinder cyclinder added release/bug cherrypick-release-v0.8 Cherry-pick the PR to branch release-v0.8. cherrypick-release-v0.9 cherrypick-release-v1.0 Cherry-pick the PR to branch release-v1.0. labels Dec 4, 2024
@cyclinder cyclinder changed the title Exclude multus configmap & secret & webhookconfig to helm upgrade Fixes a issue with multus and webhook not working when the manifest file is restored during a helm update Dec 4, 2024
@cyclinder
Copy link
Collaborator Author

有个问题:执行升级时,如果之前老版本这些资源没有这个 annotation hook,会导致升级时这些资源被删除

@cyclinder cyclinder force-pushed the charts/fix_helm_upgrade branch from ebbbde5 to d1d873b Compare December 4, 2024 10:28
@weizhoublue
Copy link
Collaborator

很多文档中提到的 升级 操作 helm upgrade , 没带 --reuse-values

@weizhoublue
Copy link
Collaborator

升级 ci 为什么没有测试出该问题

@ty-dc
Copy link
Collaborator

ty-dc commented Dec 6, 2024

升级 ci 为什么没有测试出该问题

升级 CI 这里判断 overlay 和 underlay,--set 显示指定了defaultCniCRName,helm upgrade 时这个值就不会丢。

	if [ "$(INSTALL_OVERLAY_CNI)" == "true" ]; then \
		HELM_OPTION+=" --set multus.multusCNI.defaultCniCRName= " ; \
	else \
		HELM_OPTION+=" --set multus.multusCNI.defaultCniCRName=$(MULTUS_DEFAULT_CNI_VLAN0) " ; \
	fi ; \

@weizhoublue
Copy link
Collaborator

weizhoublue commented Dec 6, 2024

升级 ci 为什么没有测试出该问题

升级 CI 这里判断 overlay 和 underlay,--set 显示指定了defaultCniCRName,helm upgrade 时这个值就不会丢。

	if [ "$(INSTALL_OVERLAY_CNI)" == "true" ]; then \
		HELM_OPTION+=" --set multus.multusCNI.defaultCniCRName= " ; \
	else \
		HELM_OPTION+=" --set multus.multusCNI.defaultCniCRName=$(MULTUS_DEFAULT_CNI_VLAN0) " ; \
	fi ; \

让这个 升级测试模拟真实行为,不要 进行定制,脱离实际测试的意义

@cyclinder
Copy link
Collaborator Author

但以前也没测出来证书被修改的问题。

@weizhoublue
Copy link
Collaborator

this PR should be abandoned , right

@cyclinder
Copy link
Collaborator Author

#4420 and #4393 will fix it.

@cyclinder cyclinder closed this Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick-release-v0.8 Cherry-pick the PR to branch release-v0.8. cherrypick-release-v0.9 cherrypick-release-v1.0 Cherry-pick the PR to branch release-v1.0. release/bug
Projects
None yet
3 participants