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

Add libipmctl to the docker image. #2674

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

Creatone
Copy link
Collaborator

This PR adds libipmctl to the docker image.

@k8s-ci-robot
Copy link
Collaborator

Hi @Creatone. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

deploy/Dockerfile Outdated Show resolved Hide resolved
deploy/Dockerfile Outdated Show resolved Hide resolved
@katarzyna-z
Copy link
Collaborator

#2676 should be merged with this pull request without these changes it is not possible to run docker container on machine without non-volatile memory


FROM alpine:3.12
MAINTAINER dengnan@google.com vmarmol@google.com vishnuk@google.com jimmidyson@gmail.com stclair@google.com

RUN apk --no-cache add libc6-compat device-mapper findutils zfs && \
RUN apk --no-cache add libc6-compat device-mapper findutils zfs ndctl-dev && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

ndctl should be enough, development files are not needed to run cAdvisor with libipmctl

Copy link
Collaborator

Choose a reason for hiding this comment

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

is this still correct? is ndctl enough or is ndctl-dev needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately not

@iwankgb
Copy link
Collaborator

iwankgb commented Oct 9, 2020

@Creatone can you visit https://git.k8s.io/community/community-membership.md#member? You will not need ok-to-test anymore.

@iwankgb
Copy link
Collaborator

iwankgb commented Oct 9, 2020

/ok-to-test

@Creatone
Copy link
Collaborator Author

Creatone commented Oct 10, 2020

@Creatone can you visit https://git.k8s.io/community/community-membership.md#member? You will not need ok-to-test anymore.

Thanks, I'll try after my holiday.

@iwankgb
Copy link
Collaborator

iwankgb commented Oct 29, 2020

@bobbypage this is another PR that is relatively straightforward and could be merged.

@Creatone
Copy link
Collaborator Author

@bobbypage Can you look at this? Nice to have this in docker :)

@bobbypage bobbypage self-assigned this Oct 30, 2020
@bobbypage
Copy link
Collaborator

Thanks for adding this. One question I have -- my understanding is that libipmctl is not required for cadvisor, libipmctl is needed to get info about Optane Persistent Memory.

By adding it to the main dockerfile, it means the Docker image will always include libipmctl, and cadvisor will be built with it (i.e. GO_FLAGS). Is there any issue always including libipmctl and building cadvisor against it in the docker image? What happens if the underlying system doesn't support ipmctl?

@Creatone
Copy link
Collaborator Author

@bobbypage after #2723 , no :)

If the underlying system doesn't support ipmctl, it would print a warning and continue working.

@Creatone
Copy link
Collaborator Author

@bobbypage

@bobbypage
Copy link
Collaborator

bobbypage commented Dec 30, 2020

Thanks for change @Creatone .

I tested it out and it seemed to build fine at the end but I got some suspicious errors and warnings, e.g.

make[1]: [Makefile:401: install] Error 1 (ignored)

See full build log here: https://gist.github.com/bobbypage/dbc5f966e44db6336c7ebf94f9da706a

Are those errors / compiler warnings expected?

@Creatone
Copy link
Collaborator Author

Creatone commented Jan 8, 2021

This is related to libpfm dependency. I'll investigate this separately.

@bobbypage
Copy link
Collaborator

bobbypage commented Jan 22, 2021

Thanks @Creatone, can you rebase and repush to trigger CI again, looks like it's stuck in a weird state.

Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>
@Creatone
Copy link
Collaborator Author

@bobbypage Done

@bobbypage
Copy link
Collaborator

LGTM, thanks!

@bobbypage bobbypage merged commit 4c8c5eb into google:master Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants