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.

Sign up for GitHub

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

Jump to bottom

etcd using asset builder #3661

Merged
merged 2 commits into from
Oct 27, 2017
Merged

etcd using asset builder #3661

merged 2 commits into from
Oct 27, 2017

Conversation

chrislovecnm
Copy link
Contributor

Updating etcd to use asset builder

@chrislovecnm chrislovecnm added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 17, 2017
@chrislovecnm
Copy link
Contributor Author

/assign @gambol99 @justinsb

chrislovecnm
chrislovecnm commented Oct 17, 2017
View reviewed changes
pkg/model/components/etcd.go Outdated
// @check the version are set and if not preset the defaults
for _, x := range spec.EtcdClusters {
// @TODO if nothing is set, set the defaults. At a late date once we have a way of detecting a 'new' cluster
// we can default all clusters to v3
if x.Version == "" {
x.Version = "2.2.1"
}

if imageVersion != "" {
if imageVersion != x.Version {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the odd thing @gambol99 - I think we could technically end up with multiple different versions of etcd here. Please tell me I am wrong. I am wondering if we should move the version up a level in the structs.

Copy link
Contributor

Choose a reason for hiding this comment

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

initial the version was set in the outer struct, i recall @justinsb want it to be per etcd cluster though ..

Copy link
Contributor

@gambol99 gambol99 Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

the multiple versions was protected on the spec validation https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/validation/legacy.go#L473-L495

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I am reading that code it does not validate it.

Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gambol99 I will back out the code, can you guys move it to validation? But, I am really confused why we have the version where it is. Oh well :) I can do that validation later, but I have a long long list of things on my plate.

@chrislovecnm
Copy link
Contributor Author

/assign @KashifSaadat

I am not loving that nodeup has to call assets builder, but maybe we refactor later?

KashifSaadat
KashifSaadat reviewed Oct 18, 2017
View reviewed changes
nodeup/pkg/model/protokube.go Outdated
LogLevel: fi.Int32(4),
Master: b(t.IsMaster),
}

// TODO this is dupicate code with etcd model
image := fmt.Sprintf("gcr.io/google_containers/etcd:%s", imageVersion)
Copy link
Contributor

@KashifSaadat KashifSaadat Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Could we make this configurable to allow users to specify either the full address or version within their Cluster Spec? Similar to how it works for KubernetesVersion. Or something to move us towards the point of allowing users to specify a gcr mirror.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is exactly what https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/cluster.go#L176 does. The assests builder uses the container registry setting.

kops will upload the dependencies to you registry during the assests phase and then your cluster can use them.

This work is me wiring everything together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I missed that, ok perfect! 👍

@KashifSaadat
Copy link
Contributor

One minor comment, otherwise LGTM and is functional 👍

justinsb
justinsb reviewed Oct 18, 2017
View reviewed changes
nodeup/pkg/model/protokube.go Outdated
assets := assets.NewAssetBuilder(nil)
remapped, err := assets.RemapImage(image)
if err != nil {
glog.Errorf("unable to remap container %q: %v", image, err)
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason to fall back? Just return fmt.Errorf I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did not want to refactor method. Keep the changes small. I can refactor to return err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored

pkg/model/components/etcd.go Outdated
// @check the version are set and if not preset the defaults
for _, x := range spec.EtcdClusters {
// @TODO if nothing is set, set the defaults. At a late date once we have a way of detecting a 'new' cluster
// we can default all clusters to v3
if x.Version == "" {
x.Version = "2.2.1"
}

if imageVersion != "" {
if imageVersion != x.Version {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to validation?

pkg/model/components/etcd.go Outdated
// TODO we do not have a API place to put this. Do we use version or get another
// TODO API value?
// TODO code is duplicated in nodeup
image := fmt.Sprintf("gcr.io/google_containers/etcd:%s", imageVersion)
Copy link
Member

Choose a reason for hiding this comment

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

This is validating that the assetbuilder is working? Maybe we should explicitly put the etcd image into the nodeup spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no this is building the URL for image builder ...

Yes, we should put it in the nodeup spec. This PR or another? I would say in another PR. But shouldn't it be one container in the etcd spec?? I am seriously confused why we have the version where we have it :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want the etcd container in the clusterspec and nodeup api? Or can we do that in another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have the asset builder here do you want to use that or an API value?

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 19, 2017
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 19, 2017
@chrislovecnm
Copy link
Contributor Author

@justinsb open question is do you want me to fix the etcd API stuff in this PR or another. I would recommend that we have the etcd container URL in the cluster api. In this struct https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/cluster.go#L286. That would also be set in the nodeup API as well.

@chrislovecnm chrislovecnm added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2017
@chrislovecnm
Copy link
Contributor Author

Just had a duh moment ... we have two etcd containers running. Let me figure out how to do two assets.

@chrislovecnm chrislovecnm removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2017
justinsb
justinsb reviewed Oct 21, 2017
View reviewed changes
nodeup/pkg/model/protokube.go Outdated
LogLevel: fi.Int32(4),
Master: b(t.IsMaster),
}

// TODO this is dupicate code with etcd model
image := fmt.Sprintf("gcr.io/google_containers/etcd:%s", imageVersion)
assets := assets.NewAssetBuilder(nil)
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to pass in the Assets from the cluster spec (and not nil)?

@k8s-github-robot
Copy link

@chrislovecnm PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2017
@chrislovecnm
Copy link
Contributor Author

@justinsb PTAL

@justinsb
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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 k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2017
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 68c9036 into kubernetes:master Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@gambol99 gambol99 gambol99 left review comments

@KashifSaadat KashifSaadat KashifSaadat approved these changes

Assignees

@justinsb justinsb

@gambol99 gambol99

@KashifSaadat KashifSaadat

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@chrislovecnm @KashifSaadat @k8s-github-robot @justinsb @gambol99 @k8s-ci-robot