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 less and e3 to the base image #339

Closed
wants to merge 1 commit into from
Closed

Add less and e3 to the base image #339

wants to merge 1 commit into from

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Feb 23, 2019

Both packages are essential for investigation/debugging and
configuring work. Additional footprint is just 414 kb.

I'm using kind for debugging kubeadm on Mac. It works great!
I've noticed that I always end up installing these packages. 'more' is not good to navigate long outputs.
e3 is the smallest possible editor I could find. Despite being very small it supports WordStar, Emacs, Pico, Nedit and VI command syntax.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bart0sh
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: munnerz

If they are not already assigned, you can assign the PR to them by writing /assign @munnerz in a comment when ready.

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 23, 2019
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2019
@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

/cc @BenTheElder

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2019
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

text reditors are opinionated.
some use nano, same vim, some emacs.

i don't think we should bundle an editor for that reason.

@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

/retest

@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

how would recommend this for debugging purposes without any editor available in the image?

@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

just because they're opinionated it doesn't mean they're not needed.

@neolit123
Copy link
Member

neolit123 commented Feb 23, 2019

just because they're opinionated it doesn't mean they're not needed.

sure but i don't know how to use vi, even.
my goto editor for such editing is nano.

@BenTheElder
Copy link
Member

BenTheElder commented Feb 23, 2019

Hmm what about docker exec node cat /bar | less ? (IE use less on the host?)

Regarding vi ... well we need an editor there anyway and this is one of the smallest one

Do we really need an editor on the nodes though? Why not edit on the host? What things are you editing?

Right now the bar for packages on the node is ~ just "things we need to boot a cluster".

I fear the next PR will want emacs /s 😛

@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

that's another story. I don't know how to use nano, so we have several choices here:

  • do nothing (your suggestion)
  • install vi (my suggestion)
  • install nano (might be your preference)
  • install vi and nano

emacs is out of question due to huge footprint I assume.

Let's ask other people. It's better to satisfy part of the users than none in my opinion.

@BenTheElder
Copy link
Member

I hear wanting to debug, but people will probably want different tools for this and I'm not sure we should bundle many if any.

My other concern is if we encourage workflows on the "nodes" people will probably start to depend on that for their CI setups too instead of just having Kubernetes. We already had a few pop up in the wild while kind load ... didn't exist yet.

@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

Do we really need an editor on the nodes though? Why not edit on the host? What things are you editing?

configs mainly. I found it quite awkward and inconvenient to switch between host and container just to edit and copy couple of files.

@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

My other concern is if we encourage workflows on the "nodes"

I had an impression that Kind is going to be used by developers to debug kubernetes tools. Please, correct me if I'm wrong here. I'm using it exactly for this purpose and I'm very happy about it so far.

@BenTheElder
Copy link
Member

I had an impression that Kind is going to be used by developers to debug kubernetes tools. Please, correct me if I'm wrong here. I'm using it exactly for this purpose and I'm very happy about it so far.

That is indeed the primary purpose, but we've picked up a lot of traction for integrating against Kubernetes (eg testing sonobuoy, cluster API) which seems to be an even stronger niche, so considering that is worthwhile. Details are outlined in the docs under project scope.

configs mainly. I found it quite awkward and inconvenient to switch between host and container just to edit and copy couple of files.

Have you seen the ability to add extra mounts? You can bind mount a directory for configs from the host.

@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

You can bind mount a directory for configs from the host.

That wouldn't help much if I'd want to edit couple of manifests, systemd drop-ins, some files in /etc etcetera.

@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

As I'm compiling on the host anyway (I develop on Mac) I don't need anything else. Just an editor. Of course I can install and do install it. Just thought that I'm not the only one who would benefit from having the opportunity to edit files on the node :)

@BenTheElder
Copy link
Member

To be clear this PR overall seems pretty reasonable, I'm just trying to understand your use cases and needs, and I want to avoid opening up to bundling every tool anyone wants for debugging 🙃

Rejecting an emacs PR (if it really is too big) is hard without any line in the sand, and right now the line doesn't encompass this PR.

It also might be worth considering if actually we do need plenty of tools for good debug, in which case we can have both a lite and debug base image.

My debug workflow doesn't need any of these, I poke at the containers from the host generally, but I'm not sure what the typical kubeadm dev needs 😅

