zilliztech / milvus-operator

The Kubernetes Operator of Milvus.
https://milvus.io
Apache License 2.0
33 stars 20 forks source link

feat:support hostNetWork for components #141

Closed qchenzi closed 4 days ago

haorenfsa commented 1 week ago

Hi @qchenzi, the patch looks good. Still there're 3 things to do, before we could merge it.

  1. [x] Generate updates for CRD by Using make generate-all command under the root of the repo.
  2. [x] Add an integration test for this new feature by adding the new fields in https://github.com/zilliztech/milvus-operator/blob/main/test/min-mc-feature.yaml, so that our CI can test if it works.
  3. [x] Sign off all your commits https://github.com/zilliztech/milvus-operator/pull/141/checks?check_run_id=26704300058
qchenzi commented 1 week ago

Hi @qchenzi, the patch looks good. Still there're 3 things to do, before we could merge it.

  1. [ ] Generate updates for CRD by Using make generate-all command under the root of the repo.
  2. [ ] Add an integration test for this new feature by adding the new fields in https://github.com/zilliztech/milvus-operator/blob/main/test/min-mc-feature.yaml, so that our CI can test if it works.
  3. [ ] Sign off all your commits https://github.com/zilliztech/milvus-operator/pull/141/checks?check_run_id=26704300058

Hi @haorenfsa,

Thank you for your feedback. I have completed the three tasks you mentioned:

  1. Updated the CRDs using make generate-all, and you can find it deploy/manifests/deployment.yaml

  2. Added an integration test for the new feature, and you can find it test/min-mc-feature.yaml

  3. Rebased and squashed all my commits into one, and sign off my commit.

Please let me know if there is anything else required.

Best regards.

haorenfsa commented 1 week ago

Hi @qchenzi, you commits still not correctly signed off, check below instructions:

image
codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 82.29%. Comparing base (592de3d) to head (03dad37).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #141 +/- ## ========================================== + Coverage 82.26% 82.29% +0.03% ========================================== Files 54 54 Lines 4944 4954 +10 ========================================== + Hits 4067 4077 +10 Misses 693 693 Partials 184 184 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

qchenzi commented 1 week ago

Hi @haorenfsa,

Thank you for pointing that out. I have correctly signed off all my commits as per your instructions. Please review.

haorenfsa commented 5 days ago

@qchenzi Oh, the new feature test seems to be failing. It's because that the CRD should not only be updated in deploy/manifests/deployment.yaml but also in https://github.com/zilliztech/milvus-operator/tree/main/config/crd/bases and https://github.com/zilliztech/milvus-operator/blob/main/charts/milvus-operator/templates/crds.yaml.

haorenfsa commented 5 days ago

The make generate-all should be able to update them all, plz check if you miss them when you commit the patch.

sre-ci-robot commented 4 days ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haorenfsa, qchenzi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/zilliztech/milvus-operator/blob/main/OWNERS)~~ [haorenfsa] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
qchenzi commented 4 days ago

Hi @haorenfsa

Thank you for pointing out the missing updates. I have reviewed the files and realized that I indeed missed updating some files.

I have now run make generate-all again to ensure all necessary files are updated and have committed these changes. Please review the latest updates.

Thank you for your patience and assistance.

haorenfsa commented 4 days ago

Thank you @qchenzi!
The new feature test now failed because port 9091 is used for all milvus pods to export metrics, when milvus pods are scheduled to the same node, it conflicts. Last feature is related to hpa. Thus in test we have more than 1 proxy pods, and they are scheduled to the same node. I checked the logs, other pods are working well. Thus I think it's ok to merge. Thank you again for providing this patch!

haorenfsa commented 4 days ago

/lgtm