report template controller's status in ControllerConfig #339

abhinavdahiya · 2019-01-23T00:40:39Z

ControllerConfig is currently used to configure the template controller.
And, while all other controllers report progress on various objects like,

render controller reports status on machine config pools
node controller reports status on machine config pools etc.

Any status updates for template controller have been missing..

This adds status field to ControllerConfig to allow reporting various conditions.
Currently 3 conditions have been added for template controller namely, completed, running and failing.

The operator needs to know when the internal machineconfigs in template controller have been synced to the cluster.
In terms of operator done is when template controller reports completed, not failing and not running for that current generation of ControllerConfig.

This makes sure that we are installing the server and daemon when the controller is installing its operands which has been causing some races like #338.

/cc @cgwalters @ashcrow

cgwalters · 2019-01-23T11:19:55Z

Is this working towards a fix for #338 ?

abhinavdahiya · 2019-01-23T17:55:32Z

/test e2e-aws

abhinavdahiya · 2019-01-23T23:09:16Z

Is this working towards a fix for #338 ?

@cgwalters it is ready for review, updated the PR message too.

abhinavdahiya · 2019-01-24T00:30:19Z

failures from e2e-aws

[sig-storage] Subpath [Volume type: hostPathSymlink] should fail for new directories when readOnly specified in the volumeSource [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: hostPathSymlink] should support existing directories when readOnly specified in the volumeSource [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: hostPathSymlink] should support existing directory [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: hostPathSymlink] should support existing single file [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: hostPathSymlink] should support file as subpath [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: hostPathSymlink] should support non-existent path [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: hostPathSymlink] should support readOnly directory specified in the volumeMount [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: hostPathSymlink] should support readOnly file specified in the volumeMount [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: hostPath] should support existing directory [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: nfsPVC] should support existing directories when readOnly specified in the volumeSource [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: nfs] should support existing directory [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Subpath [Volume type: nfs] should support existing single file [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Volumes iSCSI [Feature:Volumes] should be mountable [Suite:openshift/conformance/parallel] [Suite:k8s]

/retest

kikisdeliveryservice · 2019-01-24T19:17:52Z

Pulling this down now, I should expect to see status in oc get -o yaml controllerconfig?

ashcrow · 2019-01-25T09:38:00Z

/retest

abhinavdahiya · 2019-01-25T18:57:52Z

Pulling this down now, I should expect to see status in oc get -o yaml controllerconfig?

Yes

ControllerConfig is currently used to configure the `template controller`. And, while all other controllers report progress on various objects like, - `render controller` reports status on `machine config pools` - `node controller` reports status on `machine config pools` etc. Any status updates for `template controller` have been missing.. This adds `status` field to `ControllerConfig` to allow reporting various conditions. Currently 3 conditions have been added for `template controller` namely, `completed`, `running` and `failing`.

The operator needs to know when the internal machineconfigs in `template controller` have been synced to the cluster. In terms of operator `done` is when `template controller` reports `completed`, not `failing` and not `running` for that current generation of `ControllerConfig`.

kikisdeliveryservice

Updated images in cluster to use this PR and can config that I now see:

status:
    conditions:
    - lastTransitionTime: 2019-01-25T22:59:17Z
      reason: sync completed towards (2) generation using controller version 3.11.0-507-gf19f48d4
      status: "True"
      type: TemplateContollerCompleted
    - lastTransitionTime: 2019-01-25T22:59:17Z
      status: "False"
      type: TemplateContollerRunning
    - lastTransitionTime: 2019-01-25T22:59:17Z
      status: "False"
      type: TemplateContollerFailing
    observedGeneration: 2

when I run: oc get -o yaml controllerconfig

kikisdeliveryservice

Updated review: Can we take a look at this logging to see if we can improve the UX?

pkg/operator/sync.go

cgwalters · 2019-01-25T23:15:37Z

pkg/operator/sync.go

+	return wait.Poll(controllerConfigCompletedInterval, controllerConfigCompletedTimeout, func() (bool, error) {
+		if err := isControllerConfigCompleted(resource, optr.ccLister.Get); err != nil {
+			glog.Errorf("controllerconfig is not completed: %v", err)
+			return false, nil


Should this return false, err ?

we want to try to poll until timeout to prevent us exiting on interruptions / transient errors. the pool time out is 1 minute, which seems not too long.

cgwalters · 2019-01-25T23:15:43Z

pkg/operator/sync.go

-		return optr.waitForDeploymentRollout(mcc)
+		var waitErrs []error
+		waitErrs = append(waitErrs, optr.waitForDeploymentRollout(mcc))
+		waitErrs = append(waitErrs, optr.waitForControllerConfigToBeCompleted(cc))


Interesting. OK, I think I see how this approach should solve the problem.

And so then it should obsolete this outstanding change: c8109db

kikisdeliveryservice

Double checked the logging tweak that I requested and it's resolved 👍
Will let @cgwalters loop back in to give his final review.

cgwalters · 2019-01-26T07:09:21Z

/lgtm

openshift-ci-robot · 2019-01-26T07:09:29Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [abhinavdahiya,cgwalters]

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

ashcrow · 2019-01-26T09:06:57Z

/test e2e-aws

ashcrow · 2019-01-26T10:40:26Z

/test e2e-aws

ashcrow · 2019-01-26T12:22:57Z

/retest

ashcrow · 2019-01-26T14:23:43Z

/test e2e-aws

openshift-bot · 2019-01-26T22:04:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Add IBM Cloud managed annotations to CVO manifests

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 23, 2019

openshift-ci-robot requested review from crawford and kikisdeliveryservice January 23, 2019 00:40

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

abhinavdahiya force-pushed the template_status branch 2 times, most recently from 0eb682b to cc394d7 Compare January 23, 2019 02:05

abhinavdahiya force-pushed the template_status branch 3 times, most recently from 6224fb0 to 802ff4a Compare January 23, 2019 23:06

abhinavdahiya changed the title ~~WIP: report template controller's status in ControllerConfig~~ report template controller's status in ControllerConfig Jan 23, 2019

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

abhinavdahiya added 4 commits January 25, 2019 10:59

controller/template: report status in controllerconfig

ca8ebc0

operator: update the controllerconfig crd to include status subresource

af86138

abhinavdahiya force-pushed the template_status branch from 802ff4a to f19f48d Compare January 25, 2019 19:00

kikisdeliveryservice approved these changes Jan 25, 2019

View reviewed changes

kikisdeliveryservice mentioned this pull request Jan 25, 2019

fix initial race between template and render sub-controllers #338

Closed

kikisdeliveryservice suggested changes Jan 25, 2019

View reviewed changes

pkg/operator/sync.go Outdated Show resolved Hide resolved

cgwalters reviewed Jan 26, 2019

View reviewed changes

operator: wait for controllerconfig status when syncing controller

4d1fffa

abhinavdahiya force-pushed the template_status branch from f19f48d to 4d1fffa Compare January 26, 2019 00:10

kikisdeliveryservice approved these changes Jan 26, 2019

View reviewed changes

openshift-ci-robot assigned cgwalters Jan 26, 2019

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

openshift-merge-robot merged commit 59b15ca into openshift:master Jan 27, 2019

abhinavdahiya deleted the template_status branch February 5, 2019 19:09

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

Merge pull request openshift#339 from csrwng/roks_profile

28f0879

Add IBM Cloud managed annotations to CVO manifests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report template controller's status in ControllerConfig #339

report template controller's status in ControllerConfig #339

abhinavdahiya commented Jan 23, 2019 •

edited

cgwalters commented Jan 23, 2019

abhinavdahiya commented Jan 23, 2019

abhinavdahiya commented Jan 23, 2019

abhinavdahiya commented Jan 24, 2019

kikisdeliveryservice commented Jan 24, 2019

ashcrow commented Jan 25, 2019

abhinavdahiya commented Jan 25, 2019

kikisdeliveryservice left a comment •

edited

kikisdeliveryservice left a comment •

edited

cgwalters Jan 25, 2019

abhinavdahiya Jan 26, 2019

cgwalters Jan 25, 2019

kikisdeliveryservice left a comment

cgwalters commented Jan 26, 2019

openshift-ci-robot commented Jan 26, 2019

ashcrow commented Jan 26, 2019

ashcrow commented Jan 26, 2019

ashcrow commented Jan 26, 2019

ashcrow commented Jan 26, 2019

openshift-bot commented Jan 26, 2019

report template controller's status in ControllerConfig #339

report template controller's status in ControllerConfig #339

Conversation

abhinavdahiya commented Jan 23, 2019 • edited

cgwalters commented Jan 23, 2019

abhinavdahiya commented Jan 23, 2019

abhinavdahiya commented Jan 23, 2019

abhinavdahiya commented Jan 24, 2019

kikisdeliveryservice commented Jan 24, 2019

ashcrow commented Jan 25, 2019

abhinavdahiya commented Jan 25, 2019

kikisdeliveryservice left a comment • edited

Choose a reason for hiding this comment

kikisdeliveryservice left a comment • edited

Choose a reason for hiding this comment

cgwalters Jan 25, 2019

Choose a reason for hiding this comment

abhinavdahiya Jan 26, 2019

Choose a reason for hiding this comment

cgwalters Jan 25, 2019

Choose a reason for hiding this comment

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

cgwalters commented Jan 26, 2019

openshift-ci-robot commented Jan 26, 2019

ashcrow commented Jan 26, 2019

ashcrow commented Jan 26, 2019

ashcrow commented Jan 26, 2019

ashcrow commented Jan 26, 2019

openshift-bot commented Jan 26, 2019

abhinavdahiya commented Jan 23, 2019 •

edited

kikisdeliveryservice left a comment •

edited

kikisdeliveryservice left a comment •

edited