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

Remove netstat from cilium-bugtool and replace with ss tool #11667

Merged
merged 1 commit into from May 29, 2020

Conversation

soumynathan
Copy link
Contributor

This PR removes the 'netstat -a' command from cilium-bugtool
configuration and substitutes 'ss -a' command instead. Also
adds the 'ss' tool to the Docker.builder

Fixes: #11648
Signed-off-by: Swaminathan Vasudevan svasudevan@suse.com

@soumynathan soumynathan requested review from a team as code owners May 23, 2020 00:06
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@@ -28,6 +28,7 @@ RUN apt-get update && \
git \
libc6-dev \
libelf-dev \
iproute2 \
Copy link
Member

Choose a reason for hiding this comment

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

The builder image relies on the cilium-runtime image. The latter already have some of the tooling from iproute2.

What needs to be done instead is: changing https://github.com/cilium/packaging/blob/master/Dockerfile.iproute2 to copy ss tool into the scratch iproute2 image there. And once this is done the cilium-builder image needs to COPY --from=quay.io/cilium/cilium-iproute2:xxxx-xx-xx /bin/ss /bin/ to its own image.

Copy link
Member

@borkmann borkmann May 23, 2020

Choose a reason for hiding this comment

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

But one more clarification, does the bugtool rely on Dockerfile.builder which is why we need it in here as a dependency? (I was not aware of that.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification. I was try to fix this.

Copy link
Member

Choose a reason for hiding this comment

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

@soumynathan why does it need to be in the builder image at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soumynathan why does it need to be in the builder image at all?

Sorry, my bad, I should not have added it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The builder image relies on the cilium-runtime image. The latter already have some of the tooling from iproute2.

What needs to be done instead is: changing https://github.com/cilium/packaging/blob/master/Dockerfile.iproute2 to copy ss tool into the scratch iproute2 image there. And once this is done the cilium-builder image needs to COPY --from=quay.io/cilium/cilium-iproute2:xxxx-xx-xx /bin/ss /bin/ to its own image.

So we need to touch/modify the 'cilium/packaging' repo along with the 'cilium/bugtool'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The builder image relies on the cilium-runtime image. The latter already have some of the tooling from iproute2.

What needs to be done instead is: changing https://github.com/cilium/packaging/blob/master/Dockerfile.iproute2 to copy ss tool into the scratch iproute2 image there. And once this is done the cilium-builder image needs to COPY --from=quay.io/cilium/cilium-iproute2:xxxx-xx-xx /bin/ss /bin/ to its own image.

Ok, I have pushed in a PR to add the 'ss' to the cilium-iproute2 image.
cilium/image-tools#15
Once it merges, then we can edit the

COPY --from=quay.io/cilium/cilium-iproute2:2020-05-20 /bin/tc /bin/ip /bin/
to include the 'ss'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now since the PR 'cilium/image-tools#15' merged, do we need to wait for the latest 'cilium-iproute2: xxxx-xx-xx' build to happen. The latest I see is the 2020-05-20.

Copy link
Member

Choose a reason for hiding this comment

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

The new container build would have occurred automatically here:

https://github.com/cilium/image-tools/runs/712053055?check_suite_focus=true

There's been some changes to the way these images are generated/tagged recently so I'm not sure whether the date-based tags will show up or if we just need to adjust the main Cilium runtime image to consume these SHA tags from the repo directly.

@errordeveloper are the main Cilium dockerfiles consuming these images yet? If not, it may make sense for @soumynathan to put this work on the backburner until that work is done and then revisit this PR.

@joestringer joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label May 26, 2020
@soumynathan soumynathan force-pushed the cilium-bugtool-replace-netstat branch from 4fd4946 to 6e22e38 Compare May 26, 2020 21:28
@coveralls
Copy link

coveralls commented May 26, 2020

Coverage Status

Coverage decreased (-0.002%) to 36.869% when pulling fc220b6 on soumynathan:cilium-bugtool-replace-netstat into 1439b27 on cilium:master.

@soumynathan soumynathan force-pushed the cilium-bugtool-replace-netstat branch from 6e22e38 to 4898820 Compare May 27, 2020 00:19
This PR removes all the non-functional commands from cilium-bugtool
configuration.

Fixes: cilium#11648
Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>
@soumynathan soumynathan force-pushed the cilium-bugtool-replace-netstat branch from 4898820 to fc220b6 Compare May 29, 2020 05:14
Copy link
Member

@borkmann borkmann left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@aanm aanm merged commit 345a1ab into cilium:master May 29, 2020
1.8.0 automation moved this from In progress to Merged May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

cilium-bugtool gathers netstat, but netstat is not provided in cilium container
5 participants