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: hgctl install profile support resource configuration #823

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

sjcsjc123
Copy link
Collaborator

Ⅰ. Describe what this PR did

支持resources配置

Ⅱ. Does this pull request fix one issue?

fixes: #571

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

image
运行hgctl install --set profile=k8s命令,可以看到deploy yaml文件多了resource配置

Ⅴ. Special notes for reviews

@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f537a1c) 38.04% compared to head (058667e) 38.10%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #823      +/-   ##
==========================================
+ Coverage   38.04%   38.10%   +0.06%     
==========================================
  Files          61       61              
  Lines       10428    10428              
==========================================
+ Hits         3967     3974       +7     
+ Misses       6161     6154       -7     
  Partials      300      300              

see 2 files with indirect coverage changes

return "", err
}

err = yaml.Unmarshal(resourceYAML, &p.Values)
Copy link
Collaborator

@2456868764 2456868764 Feb 15, 2024

Choose a reason for hiding this comment

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

这里有些问题:这里会覆盖 Values 相关Resource设置, 要保证相同设置 Values里值高优先。用util.OverlayYAML 合并。

Copy link
Collaborator

Choose a reason for hiding this comment

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

这里外层的配置覆盖values里的值是否也合理?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

已修复,当在profile的value字段进行如下配置的时候,通过debug发现valueOverlayYAML结果符合预期。

  • profile value配置:
values:
  higress-core:
    controller:
        resources:
          requests:
            cpu: 1213m
            memory: 2018Mi
          limits:
            cpu: 1213m
            memory: 2018Mi
  • valueOverlayYAML结果:
higress-console:
  resources:
    limits:
      cpu: 2000m
      memory: 2048Mi
    requests:
      cpu: 250m
      memory: 512Mi
higress-core:
  controller:
    resources:
      limits:
        cpu: 1213m
        memory: 2018Mi
      requests:
        cpu: 1213m
        memory: 2018Mi
  gateway:
    resources:
      limits:
        cpu: 2000m
        memory: 2048Mi
      requests:
        cpu: 2000m
        memory: 2048Mi

Copy link
Collaborator

Choose a reason for hiding this comment

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

这里外层的配置覆盖values里的值是否也合理?

这里主要考虑到和helm 安装兼容,所以values 优先级高

@2456868764
Copy link
Collaborator

LGTM

@johnlanni johnlanni merged commit 3967eec into alibaba:main Feb 23, 2024
8 checks passed
Renz7 pushed a commit to Renz7/higress that referenced this pull request Mar 4, 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.

hgctl install profile 增加 resources 配置
4 participants