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

change the label of auto-created ippool for GC needs #1162

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

Icarus9913
Copy link
Collaborator

@Icarus9913 Icarus9913 commented Dec 9, 2022

⚠️⚠️⚠️ This change is make it incompatible with the previous release version
(The ipam and spiderpool-controller will use this label to find the application corresponding auto-created IPPool, once we change it. It's incompatible with the previous version spiderpool)

⚠️⚠️⚠️ For upgrade pre-steps, please check upgrade.md

The correlative PR: #1135 implements auto-created IPPool garbage collect, it will use the IPPool label ipam.spidernet.io/owner-application to trace back the corresponding application.
But the current function implementation just use '-' to assemble the application identification.

// AppLabelValue will joint the application type, namespace and name.
// the format is "{appKind}-{appNS}-{appName}"
func AppLabelValue(appKind string, appNS, appName string) string {
	return fmt.Sprintf("%s-%s-%s", appKind, appNS, appName)
}

For this, it's very hard to unpack every variable.
For instance, you could set the namespace as ns398-174835790 and the deployment name deploy398-82311862. And it will return a string like Deployment-ns398-174835790-deploy398-82311862

I suggest to use "_" rather than "-" as slash for spider subnet feature 'ipam.spidernet.io/owner-application' label value.
And the upper case will be Deployment_ns398-174835790_deploy398-82311862

Signed-off-by: Icarus9913 icaruswu66@qq.com

What this PR does / why we need it:
fix bug

Which issue(s) this PR fixes:
#1161 (comment)

@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #1162 (0fdcbba) into main (5eda988) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1162   +/-   ##
=======================================
  Coverage   78.85%   78.85%           
=======================================
  Files          19       19           
  Lines        1296     1296           
=======================================
  Hits         1022     1022           
  Misses        250      250           
  Partials       24       24           
Flag Coverage Δ
unittests 78.85% <ø> (ø)

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

@Icarus9913 Icarus9913 added issue/bug pr/ready-review This pull is ready for review and removed pr/not-ready not ready for merging labels Dec 9, 2022
@Icarus9913 Icarus9913 linked an issue Dec 9, 2022 that may be closed by this pull request
iiiceoo
iiiceoo previously approved these changes Dec 9, 2022
@iiiceoo iiiceoo removed the issue/bug label Dec 9, 2022
@weizhoublue
Copy link
Collaborator

weizhoublue commented Dec 9, 2022

现在版本 不能 狂奔了,应该要正规起来

第一,如果是bug fix,没有 pick 到 release 0.3, 但 release 0.3 是 不兼容的, release 0.3 怎么办?

第二,为什么 label 打的是 “none-required” ,非常 随意 ,release note怎么管理

第三,描述 没说清 "-" 为什么会引起bug 。并且 title 和 描述 只顾 说 代码 做了什么变动,elease note 中,参考者 更多是想看到 什么问题症状,解决了什么问题,才能知道这个版本 修复了它的困境 。
终归, 这个 title 和 描述 作为 release note ,对 事后溯源的参考者 没 起到 直接帮助
最后,每个发版的 release note 只有 研发人员才看得懂 修了什么问题
一个合格的release note,似乎应该描述:fix: running pod fails when pod name contain special character

@Icarus9913 Icarus9913 changed the title do refactor for spider subnet feature label fix application label tracing and do refactor for spider subnet feature label Dec 10, 2022
@weizhoublue
Copy link
Collaborator

weizhoublue commented Dec 10, 2022

(1)已经 发出去的 版本,已经 创建的 ippool 没有这种label,升级后是否存 识别 问题,例如 IP 分配 失败。怎么进行 升级兼容 ?或者说 没法升级 兼容 。
如是,这会是 V0.3 to V0.4 一个重大的 设计变更,导致 不能兼容,势必得 release note 说明

(2)fix application label tracing and do refactor for spider subnet feature label
----- 本质bug是 ippool 的label 设计 有 问题,修改设计后,避免 IPAM 无法 根据 自动创建池 ippool 的 label 为 应用 分配 IP 或回收 ippool ? 使用者 或者 项目 维护者 后续希望 根据 pr title 找到当初的问题修复
do refactor for spider subnet feature label 这是 程序员的 语言 ,不适合做 title,只适合做描述,否则 第三阅读者 基本看不懂 或者不知道 意味着什么,就不会点击进入 pr 查看细节
title 更加适合 说明 业务功能 或者 问题现象之类的

@Icarus9913 Icarus9913 changed the title fix application label tracing and do refactor for spider subnet feature label subnet: fix deleting auto-created IPPool by mistake Dec 12, 2022
@Icarus9913 Icarus9913 added cherrypick-release-v0.3 Cherry-pick the PR to branch release-v0.3. pr/release/feature-new and removed pr/release/none-required pr/ready-review This pull is ready for review cherrypick-release-v0.3 Cherry-pick the PR to branch release-v0.3. labels Dec 12, 2022
@Icarus9913 Icarus9913 added pr/release/feature-new pr/ready-review This pull is ready for review labels Dec 12, 2022
@Icarus9913 Icarus9913 added the cherrypick-release-v0.3 Cherry-pick the PR to branch release-v0.3. label Dec 12, 2022
@weizhoublue weizhoublue changed the title subnet: fix deleting auto-created IPPool by mistake change the label of auto-created ippool for GC needs Dec 12, 2022
@Icarus9913 Icarus9913 force-pushed the fix/wk/subnet-label branch 2 times, most recently from ff38467 to b73c976 Compare December 12, 2022 08:00
iiiceoo
iiiceoo previously approved these changes Dec 12, 2022
@@ -0,0 +1,27 @@
# Spiderpool upgrade

## Notice
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个不能这么写,
如果是所有升级注意 后续 都在 一个文件里,2级别标题 最好是 “低于 v0.3.x 升级到 高于 xx 版本 之类” 的 分类标题,
或者 按照 版本 来规划文件名。


## Description

There's a design flaw for SpiderSubnet feature in auto-created IPPool label.
Copy link
Collaborator

Choose a reason for hiding this comment

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

附上 链上 pr 链接 做细节参考

@@ -0,0 +1,27 @@
# Spiderpool upgrade
Copy link
Collaborator

Choose a reason for hiding this comment

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

新增文件,没看到 更新 docs/mkdocs.yml

2.add upgrade markdown

Signed-off-by: Icarus9913 <icaruswu66@qq.com>
@sonarcloud
Copy link

sonarcloud bot commented Dec 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
1.5% 1.5% Duplication

Please consult the segments from your current release until now before upgrading your spiderpool.


## Upgrade to 0.3.6 from (<=0.3.5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

0.3.5- to 0.4.0 也是 这个 case, 但不符合这个 title
这样是否合适
upgrade from version <= 0.3.5 to any high version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick-release-v0.3 Cherry-pick the PR to branch release-v0.3. pr/ready-review This pull is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Night CI 2022-12-08: Failed, the automatic pool label deparse fails.
3 participants