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

feature: 插件包管理支持对象存储模式 (#2) #17

Conversation

ZhuoZhuoCrayon
Copy link
Member

@ZhuoZhuoCrayon ZhuoZhuoCrayon commented Aug 17, 2021

issue #2

相关工作:

  • 将耦合的本地文件操作,基于Django Storage API 抽象,提供更灵活的替换文件源方式
  • 插件包上传 -> 制作 -> 导出 相关文件操作的改造
  • 对类似实现及字面量进行整合提取,并完善了utils的单元测试
  • 优化原有的逻辑

改造范围:

wecom-temp-0c7e56e641a5577b125751560d24dab4

基本思路:

wecom-temp-c445f05f0d6b92921ca9e0d9b91c0864

@ZhuoZhuoCrayon ZhuoZhuoCrayon self-assigned this Aug 17, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2021

Codecov Report

Merging #17 (9c006ed) into V2.1.X_develop (f576c6d) will increase coverage by 26.71%.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           V2.1.X_develop      #17       +/-   ##
===================================================
+ Coverage           59.00%   85.71%   +26.71%     
===================================================
  Files                 390        1      -389     
  Lines               25738        7    -25731     
===================================================
- Hits                15186        6    -15180     
+ Misses              10552        1    -10551     
Impacted Files Coverage Δ
manage.py 85.71% <0.00%> (-14.29%) ⬇️
apps/utils/orm.py
apps/backend/subscription/steps/adapter.py
pipeline/engine/conf/__init__.py
pipeline/component_framework/admin.py
pipeline/core/constants.py
apps/backend/healthz/checker/checker.py
pipeline/component_framework/apps.py
apps/backend/healthz/checker/supervisor_checker.py
apps/utils/concurrent.py
... and 356 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f576c6d...9c006ed. Read the comment docs.

@ZhuoZhuoCrayon ZhuoZhuoCrayon force-pushed the _V2.1.X_develop/feature_issue_2 branch from 9c006ed to 05d39b7 Compare August 17, 2021 14:58
dir_path,
# 判断是否第三方插件的路径
arcname="external_plugins/%s" % package_name if is_external else "plugins/",
arcname=f"external_plugins/{project}" if is_external else f"plugins/{project}",
Copy link
Member Author

Choose a reason for hiding this comment

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

提取常量:external_plugins plugins

STORAGE_TYPE = os.getenv("STORAGE_TYPE", StorageType.FILE_SYSTEM.value)

# 是否覆盖同名文件
FILE_OVERWRITE = get_type_env("FILE_OVERWRITE", _type=bool, default=False)
Copy link
Member

Choose a reason for hiding this comment

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

默认是False吗?似乎节点管理的场景应该默认是True比较合适

Copy link
Member Author

Choose a reason for hiding this comment

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

特定场景下需要覆盖,比如插件包导入的成品,DOWNLOAD_PATH下,需要保证同名覆盖,逻辑已显式传入file_overwrite=True
但是在用户自主上传的情况下及导出的情况,同名文件建议是另存,覆盖会导致同名但文件内容不同的情况,无法完成问题溯源

is_release=is_release,
creator=task_params["bk_username"],
select_pkg_abs_paths=select_pkg_abs_paths,
select_pkg_relative_paths=select_pkg_abs_paths,
Copy link
Member

Choose a reason for hiding this comment

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

这里命名很迷惑呀,到底是相对路径还是绝对路径

Copy link
Member Author

Choose a reason for hiding this comment

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

接口字段命名不规范,改为在序列化器兼容select_pkg_abs_pathsselect_pkg_relative_paths ,后续前端更正

from apps.node_man import constants


class file_open:
Copy link
Member

Choose a reason for hiding this comment

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

类命名规范

@@ -25,6 +30,7 @@
urlpatterns = [
url(r"api/", include(routers.urls)),
url(r"^package/upload/$", upload_package),
url(r"^package/upload_cos/$", upload_package_by_cos),
Copy link
Member

Choose a reason for hiding this comment

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

这里应该 package/upload/ 里根据环境变量选择上传的方法, LocalStroage or Cos ,屏蔽掉这个逻辑而不暴露出来让调用方选择哪个API?

是上次讨论的nginx转发的问题对吧,这个再讨论下

Copy link
Member Author

Choose a reason for hiding this comment

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

结论:保持现状,监控需要变更

def __exit__(self, exc_type, exc_val, exc_tb):

# 指针复位,避免 closed=False 场景下,影响上层逻辑对该文件对象的复用
# 参考:https://stackoverflow.com/questions/3906137/why-cant-i-call-read-twice-on-an-open-file
Copy link
Member

Choose a reason for hiding this comment

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

所以节点管理这边有需要读两遍的场景吗?为什么需要读两遍

Copy link
Member Author

Choose a reason for hiding this comment

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

同向读写可以通过提供冗余逻辑解决
写后读的情况,比如文件下载后,通过tarfile解析时,就需要指针复位,比起维护两个上下文,重置指针好像优雅些,也不占内存

hash_md5.update(chunk)

except IOError:
return "-1"
Copy link
Member

Choose a reason for hiding this comment

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

MD5计算失败是不是抛出异常更好,而不是返回 "-1" 隐藏了问题

return hash_md5.hexdigest()


def download_file(
Copy link
Member

Choose a reason for hiding this comment

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

什么场景下需要用到这个方法

Copy link
Member Author

Choose a reason for hiding this comment

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

后台可以接受两种参数创建上传记录,其中一种就是download_url,主要是考虑后续监控等第三方可以在插件归档到自己仓库的同时,传download_url到节点管理,同步第三方平台的插件。

self.file_overwrite = file_overwrite

# 如果文件允许覆盖,去掉 O_CREAT 配置,存在文件打开时不报错
if self.file_overwrite:
Copy link
Member

Choose a reason for hiding this comment

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

如果上面的条件非真,这里就 AttributeError 了

Copy link
Member Author

Choose a reason for hiding this comment

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

继承了FileOverWriteMixin,具备属性fileoverwrite

@@ -923,57 +924,73 @@ def create_record(
:return: True | raise Exception
"""
# 1. 判断是否存在project.yaml文件
Copy link
Member

Choose a reason for hiding this comment

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

这里的逻辑跟parse_package重复率很高,是否可以合并复用?

feature: 插件包管理支持对象存储模式,公共方法优化整合 (TencentBlueKing#2)
@ZhuoZhuoCrayon ZhuoZhuoCrayon force-pushed the _V2.1.X_develop/feature_issue_2 branch from 05d39b7 to e9717f9 Compare August 24, 2021 08:45
@ZhuoZhuoCrayon
Copy link
Member Author

分支规范修改,转移至 #31

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.

3 participants