-
Notifications
You must be signed in to change notification settings - Fork 7
chore: add docker image for ib tools #190
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
=======================================
Coverage 64.84% 64.84%
=======================================
Files 78 78
Lines 4275 4275
=======================================
Hits 2772 2772
Misses 1395 1395
Partials 108 108 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile OverviewGreptile SummaryAdds infrastructure for building and deploying InfiniBand diagnostic tools as a separate Docker image. The new workflow builds a lightweight Ubuntu-based container with Key changes:
Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant GHA as GitHub Actions
participant GHCR as ghcr.io Registry
participant K8s as Kubernetes Cluster
participant Pod as IB Tools Pod
participant IB as InfiniBand Network
Dev->>GHA: Trigger workflow_dispatch
GHA->>GHA: Checkout repository
GHA->>GHA: Build Dockerfile.ib (multi-arch)
GHA->>GHCR: Push image to ghcr.io/nvidia/topograph/ib
Note over K8s: Deploy with values.k8s-ib-example.yaml
K8s->>GHCR: Pull ghcr.io/nvidia/topograph/ib:main
K8s->>Pod: Create privileged pod with /sys/class mount
Pod->>IB: Access InfiniBand devices via mounted /sys/class
Pod->>Pod: Run ibnetdiscover and IB diagnostic tools
Pod->>K8s: Report topology data to node-observer
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 1 comment
.github/workflows/docker.yml
Outdated
| #- name: Build and push app Docker image | ||
| # id: build-and-push | ||
| # uses: docker/build-push-action@v6 | ||
| # with: | ||
| # context: . | ||
| # file: ./Dockerfile | ||
| # push: ${{ github.event_name != 'pull_request' }} | ||
| #build-args: | | ||
| # TARGETOS=linux | ||
| # TARGETARCH=amd64 | ||
| # platforms: linux/amd64,linux/arm64 | ||
| # tags: | | ||
| # ${{ env.REGISTRY }}/${{ github.repository }}/app:latest | ||
| # ${{ env.REGISTRY }}/${{ github.repository }}/app:${{ github.sha }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: this commented-out section appears to be leftover code from refactoring - remove if not needed
| #- name: Build and push app Docker image | |
| # id: build-and-push | |
| # uses: docker/build-push-action@v6 | |
| # with: | |
| # context: . | |
| # file: ./Dockerfile | |
| # push: ${{ github.event_name != 'pull_request' }} | |
| #build-args: | | |
| # TARGETOS=linux | |
| # TARGETARCH=amd64 | |
| # platforms: linux/amd64,linux/arm64 | |
| # tags: | | |
| # ${{ env.REGISTRY }}/${{ github.repository }}/app:latest | |
| # ${{ env.REGISTRY }}/${{ github.repository }}/app:${{ github.sha }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 1 comment
.github/workflows/docker.yml
Outdated
| REGISTRY: ghcr.io | ||
| # github.repository as <account>/<repo> | ||
| IMAGE_NAME: ${{ github.repository }} | ||
| REPO_NAME: ${{ github.repository.toLower() }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: .toLower() is not valid GitHub Actions expression syntax - the workflow will fail at runtime
| REPO_NAME: ${{ github.repository.toLower() }} | |
| REPO_NAME: ${{ lower(github.repository) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, no comments
756db47 to
eca373c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 1 comment
.github/workflows/docker.yml
Outdated
| REGISTRY: ghcr.io | ||
| # github.repository as <account>/<repo> | ||
| IMAGE_NAME: ${{ github.repository }} | ||
| REPO_NAME: ${{ github.repository }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: REPO_NAME env variable is defined but never used - the tags on lines 71-72 are hardcoded to nvidia/topograph
| REPO_NAME: ${{ github.repository }} | |
| # IMAGE_NAME is used in hardcoded tags below |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 1 comment
.github/workflows/docker.yml
Outdated
| REGISTRY: ghcr.io | ||
| # github.repository as <account>/<repo> | ||
| IMAGE_NAME: ${{ github.repository }} | ||
| IMAGE_NAME: ${{ github.repository }}/ib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: modifying this workflow replaces the main application image build - the original Dockerfile that builds the topograph application will no longer be built
create a separate workflow file (e.g., docker-ib.yml) for the IB tools image, or use a matrix strategy to build both images
2f1c9e2 to
38488bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 1 comment
| verbosity: 4 | ||
|
|
||
| config: | ||
| request_aggregation_delay: 15s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: key name uses incorrect snake_case format - should be camelCase requestAggregationDelay to match the template at charts/topograph/templates/configmap.yml:12 and config struct at pkg/config/config.go:34
| request_aggregation_delay: 15s | |
| requestAggregationDelay: 15s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, no comments
Signed-off-by: Dmitry Shmulevich <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, no comments
No description provided.