pkg/asset/ignition: add initial implementation #206

staebler · 2018-09-05T16:15:10Z

https://jira.coreos.com/browse/CORS-765

This generates the worker and master ignition config files that are generated by the tectonic node controller and the bootstrap ignition config file generated by the assets step.

The bootstrap ignition config file is missing manifest files that will come from dependent assets that have not yet been implemented.

wking · 2018-09-06T18:45:50Z

I dunno how much work this would be, so feel free to tell me it's too much, but it would be nice if we used these generators in the old installer as gradual replacements for steps/assets. Something like:

Update installer/installer/pkg/workflow/install.go to create assets with the pkg/asset tooling (in a generateAssetsStep?.
Update modules/bootkube/assets.tf and similar to slurp the generated files into the existing Terraform workflow.

That way we keep the asset templating DRY and ensure the Go-generated assets are covered by CI while we work up the next-gen installer.

staebler · 2018-09-07T17:20:04Z

@wking I think there is a lot of value in what you are proposing. I wonder how close we are to completing the switch to use pkg/assets for everything, though.

wking · 2018-09-07T18:02:47Z

I wonder how close we are to completing the switch to use pkg/assets for everything, though.

It doesn't have to be all-or-nothing, does it? I think we can replace the old-style asset generation per-asset as we get new-style replacements.

yifan-gu · 2018-09-10T18:35:55Z

@staebler Could you split the PR into separate commits for easier reviewing?

crawford · 2018-09-11T19:02:42Z

pkg/ignition/config.go

This can all be collapsed into the following line:

i.Storage.Files = append(i.Storage.Files, files...)

Of course. That was pretty boneheaded of me.

crawford · 2018-09-11T19:02:55Z

pkg/ignition/config.go

Same as above.

crawford · 2018-09-11T19:03:20Z

pkg/ignition/file.go

Why not just import all of the Ignition types?

crawford · 2018-09-11T19:03:41Z

pkg/ignition/config.go

Please import the Ignition config package and use the MaxVersion from there.

crawford · 2018-09-11T19:04:21Z

pkg/asset/ignition/commonsystemd.go

These files are no longer in the installer repo.

crawford · 2018-09-11T19:05:01Z

pkg/asset/ignition/node.go

No longer present.

crawford · 2018-09-11T19:06:55Z

pkg/asset/ignition/node.go

This is potentially going away with #187.

crawford · 2018-09-11T19:09:57Z

pkg/asset/ignition/master.go

This should be read from the install config.

abhinavdahiya · 2018-09-11T20:37:24Z

pkg/asset/ignition/templates/etcd_member.go

@staebler Why are these etcd templates part of the ignition assets?

abhinavdahiya · 2018-09-11T20:40:43Z

@staebler The ignition files are owned by MachineConfigOperator for masters and workers. The installer only creates ignition configs that point to cluster ignition server...

the installer currently creates ignition configs using this code for masters and worker:
https://github.com/openshift/installer/blob/master/installer/pkg/config-generator/ignition.go

staebler · 2018-09-11T21:02:51Z

@abhinavdahiya OK. Good to know. I had it in my head that the installer was going to pass to the MCO the final ignition configs for the masters and workers.

crawford · 2018-09-11T20:34:44Z

pkg/asset/ignition/bootstrap.go

You can use the util package to do this.

crawford · 2018-09-11T20:35:28Z

pkg/asset/ignition/bootstrap.go

I thought this was dropped as well.

crawford · 2018-09-11T20:38:13Z

cmd/openshift-install/main.go

I prefer to use the following to indicate that the variable hasn't been assigned:

var targetAssets []asset.Asset

crawford · 2018-09-11T20:39:17Z

pkg/asset/ignition/bootstrap.go

"tlsCertDirectory is the directory on the bootstrap node..."

crawford · 2018-09-11T20:41:51Z

pkg/asset/ignition/bootstrap.go

The username must be core.

crawford · 2018-09-11T20:52:59Z

pkg/asset/ignition/bootstrap.go

Why does this default to one?

That is what the documentation here says the default is.

crawford · 2018-09-11T20:59:28Z

pkg/asset/ignition/node.go

This has been removed.

crawford · 2018-09-11T21:01:08Z

pkg/asset/ignition/templates/bootkube_sh.go

We aren't running shellcheck on these templates, so these comments can be removed.

crawford · 2018-09-11T21:02:18Z

pkg/asset/ignition/templates/bootstrap_kubelet.go

This has been removed.

crawford · 2018-09-11T21:02:53Z

pkg/asset/ignition/templates/master_kubelet.go

This has been removed.

wking · 2018-09-11T22:02:06Z

pkg/asset/ignition/bootstrap.go

"certs" -> "assets", since we also store keys in this directory.

wking · 2018-09-11T22:05:50Z

pkg/asset/ignition/bootstrap.go

Do we want to address these in this PR? If not, I'd recommend dropping addBootstrapConfigFilesToIgnitionConfig completely and coming back around to these in follow-up work if/when we decide we need them.

I made my changes directly.

abhinavdahiya · 2018-09-14T20:20:19Z

pkg/asset/ignition/bootstrap.go

Why don't we just use the HyperkubeImage instead of KubeletImageURL and KubeletImageTag ?

Actually KubeletImage* are not being used.

It's actually not used anywhere... I thought Go warned about unused stuff.

https://golang.org/pkg/text/template/#Template.Option

abhinavdahiya · 2018-09-14T20:25:02Z

pkg/asset/ignition/bootstrap.go

all these functions should be under installconfig's package?

abhinavdahiya · 2018-09-14T20:26:29Z

pkg/asset/ignition/node.go

same thing; installconfig package

abhinavdahiya · 2018-09-14T20:27:27Z

pkg/asset/ignition/node.go

defaultIgnitionConfig / baseIgnitionConfig ?

How about pointerIgnitionConfig?

abhinavdahiya · 2018-09-14T21:10:28Z

/lint

openshift-ci-robot

@abhinavdahiya: 9 warnings.

Details

In response to this:

/lint

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.

openshift-ci-robot · 2018-09-14T21:10:38Z

pkg/asset/ignition/templates/kubelet.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-09-14T21:10:38Z

pkg/asset/ignition/templates/tectonic.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-09-14T21:10:38Z

pkg/asset/ignition/worker.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-09-14T21:10:38Z

pkg/asset/ignition/node.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-09-14T21:10:38Z

pkg/asset/ignition/bootstrap.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-09-14T21:10:38Z

pkg/asset/installconfig/installconfig.go

Golint comments: exported function ClusterDNSIP should have comment or be unexported. More info.

openshift-ci-robot · 2018-09-14T21:10:39Z

pkg/asset/ignition/stock.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-09-14T21:10:39Z

pkg/asset/ignition/templates/bootkube.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

openshift-ci-robot · 2018-09-14T21:10:39Z

pkg/asset/ignition/master.go

Golint comments: should have a package comment, unless it's in another file for this package. More info.

crawford · 2018-09-14T21:11:37Z

Why don't we run lint by default?

EDIT: We do, we just run it with different settings than /lint.

abhinavdahiya

/lgtm

This looks super neat, tests are pretty nice.

openshift-ci-robot · 2018-09-15T00:17:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, staebler

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya,staebler]

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

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 5, 2018

