operator: use infra and network manifests to create controllerconfigspec #357

abhinavdahiya · 2019-01-31T00:32:05Z

cluster-config-v1is going to be deprecated in favor of global configs 1. So moving the 2 major options that MCO requires etcd discovery domain and apiserver url to Infrastructure.config.openshift.io 2. As of 3 the installer is now setting those values and MCO can use global configs to fetch those 2 specific information.

abhinavdahiya · 2019-01-31T00:32:47Z

working on this, this is not ready for review. using it as a way to get release for local testing.

abhinavdahiya · 2019-01-31T00:37:02Z

also needs #351 to sync the status.go with vendor bump.
and openshift/installer#1013

cgwalters · 2019-02-01T13:58:35Z

Is this 4.0 work?

abhinavdahiya · 2019-02-01T20:43:50Z

sweet, this is green :yay: i'll start to cleanup and add details.

ashcrow · 2019-02-01T20:48:44Z

It looks like it is based on the referenced PR. @abhinavdahiya can you confirm?

abhinavdahiya · 2019-02-01T20:56:15Z

It looks like it is based on the referenced PR. @abhinavdahiya can you confirm?

Yes i need this PR to make progress on openshift/installer#1169. I will be filing for exception for all of it together.

ashcrow · 2019-02-01T21:10:52Z

/hold

Since the CI is green, holding until the exception is granted.

Since [1], the `Infrastructure.config.openshift.io` global config object contains the kube apiserver's url and the domain that needs to be used by etcd to do bootstrapping using dns discovery. The installer creates and sets the appropiate value since [2]. Therefore there is no longer a requirement for cluster_name and base_domain to create these values in template controller. [1]: openshift/api#189 [2]: openshift/installer#1155

abhinavdahiya · 2019-02-12T23:00:40Z

/hold

Since the CI is green, holding until the exception is granted.

This is required for openshift/installer#1169 and openshift/installer#1169 has approval.

So ping @runcom @cgwalters

infra object provides the etcd_discover_domain [1] and api_server_url, while the network object provides the service_network [3] used to calculate the cluster_dns_ip. currently the network objects's `ServiceNetwork` list only suports single service network [3] and therefore it is safe to only use the first element. [1]: https://github.com/openshift/api/blob/4912e102a00fa8bb4bf27f83808dce6ef1b6887d/config/v1/types_infrastructure.go#L42 [2]: https://github.com/openshift/api/blob/4912e102a00fa8bb4bf27f83808dce6ef1b6887d/config/v1/types_infrastructure.go#L47 [3]: https://github.com/openshift/api/blob/4912e102a00fa8bb4bf27f83808dce6ef1b6887d/config/v1/types_network.go#L32

abhinavdahiya · 2019-02-12T23:13:55Z

/hold
Since the CI is green, holding until the exception is granted.

This is required for openshift/installer#1169 and openshift/installer#1169 has approval.

So ping @runcom @cgwalters

/hold cancel

cgwalters · 2019-02-13T21:18:02Z

Gave this code a slightly-more-than-superficial review; overall LGTM.

kikisdeliveryservice

Some comments, mostly re: clarity

pkg/operator/operator.go

pkg/operator/render.go

kikisdeliveryservice · 2019-02-13T21:39:20Z

pkg/apis/machineconfiguration.openshift.io/v1/types.go

 }

 // MCOConfigSpec is the spec for MCOConfig resource.
 type MCOConfigSpec struct {


Is this just an empty struct?

Yes, we don't need to duplicate here and controllerconfigspec.

The one place it was used was deleted too (operator/render.go) so can I ask to understand why we keep it?

k8s object typically have spec and status. So this needs to staty but, for now empty.

MCOConfig can be expanded later on to drive operator level configuration like
maybe placement of controller, server etc.
log levels for its operands and such...

(Abhinav was faster than me at commenting so I didn't notice his comment before posting mine)

My take is this is still the MCO config object and we may add fields going forward (as well as deprecating and so on and so forth) so we stick it around but if we instead can remove it, let's nuke it

makes sense! @abhinavdahiya and @runcom (tag team!)

pkg/operator/bootstrap.go

pkg/operator/render.go

runcom · 2019-02-13T22:37:10Z

Apart from @kikisdeliveryservice comments, this LGTM, I'll properly lgtm it once comments are addressed

kikisdeliveryservice · 2019-02-13T23:24:46Z

Thanks for the updates and answers @abhinavdahiya . Was very helpful! 😺

runcom · 2019-02-13T23:28:36Z

/lgtm

openshift-ci-robot · 2019-02-13T23:28:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,runcom]

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

Bug 1930570: Update Jenkins monitored templates names

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 Jan 31, 2019

openshift-ci-robot requested review from ashcrow and jlebon January 31, 2019 00:32

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 31, 2019

abhinavdahiya mentioned this pull request Jan 31, 2019

Move DNS records from base_domain to cluster_domain openshift/installer#1169

Merged

6 tasks

abhinavdahiya force-pushed the move_to_global_config branch 5 times, most recently from f052bdd to 4e00452 Compare February 1, 2019 00:11

abhinavdahiya force-pushed the move_to_global_config branch 3 times, most recently from fa9a32b to 0a261d8 Compare February 1, 2019 18:47

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2019

abhinavdahiya force-pushed the move_to_global_config branch from 0a261d8 to fb20302 Compare February 1, 2019 21:45

abhinavdahiya changed the title ~~WIP: operator: use infra and network manifests to create controllerconfigspec~~ operator: use infra and network manifests to create controllerconfigspec Feb 5, 2019

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 5, 2019

abhinavdahiya added 3 commits February 12, 2019 14:56

controller/template: use updated controllerconfig

577f8ff

controller/kubelet-config: udpate tests to use update controllerconfig

b5d93f9

abhinavdahiya force-pushed the move_to_global_config branch from fb20302 to 87b6b26 Compare February 12, 2019 22:59

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2019

abhinavdahiya added 4 commits February 12, 2019 15:13

operator: update server daemonset to use new apiserver_url

936cc67

cmd: add new informers for infra and network

a31438a

vendor: bump openshift/{api,client-go}

936edd2

abhinavdahiya force-pushed the move_to_global_config branch from 87b6b26 to 936edd2 Compare February 12, 2019 23:13

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2019

kikisdeliveryservice reviewed Feb 13, 2019

View reviewed changes

pkg/operator: add doc comments

6b08eaf

openshift-ci-robot assigned runcom Feb 13, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 13, 2019

openshift-merge-robot merged commit 6b92718 into openshift:master Feb 14, 2019

osherdp pushed a commit to osherdp/machine-config-operator that referenced this pull request Apr 13, 2021

Merge pull request openshift#357 from waveywaves/sync-jenkins

7f25195

Bug 1930570: Update Jenkins monitored templates names

operator: use infra and network manifests to create controllerconfigspec #357

operator: use infra and network manifests to create controllerconfigspec #357

Uh oh!

Conversation

abhinavdahiya commented Jan 31, 2019

Uh oh!

abhinavdahiya commented Jan 31, 2019

Uh oh!

abhinavdahiya commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented Feb 1, 2019

Uh oh!

abhinavdahiya commented Feb 1, 2019

Uh oh!

ashcrow commented Feb 1, 2019

Uh oh!

abhinavdahiya commented Feb 1, 2019

Uh oh!

ashcrow commented Feb 1, 2019

Uh oh!

abhinavdahiya commented Feb 12, 2019

Uh oh!

abhinavdahiya commented Feb 12, 2019

Uh oh!

cgwalters commented Feb 13, 2019

Uh oh!

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kikisdeliveryservice Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

kikisdeliveryservice Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

runcom Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kikisdeliveryservice Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

runcom commented Feb 13, 2019

Uh oh!

kikisdeliveryservice commented Feb 13, 2019

Uh oh!

runcom commented Feb 13, 2019

Uh oh!

openshift-ci-robot commented Feb 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

abhinavdahiya commented Jan 31, 2019 •

edited

Loading

runcom Feb 13, 2019 •

edited

Loading