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

feat: add process services to expose process network explicitly #1485

Merged
merged 16 commits into from
Aug 23, 2024

Conversation

jamesgetx
Copy link
Collaborator

No description provided.

"""
val = res.metadata.annotations.get(PROC_SERVICES_ENABLED_ANNOTATION_KEY)
# bkapp.paas.bk.tencent.com/proc-services-feature-enabled: false 时, 表示版本低于 specVersion: 3, 因此向后兼容, 需要部署网络
if val == "false":
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

@jamesgetx jamesgetx Jul 24, 2024

Choose a reason for hiding this comment

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

这里的存量分为线上运行的 bkapp 和 ModuleProcessSpec 中保存的数据:

  • 对于线上运行的 bkapp, 由于没有重新部署,不会有注解内容,因此走旧逻辑。在 operator 中有兼容处理
  • 对于 ModuleProcessSpec 中保存的数据,会变更它的 services 字段数据:比如将存量的 web 进程配置上类型是 bk/http 的 services。本 pr 发布后,所有新部署的 BkApp,都会带上注解内容。 对于这一点,需要实现一个 migration ,所以想着 pr review 完,方案没问题后,补进来

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

迁移逻辑已经补充

apiserver/paasng/paas_wl/bk_app/cnative/specs/resource.py Outdated Show resolved Hide resolved
@@ -154,6 +174,7 @@ class ProcessTmpl:
autoscaling: bool = False
scaling_config: Optional[AutoscalingConfig] = None
probes: Optional[ProbeSet] = None
services: Optional[List[ProcService]] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个 ProcessTmpl 模型越来越复杂了,它目前的用处主要是什么?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dict[str, ProcessTmpl] 被设计保存到了 Deployment model 的 processes 字段中,主要被用在 ModuleProcessSpecManager.sync_from_desc(self, processes: List[ProcessTmpl])

找到了原先的代码说明:

@dataclass
class ProcessTmpl:
    """This class declare a process template which can be used to sync process spec or deploy a process(deployment)

    :param command: 启动指令
    :param replicas: 副本数
    :param plan: 资源方案名称
    """

    name: str
    command: str
    replicas: Optional[int] = None
    plan: Optional[str] = None

    def __post_init__(self):
        self.name = self.name.lower()

我加个 TODO ,看后面处理进程同步的时候,能不能去掉或者替换掉它

r.Annotations = make(map[string]string)
}
if _, ok := r.Annotations[ProcServicesFeatureEnabledAnnoKey]; !ok {
r.Annotations[ProcServicesFeatureEnabledAnnoKey] = "true"
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个 Annot 的目的主要是控制 Operator 生成 services 时的逻辑分支?我感觉它出现频率太高了, 维护起来有点复杂,能不能换一种更简单的方式:

  • 如果整个资源没有 services,走 false 逻辑(按之前的初始化的逻辑)
  • 一旦有任何一个进程有 services,走 true 逻辑
    • 对于未定义任何 services 并且想要走 true 的逻辑,必须通过某个 annot 来强制声明(这种应该非常少)

Copy link
Collaborator Author

@jamesgetx jamesgetx Aug 15, 2024

Choose a reason for hiding this comment

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

对于未定义任何 services 并且要走 true 的场景,是设计 process service 的出发点之一,目的是 后台类型的 process 可以按需分配 4层 service, 不浪费资源。

如果只在特殊情况下有某个注解,会不会反而让这个注解不好理解。其实现在的注解设计还充当点"版本"的标记作用,带有这个注解的,表示已通过新版本部署过

在 paasng 有个处理逻辑:spec_version: 2 版本,注解为 false;specVersion: 3 版本,注解为 true

我先尝试封装下注解的操作,让它更好“维护些”

operator/pkg/controllers/bkapp/processes/services.go Outdated Show resolved Hide resolved
operator/pkg/controllers/dgroupmapping/ingress/builder.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@piglei piglei left a comment

Choose a reason for hiding this comment

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

LGTM

@jamesgetx jamesgetx merged commit f5affe8 into TencentBlueKing:main Aug 23, 2024
7 checks passed
songzxc789 pushed a commit to songzxc789/blueking-paas that referenced this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants