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

wking · 2018-09-12T16:25:47Z

So callers can start transitioning to the new approach. For a short transitional period, we'll continue to support the old config format as well.

There are a few smaller commits in this PR to setup for the meat in the final commit. Details in the commit messages.

I've only run the Go unit tests locally, so I've stuck a WIP tag on this PR until we get some through the integration tests; I wouldn't be surprised if lots of things are still broken ;).

CC @dgoodwin.

openshift-ci-robot · 2018-09-12T16:25:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking

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:

~~OWNERS~~ [wking]

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

wking · 2018-09-12T16:42:59Z

@crawford, @abhinavdahiya, while working this up, I had a few thoughts about the InstallConfig struct. I'm happy to address some/all of these it this PR, or drop them, or punt to follow-up work.

We've don't have a field for extra AWS tags (previously here). I think these are useful for setting up things like expirationDate (cluster-launch-installer-e2e: Set expirationDate tags release#1103). Do we want to add them to AWSPlatform? Or are folks who need this supposed to reach in and tweak generated assets (once we get to the full net-gen workflow)?
Setting the libvirt image path for each machine pool is tedious. Can we add a cross-pool fallback to LibvirtPlatform?
platformConfig is redundant (all of these are configs). Can we rename it to just platform?
We have several properties which are structures instead of pointers (e.g. Networking). If we make those pointers, we can get working omitempty handling for them (proposal: encoding/json, encoding/xml: support zero values of structs with omitempty golang/go#11939) at the cost of slightly more complicated Go handling (but with slightly more efficient shallow copies ;).

Thoughts?

abhinavdahiya · 2018-09-12T16:51:21Z

pkg/ipnet/ipnet.go

 	"net"
 	"reflect"
+
+	yaml "gopkg.in/yaml.v2"


We should be using github.com/ghodss/yaml

Kubernetes only does JSON marshal/unmarshal; github.com/ghodss/yaml does the jsontoyaml.
No need for special YAML parsing.

Kubernetes only does JSON marshal/unmarshal; github.com/ghodss/yaml does the jsontoyaml.

Adding explicit YAML support to pkg/ipnet was only 40 lines of non-test additions, so it seemed reasonable to me. Are we sure this is only ever going to be consumed internally?

The immediate motivation was that installer/pkg/config currently uses github.com/ghodss/yaml, presumably to support legacy properties with different YAML and JSON serialization (like BaseDomain). If we wanted to only support github.com/ghodss/yaml with ipnet and pkg/types, then we'd need to import both packages in installer/pkg/config with distinct names and use the right name depending on whether we wanted the nominally-JSON or nominally-YAML tags for a given (un)marshal. It seemed less confusing to just add support for the library installer/pkg/config was already using.

installer/installer/pkg/config/cluster.go

Line 76 in 84ab99e

BaseDomain string `json:"tectonic_base_domain,omitempty" yaml:"baseDomain,omitempty"`

this was done because they didn't want to write code to create terraform.tfvars.

this was done because they didn't want to write code to create terraform.tfvars.

So if I drop our direct gopkg.in/yaml.v2 use, do you want me to write separate code to create terraform.tfvars?

terraform.tfvars is definitely a separate structure.
we shouldn't have done what we did with installer.Config to make it directly marshal into terraform.tfvars.
The installer.Config ended up with fields like IgnitionMasters that have yaml:"-", as we didn't want user's to specify.

abhinavdahiya · 2018-09-12T16:53:54Z

pkg/types/installconfig.go


-	metav1.ObjectMeta `json:"metadata"`
+	metav1.ObjectMeta `json:"metadata" yaml:"metadata"`


Please don't add specific YAML tags, Kubernetes doesn't do that.

examples/aws.yaml

abhinavdahiya · 2018-09-12T17:02:57Z

We've don't have a field for extra AWS tags (previously here). I think these are useful for setting up things like expirationDate (openshift/release#1103). Do we want to add them to AWSPlatform? Or are folks who need this supposed to reach in and tweak generated assets (once we get to the full net-gen workflow)?

I think a single platform level extraargs makes sense.

Setting the libvirt image path for each machine pool is tedious. Can we add a cross-pool fallback to LibvirtPlatform?

the config is a one time thing. we do each pool instance type in AWS too. I would want uniformity.

platformConfig is redundant (all of these are configs). Can we rename it to just platform?

sure, sounds okay.

We have several properties which are structures instead of pointers (e.g. Networking). If we make those pointers, we can get working omitempty handling for them (golang/go#11939) at the cost of slightly more complicated Go handling (but with slightly more efficient shallow copies ;).

We should try to have no embedded structs, explicit are more clearer when marshal and pointers make sense only when they are optional or one-of (AWS Or Libvirt). Networking doesn't seem like optional.

wking · 2018-09-12T17:27:04Z

Setting the libvirt image path for each machine pool is tedious. Can we add a cross-pool fallback to LibvirtPlatform?

the config is a one time thing.

It's still nice to be convenient ;). And with #205, it seems like it might be something that is edited occasionally over the life of the cluster.

we do each pool instance type in AWS too.

But I don't have to tweak those because it's easy to give them sane defaults. I do have to provide an image path, at least until we get code that automatically grabs this from the network (and caches it somewhere for future runs?) when the caller doesn't provide a path.

I would want uniformity.

I'm fine with AWS-wide defaults for those as well, if you're ok with the cross-pool fallback approach. Should I add a LibvirtMachinePoolPlatformConfig property (as defaultMachinePlatform?) to Libvirt (and similarly for AWS). That would allow something like:

platform:
  libvirt:
    uri: qemu:///system
    defaultMachinePlatform:
      qcowImagePath: /path/to/default-image

machines:
  - name: master
    replicas: 1
    platform:
      libvirt:
        qcowImagePath: /path/to/special-case-image

  - name: worker
    replicas: 2

Networking doesn't seem like optional.

No? We currently have defaults for all of its properties, and making it omitempty (and a pointer) would allow us to write configs from Go that said "use whatever default networking you like, I don't care".

abhinavdahiya · 2018-09-12T17:43:15Z

No? We currently have defaults for all of its properties, and making it omitempty (and a pointer) would allow us to write configs from Go that said "use whatever default networking you like, I don't care".

We default, so that all the operators that depend on networking block don't need to separately make decisions about the defualts.

abhinavdahiya · 2018-09-12T17:51:03Z

I'm fine with AWS-wide defaults for those as well, if you're ok with the cross-pool fallback approach. Should I add a LibvirtMachinePoolPlatformConfig property (as defaultMachinePlatform?) to Libvirt (and similarly for AWS). That would allow something like:

I'm fine if we make that default for all platforms.

wking · 2018-09-12T22:39:05Z

No? We currently have defaults for all of its properties, and making it omitempty (and a pointer) would allow us to write configs from Go that said "use whatever default networking you like, I don't care".

We default, so that all the operators that depend on networking block don't need to separately make decisions about the defualts.

This makes sense to me when we write to the cluster (#205). I'm less clear on whether it makes sense in all cases. What if someone wants to use our package to write a cluster config before feeding it into the installer? We don't want to have to make them create their own defaults for all of these settings.

abhinavdahiya · 2018-09-12T23:13:04Z

We don't want to have to make them create their own defaults for all of these settings.

Only new openshift-installer init will make defaulting decisions. everybody else coming with InstallConfig directly must come with Networking options complete.

wking · 2018-09-12T23:18:54Z

We don't want to have to make them create their own defaults for all of these settings.

Only new openshift-installer init will make defaulting decisions. everybody else coming with InstallConfig directly must come with Networking options complete.

I think you're thinking of other consumers who are loading an InstallConfig. What about other consumers who are writing an install config to feed into init? For example:

Some personal generator uses our package to create an InstallConfig YAML. They feed that into...
openshift-install init, which injects default opinions for networking, etc.
openshift-install something-else pushes the fully-defaulted YAML into the cluster.
Operators fetch the fully-defaulted YAML from the cluster.

You want to make sure (4) doesn't need local defaults. I'm fine with that. I'd also like (1) to not need local defaults, and for that, I think we need omitempty for everything that can have a default in (2).

dgoodwin · 2018-09-13T13:07:17Z

installer/pkg/config/parser.go

@@ -42,6 +36,126 @@ func ParseConfig(data []byte) (*Cluster, error) {
 	return &cluster, nil
 }

+func parseInstallConfig(data []byte, cluster *Cluster) (err error) {
+	installConfig := &types.InstallConfig{}


It would be nice if the defaulting, and validation were both extracted separately here such that external callers could vendor and use that code. We're still trying to determine if we can re-use your InstallConfig or we have to use our own type and generate InstallConfig, but it might be good to do this regardless for futureproofing.

It would be nice if the defaulting, and validation were both extracted separately here such that external callers could vendor and use that code.

New external consumers should be using pkg/types directly; everything under installer/pkg should be considered deprecated. Having a validator for these types under pkg/ makes sense (currently pkg/asset/installconfig has user-input-time validation, but that's not useful for downstream Go consumers reading this from the cluster, e.g. openshift/machine-config-operator#47). And yeah, we'll probably need to expose the defaulting for tools that are monitoring InstallConfig to pick up post-install changes. But that will also be a pkg/ change, and this PR is trying to focus on getting support in the legacy installer/pkg/config (which has its own validation and defaulting approach). Can I punt the new APIs to follow-up work?

Totally fine with me.

dgoodwin · 2018-09-13T13:14:23Z

installer/pkg/config/parser.go

+				}
+			}
+		default:
+			return fmt.Errorf("unrecognized machine pool %q", machinePool.Name)


These names feel more like types. Additionally it looks like there's not a lot of validation here, do we require two pools with these exact names? What should happen if we see multiple worker or master pool names?

Maybe too preliminary for that but just wanted to mention.

These names feel more like types.

I agree. @abhinavdahiya, can I replace:

// Machines is the list of MachinePools that need to be installed. Machines []MachinePool `json:"machines"`

with:

Master MachinePool `json:"master" Worker MachinePool `json:"worker"

or similar?

This would leave us with a little issue in Hive. If we use InstallConfig directly in our ClusterDeployment, this would be the long term definition of all machine pools. If we go to just one master, and one worker, then we would need to duplicate this info higher up in our own portion of ClusterDeployment for the long term definition of what machine pools should exist. That would mean a little duplication of data, and a strange section of install config that was only used during install.

Kinda feels like this should be capable of being the canonical source of machine sets that should exist in the cluster.

@wking I think we decided that machinepools remain as is for now. no changes. we can expand them if needed later.

If we go to just one master, and one worker...

Just to be clear, I was suggesting one master pool (and one worker pool).

I think we decided that machinepools remain as is for now. no changes

So just die screaming if master (say) is listed multiple times or not listed at all? And silently ignore entries with names that aren't master or worker?

I would suggest leaving name a freeform field, add an explicit type, technically the two types should probably be "control-plane" and "compute", verify you have exactly one control-plane, and at least one compute pool.

wking · 2018-09-13T20:36:16Z

I've rebased this around #231 and #239 with 13e0f52 -> 3de9cef, which also adds support for loading userTags from InstallConfig YAML files.

wking · 2018-09-14T10:20:02Z

Rebased around #119 with 41f66f4 -> a87bab7.

wking · 2018-09-14T18:01:27Z

The smoke error was:

2018/09/14 10:27:17 Container cli in pod e2e-aws-smoke completed successfully
2018/09/14 13:27:13 Copying artifacts from e2e-aws-smoke into /logs/artifacts/e2e-aws-smoke
2018/09/14 13:27:13 error: unable to signal to artifacts container to terminate in pod e2e-aws-smoke, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")
2018/09/14 13:27:13 error: unable to retrieve artifacts from pod e2e-aws-smoke: could not read gzipped artifacts: unable to upgrade connection: container not found ("artifacts")
E0914 13:27:18.024001      11 event.go:200] Server rejected event '&v1.Event{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:v1.ObjectMeta{Name:".1554472e885dec97", GenerateName:"", Namespace:"ci-op-w3lsmhv3", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*v1.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]v1.OwnerReference(nil), Initializers:(*v1.Initializers)(nil), Finalizers:[]string(nil), ClusterName:""}, InvolvedObject:v1.ObjectReference{Kind:"", Namespace:"ci-op-w3lsmhv3", Name:"", UID:"", APIVersion:"", ResourceVersion:"", FieldPath:""}, Reason:"CiJobFailed", Message:"Running job pull-ci-openshift-installer-e2e-aws-smoke for PR https://github.com/openshift/installer/pull/236 in namespace ci-op-w3lsmhv3 from author wking", Source:v1.EventSource{Component:"ci-op-w3lsmhv3", Host:""}, FirstTimestamp:v1.Time{Time:time.Time{wall:0xbedf0bad81515097, ext:11210761228709, loc:(*time.Location)(0x19b8fc0)}}, LastTimestamp:v1.Time{Time:time.Time{wall:0xbedf0bad81515097, ext:11210761228709, loc:(*time.Location)(0x19b8fc0)}}, Count:1, Type:"Normal", EventTime:v1.MicroTime{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, Series:(*v1.EventSeries)(nil), Action:"", Related:(*v1.ObjectReference)(nil), ReportingController:"", ReportingInstance:""}': 'events ".1554472e885dec97" is forbidden: unable to create new content in namespace ci-op-w3lsmhv3 because it is being terminated' (will not retry!)
2018/09/14 13:27:19 Ran for 3h6m51s
error: could not run steps: could not wait for pod to complete: could not wait for pod completion: pod e2e-aws-smoke was already deleted

Looks like something just hung. In case it was a flake, I'll kick it again:

/retest

but this may be something I've broken ;).

For folks using gopkg.in/yaml.v2. Folks using github.com/ghodss/yaml will use the JSON (un)marshallers. But supporting yaml.v2 is convenient for consumers who are already importing that package (e.g. because the want different JSON and YAML serializations).

I'd missed this while rerolling 89f05da (pkg/types/installconfig: Add AWSPlatform.UserTags, 2018-09-12, openshift#239). I've also updated the UserTags comment to use "for the cluster" instead of "by the cluster". Resources can be created by the installer (not part of the cluster) or by operators living inside the cluster, but regardless of the creator, these cluster resources should get UserTags.

github.com/ghodss/yaml leans on JSON tags and (un)marshallers, but yaml.v2 depends on distinct yaml tags like the ones I'm adding here. This commit also adds a number of omitempty tags for a number of properties that could have reasonable defaults. This keeps the output more compact when serializing a partially-initialized structure.

This is now the OpenShift installer, not the Tectonic installer. And the context of the examples is clear enough without the prefix.

So callers can start transitioning to the new approach. For a short transitional period, we'll continue to support the old config format as well.

@Non-Git

Generated with: $ bazel run //:gazelle using: $ bazel version Build label: 0.16.1- (@Non-Git) Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar Build time: Mon Aug 13 16:42:29 2018 (1534178549) Build timestamp: 1534178549 Build timestamp as int: 1534178549

wking · 2018-09-15T04:09:46Z

Rebased around #206 with a87bab7 -> 48f3a4f.

wking · 2018-09-15T14:46:41Z

Smoke passed, so the e2e-aws:

Waiting for API at https://ci-op-fw6dxmqd-5849d-api.origin-ci-int-aws.dev.rhcloud.com:6443 to respond ...
Interrupted

must be a flake.

/retest

wking · 2018-09-17T17:44:39Z

We're intending to switch to cmd/openshift-install this week, which means we don't need to bother with this interim step. I've spun off #260 and #261 for the commits that will still be useful in the cmd/openshift-install world.

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

openshift-ci-robot requested review from abhinavdahiya and staebler September 12, 2018 16:25

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 12, 2018

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

abhinavdahiya suggested changes Sep 12, 2018

View reviewed changes

abhinavdahiya reviewed Sep 12, 2018

View reviewed changes

examples/aws.yaml Outdated Show resolved Hide resolved

wking mentioned this pull request Sep 12, 2018

pkg/types/machinepools: Replace platformConfig with platform #237

Merged

wking mentioned this pull request Sep 12, 2018

pkg/types/installconfig: Add AWSPlatform.UserTags #239

Merged

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2018

dgoodwin reviewed Sep 13, 2018

View reviewed changes

wking force-pushed the install-config-for-old-cmd branch from 13e0f52 to 2faf6de Compare September 13, 2018 20:26

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2018

wking force-pushed the install-config-for-old-cmd branch from 2faf6de to 3de9cef Compare September 13, 2018 20:35

wking force-pushed the install-config-for-old-cmd branch from 3de9cef to 41f66f4 Compare September 13, 2018 20:38

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2018

wking force-pushed the install-config-for-old-cmd branch from 41f66f4 to a87bab7 Compare September 14, 2018 10:19

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2018

wking added 6 commits September 14, 2018 21:07

examples: Remove 'tectonic.' prefixes

b01432b

This is now the OpenShift installer, not the Tectonic installer. And the context of the examples is clear enough without the prefix.

installer/pkg/config: Support loading InstallConfig YAML

903543d

So callers can start transitioning to the new approach. For a short transitional period, we'll continue to support the old config format as well.

wking force-pushed the install-config-for-old-cmd branch from a87bab7 to 48f3a4f Compare September 15, 2018 04:08

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2018

wking changed the title ~~WIP: installer/pkg/config: Support loading InstallConfig YAML~~ installer/pkg/config: Support loading InstallConfig YAML Sep 15, 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 15, 2018

wking mentioned this pull request Sep 15, 2018

examples/*.yaml contain non-YAML example comments #146

Closed

This was referenced Sep 17, 2018

examples/*: re-order examples and make them consistent #255

Closed

pkg/types/installconfig: Fix 'tags' -> 'userTags' JSON serialization #260

Merged

examples: Remove 'tectonic.' prefixes #261

Merged

wking closed this Sep 17, 2018

wking deleted the install-config-for-old-cmd branch September 17, 2018 17:44

This was referenced Sep 17, 2018

InstallConfig: Add machine-pool defaults #262

Closed

InstallConfig: Pointers for Networking, etc.? #263

Closed

InstallConfig machine pool typing and validation #264

Closed

yifan-gu mentioned this pull request Sep 18, 2018

asset: Add TerraformVariables asset to generated tfvar files #273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wking commented Sep 12, 2018 •

edited

Loading

openshift-ci-robot commented Sep 12, 2018

wking commented Sep 12, 2018

abhinavdahiya Sep 12, 2018

wking Sep 12, 2018 •

edited

Loading

abhinavdahiya Sep 12, 2018 •

edited

Loading

wking Sep 12, 2018

abhinavdahiya Sep 12, 2018 •

edited

Loading

abhinavdahiya Sep 12, 2018

abhinavdahiya commented Sep 12, 2018

wking commented Sep 12, 2018

abhinavdahiya commented Sep 12, 2018

abhinavdahiya commented Sep 12, 2018

wking commented Sep 12, 2018

abhinavdahiya commented Sep 12, 2018

wking commented Sep 12, 2018

dgoodwin Sep 13, 2018

wking Sep 13, 2018

dgoodwin Sep 13, 2018

dgoodwin Sep 13, 2018

wking Sep 13, 2018

dgoodwin Sep 13, 2018

abhinavdahiya Sep 13, 2018

wking Sep 13, 2018

dgoodwin Sep 14, 2018

wking commented Sep 13, 2018

wking commented Sep 14, 2018

wking commented Sep 14, 2018

wking commented Sep 15, 2018

wking commented Sep 15, 2018

wking commented Sep 17, 2018


		metav1.ObjectMeta `json:"metadata"`
		metav1.ObjectMeta `json:"metadata" yaml:"metadata"`

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

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

Conversation

wking commented Sep 12, 2018 • edited Loading

openshift-ci-robot commented Sep 12, 2018

wking commented Sep 12, 2018

Choose a reason for hiding this comment

wking Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

abhinavdahiya Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya commented Sep 12, 2018

wking commented Sep 12, 2018

abhinavdahiya commented Sep 12, 2018

abhinavdahiya commented Sep 12, 2018

wking commented Sep 12, 2018

abhinavdahiya commented Sep 12, 2018

wking commented Sep 12, 2018

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

wking commented Sep 13, 2018

wking commented Sep 14, 2018

wking commented Sep 14, 2018

wking commented Sep 15, 2018

wking commented Sep 15, 2018

wking commented Sep 17, 2018

wking commented Sep 12, 2018 •

edited

Loading

wking Sep 12, 2018 •

edited

Loading

abhinavdahiya Sep 12, 2018 •

edited

Loading

abhinavdahiya Sep 12, 2018 •

edited

Loading