Skip to content

remove gvnic-1 from pods manifests#4088

Merged
ighosh98 merged 1 commit into
GoogleCloudPlatform:developfrom
liuyuan10:removegvnic
May 14, 2025
Merged

remove gvnic-1 from pods manifests#4088
ighosh98 merged 1 commit into
GoogleCloudPlatform:developfrom
liuyuan10:removegvnic

Conversation

@liuyuan10

Copy link
Copy Markdown
Contributor

the 2nd gvnic is not needed by NCCL

Submission Checklist

NOTE: Community submissions can take up to 2 weeks to be reviewed.

Please take the following actions before submitting this pull request.

  • Fork your PR branch from the Toolkit "develop" branch (not main)
  • Test all changes with pre-commit in a local branch #
  • Confirm that "make tests" passes all tests
  • Add or modify unit tests to cover code changes
  • Ensure that unit test coverage remains above 80%
  • Update all applicable documentation
  • Follow Cluster Toolkit Contribution guidelines #

@liuyuan10 liuyuan10 requested review from a team and samskillman as code owners May 8, 2025 19:22
@liuyuan10

Copy link
Copy Markdown
Contributor Author

@Jiaqicao257

the 2nd gvnic is not needed by NCCL
@liuyuan10

Copy link
Copy Markdown
Contributor Author

@ighosh98 could you take a look of this change. We are removing gvnic-1 from the recommended pod manifest. We've meregd the similar change GoogleCloudPlatform/container-engine-accelerators#492

@ighosh98 ighosh98 added the release-improvements Added to release notes under the "Improvements" heading. label May 13, 2025
@ighosh98

Copy link
Copy Markdown
Contributor

/gcbrun

@ighosh98

Copy link
Copy Markdown
Contributor

@liuyuan10 Could you also add a reason for this change? Will approve if integration tests pass.

@liuyuan10

Copy link
Copy Markdown
Contributor Author

@liuyuan10 Could you also add a reason for this change? Will approve if integration tests pass.

@ighosh98 I have that in the commit message. the 2nd gvnic is not needed by NCCL so no need to pass them to the workloads in general

@SwarnaBharathiMantena SwarnaBharathiMantena self-requested a review May 13, 2025 06:49
@ighosh98 ighosh98 enabled auto-merge May 13, 2025 06:53
@ighosh98 ighosh98 disabled auto-merge May 13, 2025 06:58
@SwarnaBharathiMantena SwarnaBharathiMantena self-requested a review May 13, 2025 07:00
@SwarnaBharathiMantena

Copy link
Copy Markdown
Contributor

@liuyuan10 Should it be removed from the A4 nccl jobset example too?

@liuyuan10

Copy link
Copy Markdown
Contributor Author

@liuyuan10 Should it be removed from the A4 nccl jobset example too?

Yes. it's already included in the commit. @SwarnaBharathiMantena

@ighosh98 ighosh98 enabled auto-merge May 14, 2025 04:25
@ighosh98 ighosh98 merged commit 4a672c4 into GoogleCloudPlatform:develop May 14, 2025
17 of 68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-improvements Added to release notes under the "Improvements" heading.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants