inventory assets - mapping and uploading kubernetes containers #3025

chrislovecnm · 2017-07-21T20:33:14Z

No description provided.

justinsb · 2017-07-22T04:38:39Z

pkg/assets/builder.go

@@ -95,7 +99,13 @@ func (a *AssetBuilder) remapImage(image string) (string, error) {
 	}

 	registryMirror := os.Getenv("DEV_KOPS_REGISTRY_MIRROR")


We can dump the env var now that we have AssetRemapping in the API.. it was just to decouple the two PRs

justinsb · 2017-07-22T04:39:37Z

upup/pkg/fi/cloudup/phase.go

@@ -22,4 +22,5 @@ const (
 	PhaseIAM     Phase = "iam"
 	PhaseNetwork Phase = "network"
 	PhaseCluster Phase = "cluster"
+	PhaseAssetsUpdate Phase = "assets-update"


Probably just assets I'd say

justinsb · 2017-07-22T04:41:12Z

pkg/model/components/context.go

-		return "gcr.io/google_containers/" + component + ":" + "v" + clusterSpec.KubernetesVersion, nil
+		image := "gcr.io/google_containers/" + component + ":" + "v" + clusterSpec.KubernetesVersion
+
+		assetsBuilder := assets.NewAssetBuilder(clusterSpec.Assets)


OK, but I think we'll have to use the same AssetBuilder if we want this to actually work... in the kubelet pause container it gets put into the OptionsContext

Should now be identical to this:

https://github.com/kubernetes/kops/blob/master/pkg/model/components/kubelet.go#L183-L188

justinsb · 2017-07-22T04:42:23Z

upup/pkg/fi/cloudup/apply_cluster.go

+			assetBuilder.Assets = append(assetBuilder.Assets, asset)
+		}
+
+		canonicalLocation := "gcr.io/google_containers/pause-amd64:3.0"


We definitely don't need to special-case this one

justinsb · 2017-07-22T04:45:16Z

upup/pkg/fi/cloudup/apply_cluster.go