openshift-ci-robot requested review from smarterclayton and wking September 5, 2018 16:15

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2018

staebler changed the title ~~WIP: Add assets for generating the ignition config files~~ Add assets for generating the ignition config files Sep 11, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2018

crawford suggested changes Sep 11, 2018

View reviewed changes

pkg/asset/ignition/master.go Outdated

Copy link

Contributor

crawford Sep 11, 2018

Choose a reason for hiding this comment

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

This should be read from the install config.

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 11, 2018

abhinavdahiya reviewed Sep 11, 2018

View reviewed changes

pkg/asset/ignition/templates/etcd_member.go Outdated

Copy link

Contributor

abhinavdahiya Sep 11, 2018

Choose a reason for hiding this comment

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

@staebler Why are these etcd templates part of the ignition assets?

crawford previously requested changes Sep 11, 2018

View reviewed changes

wking reviewed Sep 11, 2018

View reviewed changes

pkg/asset/ignition/bootstrap.go Outdated

Copy link

Member

wking Sep 11, 2018

Choose a reason for hiding this comment

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

"certs" -> "assets", since we also store keys in this directory.

wking reviewed Sep 11, 2018

View reviewed changes

crawford changed the title ~~Add assets for generating the ignition config files~~ pkg/assetlignition: add initial implementation Sep 14, 2018

abhinavdahiya reviewed Sep 14, 2018

View reviewed changes

pkg/asset/ignition/bootstrap.go Outdated

Copy link

Contributor

abhinavdahiya Sep 14, 2018

Choose a reason for hiding this comment

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

all these functions should be under installconfig's package?

abhinavdahiya reviewed Sep 14, 2018

View reviewed changes

pkg/asset/ignition/node.go Outdated

Copy link

Contributor

abhinavdahiya Sep 14, 2018

Choose a reason for hiding this comment

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

same thing; installconfig package

abhinavdahiya reviewed Sep 14, 2018

View reviewed changes

openshift-ci-robot reviewed Sep 14, 2018

View reviewed changes

abhinavdahiya changed the title ~~pkg/assetlignition: add initial implementation~~ pkg/asset/ignition: add initial implementation Sep 14, 2018

pkg/assetlignition: add initial implementation

7af7b5c

openshift-ci-robot assigned abhinavdahiya Sep 15, 2018

abhinavdahiya approved these changes Sep 15, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2018

openshift-merge-robot merged commit de9d2c6 into openshift:master Sep 15, 2018

abhinavdahiya mentioned this pull request Sep 15, 2018

modules/bootkube/resources/bootkube: Restore --tmpfs #253

Merged

wking mentioned this pull request Sep 15, 2018

installer/pkg/config: Support loading InstallConfig YAML #236

Closed

wking mentioned this pull request Mar 26, 2019

*: move to Ignition v3 #1468

Closed

pkg/asset/ignition: add initial implementation #206

pkg/asset/ignition: add initial implementation #206

Uh oh!

Conversation

staebler commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented Sep 6, 2018

Uh oh!

staebler commented Sep 7, 2018

Uh oh!

wking commented Sep 7, 2018

Uh oh!

yifan-gu commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staebler commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staebler commented Sep 5, 2018 •

edited

Loading

abhinavdahiya commented Sep 11, 2018 •

edited

Loading