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.

Sign up for GitHub

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

Jump to bottom

Housekeeping #36

Merged
merged 2 commits into from
Feb 6, 2024
Merged

Housekeeping #36

merged 2 commits into from
Feb 6, 2024

Conversation

almaslennikov
Copy link
Collaborator

No description provided.

ykulazhenkov
ykulazhenkov reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@ykulazhenkov ykulazhenkov left a comment

Choose a reason for hiding this comment

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

Should we remove this string from the README.md https://github.com/Mellanox/ipoib-cni/blob/master/README.md?plain=1#L64?

Should we also update deployment yamls with correct image registry/tags? https://github.com/Mellanox/ipoib-cni/blob/master/images/ipoib-cni-daemonset.yaml

Dockerfile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
images/entrypoint.sh Outdated Show resolved Hide resolved
@almaslennikov almaslennikov force-pushed the housekeeping branch 2 times, most recently from 74049f8 to 79b7d02 Compare July 20, 2023 10:10
@almaslennikov
Copy link
Collaborator Author

@ykulazhenkov Updated the Readme and Makefile for building multi-arch images

@abdallahyas
Copy link

error in CI

Failed to create pod sandbox: rpc error: code = Unknown desc = [failed to set up sandbox container "595d5d8445555543065d366440537e62eba163305fac8136fd53f08547c13a4d" network for pod "legacy-test-pod1": networkPlugin cni failed to set up pod "legacy-test-pod1_default" network: [default/legacy-test-pod1/:mynet]: error adding container to network "mynet": failed to find plugin "ipoib" in path [/opt/cni/bin /opt/cni/bin], failed to clean up sandbox container "595d5d8445555543065d366440537e62eba163305fac8136fd53f08547c13a4d" network for pod "legacy-test-pod1": networkPlugin cni failed to teardown pod "legacy-test-pod1_default" network: DelegateDel: error invoking DelegateDel - "ipoib": error in getting result from DelNetwork: failed to find plugin "ipoib" in path [/opt/cni/bin /opt/cni/bin]]

we are deploying the daemonset in the repo to install the ipoib binary, so it seems something changed in the daemonset that cuases it to fail to install the ipoib binary can you please check it

@almaslennikov
Copy link
Collaborator Author

/retest-all

@abdallahyas
Copy link

/retest

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@almaslennikov
Update golang to 1.20 and project dependencies
bb59930 
Update .golangi-lint.yml

Signed-off-by: amaslennikov <amaslennikov@nvidia.com>
@almaslennikov almaslennikov force-pushed the housekeeping branch 2 times, most recently from 7194776 to d81dcda Compare January 24, 2024 11:14
adrianchiris
adrianchiris reviewed Feb 5, 2024
View reviewed changes
images/ipoib-cni-daemonset.yaml
@@ -33,63 +33,13 @@ spec:
name: ipoib-cni
spec:
hostNetwork: true
nodeSelector:
beta.kubernetes.io/arch: amd64
tolerations:
- key: node-role.kubernetes.io/master
Copy link
Collaborator

Choose a reason for hiding this comment

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

While at it, could you add toleration for node-role.kubernetes.io/control-plane ? which is the new naming for tainting.

adrianchiris
adrianchiris approved these changes Feb 5, 2024
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment •
edited
Loading

Choose a reason for hiding this comment

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

accidently reviewed only the last patch. ill take a look at the entire thing

@adrianchiris adrianchiris self-requested a review February 5, 2024 08:05
adrianchiris
adrianchiris reviewed Feb 5, 2024
View reviewed changes
Makefile Outdated
@@ -112,6 +121,18 @@ test-coverage: test-coverage-tools | $(BASE) ; $(info running coverage tests...
image: | $(BASE) ; $(info Building Docker image...) ## Build conatiner image
$(IMAGE_BUILDER) build -t $(TAG) -f $(DOCKERFILE) $(CURDIR) $(IMAGE_BUILD_OPTS)

.PHONY: image-ppc64le
image-ppc64le: | $(BASE) ; $(info Building Docker image...) ## Build container image for ppc64le
$(IMAGE_BUILDER) run --rm --privileged tonistiigi/binfmt:latest --install all
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this ?

@almaslennikov
Update github actions
6510338 
Multiarch build
Static schecks
Image publishing
Enable hadolint as a make target and update Dockerfile
Remove unneeded Dockerfile for ppc64le
Enable shellcheck in makefile and update entrypoint.sh to comply

Signed-off-by: amaslennikov <amaslennikov@nvidia.com>

Update image-push-release.yaml

Update image-push-master.yaml

allow multiarch builds in ds
adrianchiris
adrianchiris approved these changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I trust the CI workflows were tested on the development branch and operate as expected for both pushes to master and tag creation.

LGTM!

@adrianchiris adrianchiris merged commit 78454d4 into Mellanox:master Feb 6, 2024
8 checks passed
@adrianchiris adrianchiris mentioned this pull request May 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ykulazhenkov ykulazhenkov ykulazhenkov left review comments

@adrianchiris adrianchiris adrianchiris approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@almaslennikov @abdallahyas @adrianchiris @ykulazhenkov