@@ -700,6 +716,39 @@ func (c *ApplyClusterCmd) Run() error {

 	tf.AddTo(l.TemplateFunctions)

+	if cluster.Spec.Assets != nil && cluster.Spec.Assets.ContainerRegistry != nil {
+		kubernetesComponents := []string{"kube-proxy", "kube-apiserver", "kube-controller-manager", "kube-scheduler"}


Anything that calls Image above should do this automatically. That is called by proxy, kcm & scheduler.

kube-apiserver is still using the models/config/kube-apiserver (!)

But because we are not passing around the same assets builder we are loosing the information. I am rebuilding it here. Any recommendations?

Yah we need to cleanup api server, and git rid of the non go model.

justinsb · 2017-07-22T04:45:59Z

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

@@ -39,6 +39,8 @@ type BootstrapChannelBuilder struct {
 var _ fi.ModelBuilder = &BootstrapChannelBuilder{}

 func (b *BootstrapChannelBuilder) Build(c *fi.ModelBuilderContext) error {
+
+


Can we remove this one?

justinsb · 2017-07-22T04:47:09Z

pkg/assets/builder.go

@@ -43,8 +45,10 @@ type Asset struct {
 	CanonicalLocation string
 }

-func NewAssetBuilder() *AssetBuilder {
-	return &AssetBuilder{}
+func NewAssetBuilder(assetRemapping *kops.Assets) *AssetBuilder {


Depending on the sequencing of merges here, we will likely want to take a cluster as input... but we'll cross that bridge when we come to it

justinsb · 2017-07-22T04:48:12Z

pkg/model/components/context.go

@@ -125,8 +126,16 @@ func Image(component string, clusterSpec *kops.ClusterSpec) (string, error) {
 		return "gcr.io/google_containers/kubedns-amd64:1.3", nil


Might as well send this one through RemapImage as well...

We may actually need to remove this from the API. This is not being used at all and really need to get out of the API. It is an Addon ...

Anoher PR, but advice?

We do not want to remap this image, as it is already in addons. Why add another one?

Maybe change it to glog.Fatalf if this is unused

justinsb · 2017-07-22T04:49:24Z

upup/pkg/fi/cloudup/apply_cluster.go

 			iamLifecycle = lifecyclePointer(fi.LifecycleExistsAndWarnIfChanges)
 			networkLifecycle = lifecyclePointer(fi.LifecycleExistsAndWarnIfChanges)
 		} else {
+			//assetTransferLifecycle = lifecyclePointer(fi.LifecycleIgnore)


LifecycleExistsAndValidates I believe - we want to know if images aren't in place.

In air gapped and other proxied environments you often will not have access to gcr.io, docker.io or other public container registries. I would recommend an API level option to override. We can definitely check if containers exist in the assets registry or the regular registry, but I would recommend failing.

justinsb · 2017-07-22T04:50:25Z

upup/pkg/fi/cloudup/loader.go

@@ -202,7 +203,10 @@ func (l *Loader) addAssetCopyTasks(assets []*assets.Asset) error {
 				Name:        fi.String(asset.DockerImage),
 				SourceImage: fi.String(asset.CanonicalLocation),
 				TargetImage: fi.String(asset.DockerImage),
+				// TODO


Suggest threading it through BuildTasks, but you can leave a TODO for me if it gets messy

k8s-github-robot · 2017-07-23T02:04:56Z

@chrislovecnm PR needs rebase

justinsb · 2017-07-24T03:46:57Z

cmd/kops/update_cluster.go

-			phase = cloudup.PhaseIAM
-		case "network":
+		case string(cloudup.PhaseAssets):
+			phase = cloudup.PhaseAuth


typo: should be PhaseAssets I believe

k8s-github-robot · 2017-08-04T00:01:38Z

@chrislovecnm PR needs rebase

justinsb · 2017-08-22T06:26:25Z

pkg/model/components/context.go

@@ -125,8 +126,16 @@ func Image(component string, clusterSpec *kops.ClusterSpec) (string, error) {
 		return "gcr.io/google_containers/kubedns-amd64:1.3", nil


Maybe change it to glog.Fatalf if this is unused

justinsb · 2017-08-22T06:28:21Z

pkg/model/components/context.go

+		if err != nil {
+			return "", fmt.Errorf("unable to remap container %q: %v", image, err)
+		}
+		return image, nil
 	}

 	baseURL := clusterSpec.KubernetesVersion


Maybe a comment here about why we're not remapping this case. (It's not self-evident to me why we are not.)

justinsb · 2017-08-22T06:30:07Z

upup/pkg/fi/cloudup/apply_cluster.go

 			iamLifecycle = lifecyclePointer(fi.LifecycleExistsAndWarnIfChanges)
 			networkLifecycle = lifecyclePointer(fi.LifecycleExistsAndWarnIfChanges)
 		} else {
+			stageAssetsLifecycle = lifecyclePointer(fi.LifecycleIgnore)


LifecycleExistsAndValidates I believe - we want to check it for problems I think.

Once we have the code for containers and files existing I aggree, will file an issue

justinsb · 2017-08-22T06:30:54Z

upup/pkg/fi/cloudup/loader.go

@@ -150,7 +149,7 @@ func ignoreHandler(i *loader.TreeWalkItem) error {
 	return nil
 }

-func (l *Loader) BuildTasks(modelStore vfs.Path, models []string, assetBuilder *assets.AssetBuilder) (map[string]fi.Task, error) {
+func (l *Loader) BuildTasks(modelStore vfs.Path, models []string, assetBuilder *assets.AssetBuilder, lifecycle *fi.Lifecycle) (map[string]fi.Task, error) {


Renaming lifecycle to assetsLifecycle would make this clearer

justinsb · 2017-08-22T06:36:20Z

upup/pkg/fi/cloudup/template_functions.go

@@ -76,8 +75,6 @@ func (tf *TemplateFunctions) AddTo(dest template.FuncMap) {

 	dest["HasTag"] = tf.HasTag

-	dest["Image"] = tf.Image


Hooray - another step towards getting rid of all the templates!

I filed an issue for getting rid of the API server templates

justinsb · 2017-08-22T06:37:00Z

/lgtm

k8s-github-robot · 2017-08-22T06:37:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, justinsb

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm,justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-22T06:37:21Z

/test all [submit-queue is verifying that this PR is safe to merge]

justinsb · 2017-08-22T09:00:44Z

/test pull-kops-e2e-kubernetes-aws

chrislovecnm · 2017-08-22T16:36:59Z

/re-test

justinsb · 2017-08-22T16:55:32Z

/retest

justinsb · 2017-08-22T22:00:52Z

/retest

justinsb · 2017-08-22T23:15:54Z

Flakiness is being tracked in upstream issue: kubernetes/kubernetes#51128, hopefully fixed by kubernetes/kubernetes#51144

eparis · 2017-08-23T00:03:34Z

/retest
merged the referenced fix

k8s-github-robot · 2017-08-23T01:41:14Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-23T02:15:30Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2017

justinsb reviewed Jul 22, 2017

View reviewed changes

chrislovecnm force-pushed the mapping-and-uploading-kubernetes-assets branch from 169c965 to 96500de Compare July 23, 2017 01:32

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2017

chrislovecnm force-pushed the mapping-and-uploading-kubernetes-assets branch from 96500de to 2043c64 Compare July 23, 2017 02:25

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2017

chrislovecnm force-pushed the mapping-and-uploading-kubernetes-assets branch from 2144d9a to c51db4c Compare July 23, 2017 06:36

justinsb reviewed Jul 24, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2017

chrislovecnm force-pushed the mapping-and-uploading-kubernetes-assets branch from c51db4c to 4228b52 Compare August 21, 2017 21:02

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2017

chrislovecnm added 2 commits August 21, 2017 15:16

mapping-and-uploading-kube-assets

2afdb5a

updating phases

7efb0f3

chrislovecnm force-pushed the mapping-and-uploading-kubernetes-assets branch from 4228b52 to c280cf3 Compare August 21, 2017 21:17

refactoring for tracking via assets container registry

6071ce7

chrislovecnm force-pushed the mapping-and-uploading-kubernetes-assets branch from c280cf3 to 6071ce7 Compare August 21, 2017 23:29

justinsb reviewed Aug 22, 2017

View reviewed changes

k8s-ci-robot assigned justinsb Aug 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2017

k8s-github-robot merged commit 2d7fb51 into kubernetes:master Aug 23, 2017

chrislovecnm deleted the mapping-and-uploading-kubernetes-assets branch December 30, 2017 20:45

johngmyers mentioned this pull request Jun 1, 2021

Create document on asset repositories #11654

Merged

		@@ -95,7 +99,13 @@ func (a *AssetBuilder) remapImage(image string) (string, error) {
		}

		registryMirror := os.Getenv("DEV_KOPS_REGISTRY_MIRROR")

		@@ -39,6 +39,8 @@ type BootstrapChannelBuilder struct {
		var _ fi.ModelBuilder = &BootstrapChannelBuilder{}

		func (b BootstrapChannelBuilder) Build(c fi.ModelBuilderContext) error {

		@@ -125,8 +126,16 @@ func Image(component string, clusterSpec *kops.ClusterSpec) (string, error) {
		return "gcr.io/google_containers/kubedns-amd64:1.3", nil

		@@ -76,8 +75,6 @@ func (tf *TemplateFunctions) AddTo(dest template.FuncMap) {

		dest["HasTag"] = tf.HasTag

		dest["Image"] = tf.Image

inventory assets - mapping and uploading kubernetes containers #3025

inventory assets - mapping and uploading kubernetes containers #3025

Conversation

chrislovecnm commented Jul 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Jul 23, 2017

Choose a reason for hiding this comment

k8s-github-robot commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

k8s-github-robot commented Aug 22, 2017

justinsb commented Aug 22, 2017

chrislovecnm commented Aug 22, 2017

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

justinsb commented Aug 22, 2017

eparis commented Aug 23, 2017

k8s-github-robot commented Aug 23, 2017

k8s-github-robot commented Aug 23, 2017