Build images from tarballs #396
Conversation
|
Welcome @akutz! |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akutz If they are not already assigned, you can assign the PR to them by writing 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 |
k8s-ci-robot
added
the
size/L
akutz
force-pushed
the
feature/build-from-tar
branch
6 times, most recently
from
b1fdf35 to
7e62c21
Compare
There was a problem hiding this comment.
@akutz
i've added some comments, but i would hold on @BenTheElder 's +1 on the overall idea for this PR, before doing changes.
| # the .NodeRegistration.KubeletExtraArgs object in the configuration files instead. KUBELET_EXTRA_ARGS should be sourced from this file. | ||
| EnvironmentFile=-/etc/default/kubelet | ||
| ExecStart= | ||
| ExecStart=/usr/bin/kubelet $KUBELET_KUBECONFIG_ARGS $KUBELET_CONFIG_ARGS $KUBELET_KUBEADM_ARGS $KUBELET_EXTRA_ARGS |
There was a problem hiding this comment.
baking in the unit/drop-in is not ideal. because kind now has to maintain and sync with the upstream systemd files...
we have deb/rpm specs scattered in k/k and k/release. kind probably shouldn't manage it's own copies.
i would HTTP download them from github:
https://github.com/kubernetes/release/blob/master/debian/xenial/kubelet/lib/systemd/system/kubelet.service
https://github.com/kubernetes/release/blob/master/debian/xenial/kubeadm/channel/stable/etc/systemd/system/kubelet.service.d/post-1.10/10-kubeadm.conf
(1.15 is hopefully going to nuke the above, though).
There was a problem hiding this comment.
Hi @neolit123,
I was just following @BenTheElder's // TODO from the BazelBits code. Still, I've no issue downloading them either. Ben?
There was a problem hiding this comment.
Hi @neolit123,
I've addressed your feedback here. I first try to get the files at the remote URIs, and if that fails I default back to the constant values.
There was a problem hiding this comment.
waiting on Ben's comments on this one. pulling from the release repo is not ideal too.
There was a problem hiding this comment.
Hi @neolit123,
Sounds good. I'm happy to take this in any direction. I figured the constants were what @BenTheElder was thinking since Kind already embeds the Weave manifest as a constant.
There was a problem hiding this comment.
The weave constant is not per-kubernetes version. This one is a bit more problematic.
There was a problem hiding this comment.
Downloading it separately is also not super ideal, will have to poke around the tarballs a bit. There should be a way to consistently obtain this 🤔
There was a problem hiding this comment.
The TODO about our own config is stale and wrong but the overall situation for these has not been good :(
There was a problem hiding this comment.
Hi @BenTheElder,
The weave constant is not per-kubernetes version. This one is a bit more problematic.
And yet I also cannot find the affected files in any of the k/k repos except for release. So while they may be specific to the k/k version, they're not seemingly available per build. I initially wanted to use the version grokked from kubernetes.tar.gz and get the service and kubadm conf file specific for that version. Then I realized those were not available.
There was a problem hiding this comment.
And yet I also cannot find the affected files in any of the k/k repos except for release.
there are systemd unit files in kubernetes/kubernetes in the build directory, the non-versioned k/release files are considered a bug and have caused many problems (and they may change inbetween releases happening, so checking in a static version here may not be right)
pkg/build/kube/tarbits.go
Outdated
|
|
||
| // Get the location where to the remote files are cached locally. | ||
| if b.tarRoot == "" { | ||
| currentUser, err := user.Current() |
There was a problem hiding this comment.
this is entering a retrospective minefield of the standard go library.
nowadays this should be portable to all the OSes..
There was a problem hiding this comment.
Hi @neolit123,
I'm not sure if your comment was more opining the portability or if you're suggesting a change?
FWIW the user.Current() call is portable. I had originally used os.UserHomeDir, but the CI builds failed. I don't know when UserHomeDir was introduced, but it must be in a version later than the one used to build Kind.
There was a problem hiding this comment.
it was just a comment.
the go in the CI that builds kind should be 1.12.1.
alternatively this can be just a mandatory folder specified by the user or defaulting to ./ instead of defaulting to the user home path.
There was a problem hiding this comment.
Hi @neolit123,
I opted to avoid adding any new flags since @BenTheElder indicated he would like all the flags to apply to all the commands.
I'm fervently against the idea of ./ because of the size of what's being downloaded. It's much friendlier to local development to support a well-defined path for the tarbits to avoid re-downloading files when they're already cached locally.
I really don't think it's a problem to use the user's home directory. It's a fairly standard approach for storing app data.
There was a problem hiding this comment.
Why should they be cached at all? When do entries get evicted?
There was a problem hiding this comment.
Hi @BenTheElder,
See my comment above. I agree that perhaps caching ci/ builds is unnecessary. Caching release builds, however, seems to be not the worst idea. At the very minimum having an opt-in mode for caching would be nice for local devs.
akutz
force-pushed
the
feature/build-from-tar
branch
from
7e62c21 to
4e42ea8
Compare
k8s-ci-robot
added
size/XL
akutz
force-pushed
the
feature/build-from-tar
branch
3 times, most recently
from
e0e2e6b to
45409fc
Compare
|
My two cents
More specifically if we can move the extract logic moved in a separated package / decoupled with an interface this will cleanup the code and additionally unblock other issues/feature, like e.g extract the second Kubernetes version required for testing upgrades in kind |
+1 i'm all in favor of whatever facilitates upgrades. |
|
Hi @fabriziopandini,
Let me preface my response by saying I like where your head is at. I'm a big fan of abstraction layers. I do think there's merit in what you're suggesting, and I think it's possible to get there with some future iterations. But... (there's always a but, isn't there?) ... I'm also not sure it's necessary at this time. The original commit tree for this PR had two new Bit implementations, From what I've seen, the
I've no issue seeing the evolution of the Or perhaps you were you suggesting that // TarBitsTractorBeam is implemented by types that pull the image archives and binaries
// from a remote location.
type TarBitsTractorBeam interface {
// CanLockOn returns a flag that indicates whether this tractor beam is able to pull
// the bits from the given source.
CanLockOn(src string) bool
// Pull engages the tractor beam to download the bits from the remote source
// and store them in the provided destination.
Pull(src, dst string) error
}There are half a dozen to 3,000 ways to design most things, and even after the 20+ years I've been doing this, I'd be lying if I didn't say that I still tend to delay features until I think they're "perfect." In reality nothing ever is. In fact, one of the lessons I'm still learning nearly two decades later is that it's sometimes best to start with what works and iterate on it, time and resources permitting. This PR works, and so I'm a little hesitant to delay such an important feature for a design change that I'm unsure is entirely necessary at this time. Still, I'd love to hear more about what you were thinking. I think you were referring to the tractor beam idea, but I don't want to assume. Thanks! -- p.s.
Excuse me? I'll have you know that: My code is pretty, and witty and gay :) |
|
Hi @fabriziopandini, May I ask if you were thinking something along the lines of: // GetTarBitsExtractorFor returns the TarBitsExtractor for the provided
// kubeRoot value. A nil value is returned if there is no extractor
// available for the provided kubeRoot.
func GetTarBitsExtractorFor(kubeRoot string) TarBitsExtractor {
if strings.HasPrefix(kubeRoot, "file://") {
return TarBitsFileExtractor{}
} else if strings.HasPrefix(kubeRoot, "http:") ||
strings.HasPrefix(kubeRoot, "https:") {
return TarBitsHTTPExtractor{}
} else if _, err := apimv.ParseGeneric(kubeRoot); err == nil {
return TarBitsSemVerExtractor{}
} else if strings.HasPrefix(kubeRoot, "ci/") {
return TarBitsCIBuildExtractor{}
} else if strings.HasPrefix(kubeRoot, "release/") {
return TarBitsReleaseBuildExtractor{}
}
return TarBitsFileExtractor{}
}
// TarBitsExtractor is implemented by types that can extract the pre-built
// image archives (tar files), the version file from kubernetes.tar.gz, and
// the kubeadm, kubectl, and kubelet binaries.
//
// Files are extracted to $KIND_TARBITS/VERSION. The default value of
// $KIND_TARBITS is $HOME/.kind/tarbits.
type TarBitsExtractor interface {
// Extract extracts the pre-built image archives (tar files), the
// version file from kubernetes.tar.gz, and the kubeadm, kubectl, and
// kubelet binaries. The returned map adheres to the same rules as the
// map returned by Bits.Paths.
Extract(src string) (map[string]string, error)
}
// TarBitsFileExtractor verifies the extracted bits on the local disk.
type TarBitsFileExtractor struct{}
// Extract implements TarBitsExtractor.Extract.
func (b TarBitsFileExtractor) Extract(src string) (map[string]string, error) {
return nil, nil
}
// TarBitsHTTPExtractor extracts the bits for a given HTTP endpoint.
type TarBitsHTTPExtractor struct{}
// Extract implements TarBitsExtractor.Extract.
func (b TarBitsHTTPExtractor) Extract(src string) (map[string]string, error) {
return nil, nil
}
// TarBitsSemVerExtractor extracts the bits for a release by the given semantic
// version.
type TarBitsSemVerExtractor struct{}
// Extract implements TarBitsExtractor.Extract.
func (b TarBitsSemVerExtractor) Extract(src string) (map[string]string, error) {
return nil, nil
}
// TarBitsCIBuildExtractor extracts the bits for a given CI build.
type TarBitsCIBuildExtractor struct{}
// Extract implements TarBitsExtractor.Extract.
func (b TarBitsCIBuildExtractor) Extract(src string) (map[string]string, error) {
return nil, nil
}
// TarBitsReleaseBuildExtractor extracts the bits for a given release build.
type TarBitsReleaseBuildExtractor struct{}
// Extract implements TarBitsExtractor.Extract.
func (b TarBitsReleaseBuildExtractor) Extract(src string) (map[string]string, error) {
return nil, nil
}I will say that this has made me consider that it's unnecessary to redownload |
|
@akutz thanks for your detailed answers. I really appreciate the effort you did for understanding my rant. Your second comment is definitely hitting the point. If it is possible to create a new public package What is important is that the code of your I also agree on the fact that any change to Bits interface/behaviour is out of scope of this PR. |
|
I don't see why you would even need an extractor object / interface? Just have a function to map a source spec to downloading a tarball to a specified location on disk, and then you can have unrelated code extract the tarball and pick out files. That can be pulled out, but I don't think kind should be in the business of offerring this publicly. Perhaps a copy in another repo? I also don't think this should extract in $HOME. If we want to repeatedly use ci/latest, reuse the built image? This should probably get downloaded to a tempdir and removed on exit. |
|
@BenTheElder I agree extract is a "core" function that is not kind specific and can be reused across projects. |
Hi @BenTheElder, HA! I was just writing this when I saw your comment:
It also occurred to me all you really need is a series of functions, not an interface. |
Hi @BenTheElder, Except if the primary use case is for building in Prow jobs, who cares if they're not removed? I agree when building locally they should be, so perhaps anything extracted from |
|
Hi @BenTheElder, One more item regarding the caching. For purposes of local dev, what about using |
akutz
force-pushed
the
feature/build-from-tar
branch
from
45409fc to
b38e603
Compare
|
Hi @fabriziopandini, Referring to your desire to use the logic externally, what do you think of the new |
This patch adds a Clean function to the Bits interface to allow a Bits implementation to clean up after itself.
akutz
force-pushed
the
feature/build-from-tar
branch
2 times, most recently
from
e81039d to
ee83d4d
Compare
|
Hi @fabriziopandini, Thank you for taking the time to explain things on Slack. FWIW, I've updated the PR so that the function // TarBitsExtract extracts bits from a given kubeRoot to the given extractPath
// Please see NewTarBits for information on the valid kubeRoot values.
func TarBitsExtract(kubeRoot, extractPath string) error {
bits, err := NewTarBits(kubeRoot)
if err != nil {
return err
}
tarBits := bits.(*TarBits)
if _, err := tarBits.extract(tarBits.kubeRoot, extractPath); err != nil {
return err
}
return nil
} |
|
I'm OK to leave it for this PR I guess, but I think the I also kinda think exported libraries for extracting should be more general and live in a shared location for eg kubetest to consume, but I'm not sure where that would be right now, and if we're just exporting for kinder to vendor without stability guarantees I guess that's ok. |
akutz
force-pushed
the
feature/build-from-tar
branch
from
ee83d4d to
327f1c4
Compare
|
Hi @BenTheElder,
I renamed
I concur, and I'd happily spend an hour moving the |
This patch makes it possible to stage bits as a distinct step in the TarBits Build process by invoking pkg/build/kube.TarBitsExtract(buildID, extractPath string).
akutz
force-pushed
the
feature/build-from-tar
branch
from
327f1c4 to
1609258
Compare
| @@ -315,6 +315,9 @@ func (c *BuildContext) buildImage(dir string) error { | |||
| return err | |||
| } | |||
|
|
|||
| // Let Bits clean up after itself. | |||
| c.bits.Clean() | |||
There was a problem hiding this comment.
this won't run if we return early
There was a problem hiding this comment.
Hi @BenTheElder,
I considered that, and I thought that perhaps it would be desirable to leave the bits/data that failed to build for purposes of figuring out what went wrong...
There was a problem hiding this comment.
Seems reasonable for now. Might revisit later 👍
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
@BenTheElder is this something that we want to add to 0.5? |
|
any updates on this PR? |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
@akutz: 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. |
k8s-ci-robot
added
the
needs-rebase
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
@akutz: The following tests failed, say
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. |
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
@fejta-bot: Closed this PR. In response to this:
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. |
Hi all,
I'd love to get some feedback on this. I have a need for it, and I discussed it briefly with @BenTheElder on Slack. I'm aware others have ideas about it, and I'd love to kick off the discussion with this PR. Thanks all!
This PR introduces support for building images from tarballs and fixes #381. For example:
$ KIND_TARBITS=$HOME/.kind/tarbits GOOS=linux ./kind build node-image --type tar --kube-root ci/latest --image kindest/node:ci-latest Building version v1.15.0-alpha.0.1381+8af1bf313efeea Created symlink /etc/systemd/system/multi-user.target.wants/kubelet.service → /kind/systemd/kubelet.service. Created symlink /etc/systemd/system/kubelet.service → /kind/systemd/kubelet.service. Pulling: k8s.gcr.io/pause:3.1 Pulling: k8s.gcr.io/etcd:3.3.10 Pulling: k8s.gcr.io/coredns:1.3.1 Pulling: weaveworks/weave-kube:2.5.1 Pulling: weaveworks/weave-npc:2.5.1 sha256:99649ee63ff408e72c0261858c9a13edc17a5701f040e9167ab84606dede10a9$ ls $HOME/.kind/tarbits/v1.15.0-alpha.0.1437+b3824cd094f73d total 24 drwxr-xr-x 6 akutz staff 192B Mar 22 14:36 ./ drwxr-xr-x 6 akutz staff 192B Mar 22 14:41 ../ -rw-r--r-- 1 akutz staff 898B Mar 22 14:36 10-kubeadm.conf drwxr-xr-x 3 akutz staff 96B Mar 22 14:35 bin/ -rw-r--r-- 1 akutz staff 227B Mar 22 14:36 kubelet.service -rw-r--r-- 1 akutz staff 35B Mar 22 14:35 versionThe
-typeistarand-kube-rootmay be set to one of three values:A local filesystem path containing the files listed above structure that matches the file structure of the public, GCS buckets.
An HTTP address that contains the files listed above in a structure that adheres to the layout of the public GCS buckets kubernetes-release and kubernetes-release-dev. For example, if tarRoot is set to https://k8s.ci/v1.13 then the following URIs should be valid:
A valid semantic version for a released version of Kubernetes or begins with
ci/orrelease/. If the string matches any of these then the value is presumed to be a CI or release build hosted on one of the public GCS buckets.This option also supports values ended with
.txt, ex.ci/latest.txt. In fact, all values beginning withci/orrelease/are first checked to see if there's a matching txt file for that value. For example, ifci/latestis provided then before assuming that is a directory,ci/latest.txtis queried.Some additional notes:
file://explicitly indicates the given tarRoot is a local file path.NEW_TEMP_DIR/VERSION/GOOS/GOARCHwhereVERSIONis the value extracted from the "version" file in thekubernetes.tar.gztarball. If$KIND_TARBITSis defined it will be used instead of a new temporary directory.Finally, per @BenTheElder's
TODO:The files
kubelet.serviceand10-kubeadm.confare written to disk byTarBitsfrom string constants inpkg/build/kube/files.goTODO
versionfile. I realize that the existing version logic uses extended information from the Git version, but that isn't readily available (or rather readily apparent to me if it is available) in the tar files.