@BenTheElder
Copy link
Member

With kind build node-image --base-image... we could even make the default base be an extended debug image and then when publishing prebuilt node images we could opt into using the lite image.

Knowing what tools and uses would help figure out if that makes sense. Maybe we should do a survey.

I'd like to bring this up on Monday at our next meeting and see what everyone else thinks. If less and vi suit everyone we can just do this, but if we expect more tools I think something like a debug option for the image might make more sense.

With my own workflow this wasn't even on my radar 😅 I agree that others will probably find something like this useful for debugging kubeadm and probably some of the other components too.

@bart0sh
Copy link
Contributor Author

bart0sh commented Feb 23, 2019

To be clear this PR overall seems pretty reasonable, I'm just trying to understand your use cases and needs, and I want to avoid opening up to bundling every tool anyone wants for debugging 🙃

Thank you. I appreciate your attitude.
I'd not call a tiny editor a debugging tool. People would need it for different purposes I believe. Testers, devs, admins - all of them would need an editor to tweak cluster configuration, wouldn't they?

Rejecting an emacs PR (if it really is too big) is hard without any line in the sand, and right now the line doesn't encompass this PR.

Don't worry about that :)

# apt-get install emacs-nox
...
After this operation, 138 MB of additional disk space will be used.
# apt-get install emacs
After this operation, 286 MB of additional disk space will be used.

I'd like to bring this up on Monday at our next meeting and see what everyone else thinks. If less and vi suit everyone we can just do this, but if we expect more tools I think something like a debug option for the image might make more sense.

Sure, let's discuss it. Of course people would want more tools and I don't see anything bad with this. We just need to come up with a criteria for inclusion/rejection.

P.S.: if a footprint is a big concern we can reduce this image. Just a couple of ideas:

root@kind-control-plane:/# du -sh /usr/share/man/
272K	/usr/share/man/
root@kind-control-plane:/# du -sh /usr/share/doc 
3.5M	/usr/share/doc

@aojea
Copy link
Contributor

aojea commented Mar 4, 2019

having ping inside the image will be very useful for troubleshooting network issues too apt-get install iputils-ping

@BenTheElder
Copy link
Member

Good ideas for reducing it.

I completely failed to comment back after the meeting, the suggestion there was to create something like:

create images/debug/Dockerfile with:

FROM kindest/base
RUN apt install vi less ping ...

build a base image with:
docker build -t kindest/base:debug images/debug

develop kubernetes with node images built like:
kind build node-image --base-image=kindest/base:debug ...

...

That said, installing a couple of these things in the normal base image is probably reasonable? I'm just not sure where to set the cutoff ... It's very easy to set it at "just what Kubernetes needs" and trim it back at any point based on that.

@tao12345666333
Copy link
Member

text reditors are opinionated.
some use nano, same vim, some emacs.

i don't think we should bundle an editor for that reason.

+1 IMO we can provide a manual to tell people how to debug, not to force some way.

I completely failed to comment back after the meeting, the suggestion there was to create something like:

create images/debug/Dockerfile with:

FROM kindest/base
RUN apt install vi less ping ...

build a base image with:
docker build -t kindest/base:debug images/debug

develop kubernetes with node images built like:
kind build node-image --base-image=kindest/base:debug ...

The above can be used as a guide content.

But like the choice of editor or debugging tools, I don't want it to be the focus of our attention, even though I like vi very much.

@bart0sh
Copy link
Contributor Author

bart0sh commented Mar 7, 2019

That said, installing a couple of these things in the normal base image is probably reasonable?

I believe it is.

I'm just not sure where to set the cutoff ...

How about setting threshold for a size of the base image and then use 'first come - first served' approach and good old common sense :) ? After all this and similar changes are just PRs and any approver can either approve or reject them.

@bart0sh
Copy link
Contributor Author

bart0sh commented Mar 7, 2019

/retest

e3 is a tiny editor and less is a convenient pager.

Both are essential for investigation/debugging and
configuring work. Additional footprint is just 414 Kb.

Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
@bart0sh bart0sh changed the title Add less and vi to the base image Add less and e3 to the base image Mar 7, 2019
@alejandrox1
Copy link
Contributor

Throwing in my two cents: I don't like the idea of adding tools to the base image - they are not necessary and if we do then it becomes completely arbitrary what to add and what not to.

