Split stderr and stdout in docker info parse

ironyman commented 3 years ago

Stderr has warnings that breaks JSON unmarshal. Fix this by only reading stdout.

This is the warning

HOME= sudo -E docker info --format "{{json .}}"  > /dev/null
WARNING: Error loading config file: .dockercfg: $HOME is not defined

Looks like docker changed the way it gets HOME recently possibly this commit that's causing the warning to be emitted. https://github.com/docker/cli/commit/c85a37dbb472c39bc88cf707fce83fed7a644fed

In some environments, like cloud-init I think, HOME is not set. So running kubeadm in cloud-init will fail. This fixes kubeadm init in some environments like cloud-init.

k8s-ci-robot commented 3 years ago

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).

k8s-ci-robot commented 3 years ago

Welcome @ironyman!

It looks like this is your first PR to kubernetes/system-validators 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/system-validators has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

k8s-ci-robot commented 3 years ago

Hi @ironyman. Thanks for your PR.

I'm waiting for a kubernetes 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

KentaTada commented 3 years ago

Hello @ironyman Could you sign the CLA at first?

At first glance, you changed not to use CombinedOutput() because its function includes both stdout and stderr. I'll review this commit later. Related issue: https://github.com/moby/moby/issues/41890

neolit123 commented 3 years ago

please keep the commits squashed to 1 and sign the CLA. without the CLA we cannot accept the PR.

i think the change is good and CombinedOutput should not be used. in terms of this actually causing failures, you should just set $HOME to mitigate.

given it's not a critical bug fix, we won't be able to backport this older kubeadm versions.

ironyman commented 3 years ago

Sounds good I will do the CLA soon.

neolit123 commented 3 years ago

please keep the commits squashed to 1.

1.22 will be in code freeze soon (early July) so if you wish this change to be part of it we must tag a new PATCH version in this repo and re-vendor it in k8s. would you have the time to do that? https://github.com/kubernetes/system-validators/tree/ca2e257c13e6d6e49e0de2a31d08c03c790f548f#creating-releases

see the pin-dependency.sh part.

neolit123 commented 3 years ago

/lgtm

k8s-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironyman, neolit123

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes/system-validators/blob/master/OWNERS)~~ [neolit123] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

neolit123 commented 3 years ago

actually we need to release a new version due to https://github.com/kubernetes/system-validators/commit/c9ffc95d14808b10c7d2780eedd3cfcf94dca869

@KentaTada is this correct?

KentaTada commented 3 years ago

actually we need to release a new version due to c9ffc95

@KentaTada is this correct?

Yes. I think the preflight check for seccomp is needed for the current k8s.

ironyman commented 3 years ago

So we should create a new tag v.1.4.1 on master? What do we have to do for preflight check?

neolit123 commented 3 years ago

since we are adding a new feature too in https://github.com/kubernetes/system-validators/commit/c9ffc95d14808b10c7d2780eedd3cfcf94dca869 this means we should release a new MINOR version - 1.5.0.

i will do that in a bit.

neolit123 commented 3 years ago

done v1.5.0 is now created. we can now update kubernetes/kubernetes to use it for 1.22. https://github.com/kubernetes/system-validators/releases/tag/v1.5.0

ironyman commented 3 years ago

Should I make a PR in kubernetes repo after the pin-dependency.sh? Anything I should do to validate the changes in kubernetes repo?

neolit123 commented 3 years ago

yes, if you can send the PR that would be great.

here is how the 1.4.0 PR looked like https://github.com/kubernetes/kubernetes/pull/98977 you add a similar title / description.

to verify the change you can:

pin the dependency to v1.5.0
use make all WHAT=cmd/kubeadm to build kubeadm
find the kubeadm binary in the /output folder
check if the kubeadm binary exhibits the same problem that you tried to fix in this current PR

neolit123 commented 3 years ago

1.22 will be in code freeze soon (early July)

the date is July 8th (next week Thursday). at that point this 1.5.0 update will not be accepted for 1.22 and has to wait for 1.23.

kubernetes / system-validators

Split stderr and stdout in docker info parse #27