I think the idea to add whatever tools a user may ned on top of the node image is the best solution here.

@neolit123
Copy link
Member

/hold
no editors please. everybody uses a different one, with different shortcut keys...
a custom Docker image or a script that pulls editors on all nodes is the way to go.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2019
@bart0sh
Copy link
Contributor Author

bart0sh commented Mar 8, 2019

@neolit123
it's actually 5 editors in one :)
And editor is even more essential than anything else in my opinion.

Feel free to close this PR. no editor - no interest for me to continue with it.

a custom Docker image or a script that pulls editors on all nodes is the way to go.

That's what I already do, but it's additional work, which could be eliminated, hence this PR.

I agree with @BenTheElder here that it makes sense to have at least some tools in the base image.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2019
@k8s-ci-robot
Copy link
Contributor

@bart0sh: PR needs rebase.

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.

@BenTheElder
Copy link
Member

I've been meaning to setup a possibly compromise for this, just haven't gotten to it yet... we could put a contrib dockerfile like:

ARG BASE_IMAGE=<some default base image version>
FROM ${BASE_IMAGE}
RUN <install editors and other handy debug tools>

With build instructions etc., and then when iterating on kubeadm suggest using that as the --base-image.

@k8s-ci-robot
Copy link
Contributor

@bart0sh: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kind-conformance-parallel-1-14 c933871 link /test pull-kind-conformance-parallel-1-14

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@aojea
Copy link
Contributor

aojea commented May 6, 2019

I want to share what I'm doing right now for debugging maybe it can be useful, I'm logging into the image and installing my toolset apt-get update && apt-get install vim tcpdump ...
It takes 30 seconds more or less to install them

@aojea aojea mentioned this pull request May 8, 2019
@bart0sh bart0sh closed this Jun 7, 2019
stg-0 pushed a commit to stg-0/kind that referenced this pull request Feb 6, 2024
* chore: bump azure provider

* bump azure provider version

* [CLOUD-109] Integrar GKE como nuevo provider (kubernetes-sigs#320)

* Initial gke support

* Fix gcr authentication

* Minor fixes

* Fix gcr authentication

* Enable machine pools for gke

* Enable gke in capg

* Add google artifact registry support

* Add gke version validation

* Improve error message

* Update cluster-operator version

* Fix gke install logic

* Improve execute command retries (kubernetes-sigs#298)

* Improve execute command retries

* Fix kubeconfig condition

* Fix ecr login when ecr is in another region

* Remove comment

* [BUILD] 0.17.0-0.3.0: New pre-release

* Prepare for next version: 0.17.0-0.3.0-SNAPSHOT

* Prepare for next version: 0.17.0-0.3.1-SNAPSHOT

* Improve retry logic

---------

Co-authored-by: stratiocommit <stratiocommit@stratio.com>

* Feature/add infra validations (kubernetes-sigs#335)

* version v0.18.0-alpha

* update docs for v0.17.0

* fix kind version in readme

* comments-update-buildcontext

* Added validations for azs, k8sVersion, vpcs and subnets in each provider

* fixing dependency bugs

* Merge with master

* fixed gcp

* Update pkg/cluster/internal/validate/gcp.go

Co-authored-by: esierra-stratio <102975650+esierra-stratio@users.noreply.github.com>

* fixed build

* Update CHANGELOG.md

---------

Co-authored-by: Benjamin Elder <bentheelder@google.com>
Co-authored-by: Daman <aroradaman@gmail.com>
Co-authored-by: Kubernetes Prow Robot <k8s-ci-robot@users.noreply.github.com>
Co-authored-by: esierra-stratio <102975650+esierra-stratio@users.noreply.github.com>

* version v0.18.0-alpha

* delete unneeded files

* add CHANGELOG

---------

Co-authored-by: Francisco Augusto <faugusto@stratio.com>
Co-authored-by: stratiocommit <stratiocommit@stratio.com>
Co-authored-by: lreciomelero <120394823+lreciomelero@users.noreply.github.com>
Co-authored-by: Benjamin Elder <bentheelder@google.com>
Co-authored-by: Daman <aroradaman@gmail.com>
Co-authored-by: Kubernetes Prow Robot <k8s-ci-robot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants