Skip to content

Conversation

@stlaz
Copy link
Contributor

@stlaz stlaz commented Jan 22, 2019

Adds a library-go controller for status changes and also
adds status failure reporting in auth operator Sync() if an error
occured in the lower level machinery.

Based on current library-go and kube-apiserver-operator code.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 22, 2019
[]configv1.ObjectReference{
{Group: "authentication.operator.openshift.io", Resource: "authenticationoperatorconfig", Name: globalConfigName},
{Resource: "namespaces", Name: targetName}, // TODO: fix after rename PR?
{Resource: "namespaces", Name: "openshift-core-operators"}, // TODO: fix after rename 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.

Do we need more resources here?

@stlaz
Copy link
Contributor Author

stlaz commented Jan 22, 2019

The weird order of commits is caused by Github ordering them based on their date rather than the git commit tree order.

@stlaz
Copy link
Contributor Author

stlaz commented Jan 22, 2019

/retest

@enj
Copy link
Contributor

enj commented Jan 23, 2019

/hold

Wait for #34

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 23, 2019
@stlaz
Copy link
Contributor Author

stlaz commented Jan 24, 2019

Rebased on top of current master.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2019
@stlaz
Copy link
Contributor Author

stlaz commented Jan 30, 2019

Rebased

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 30, 2019
@stlaz
Copy link
Contributor Author

stlaz commented Feb 11, 2019

/hold cancel
#34 was merged long ago

@openshift-ci-robot 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 11, 2019
@stlaz stlaz force-pushed the status_updates branch 2 times, most recently from bd505e5 to b38d9e2 Compare February 12, 2019 12:37
@derekwaynecarr
Copy link
Member

/test e2e-aws

@stlaz
Copy link
Contributor Author

stlaz commented Feb 13, 2019

/retest

@stlaz
Copy link
Contributor Author

stlaz commented Feb 13, 2019

For some reason I'm seeing

E0213 15:37:30.023897       1 operator.go:173] error updating operator status: Operation cannot be fulfilled on authentications.operator.openshift.io "cluster": the object has been modified; please apply your changes to the latest version and try again
E0213 15:37:30.027876       1 operator.go:176] error updating operator status: Operation cannot be fulfilled on authentications.operator.openshift.io "cluster": the object has been modified; please apply your changes to the latest version and try again

@ericavonb
Copy link
Contributor

/test e2e-aws

@stlaz
Copy link
Contributor Author

stlaz commented Feb 19, 2019

/retest

3 similar comments
@ericavonb
Copy link
Contributor

/retest

@stlaz
Copy link
Contributor Author

stlaz commented Feb 20, 2019

/retest

@ericavonb
Copy link
Contributor

/retest

@stlaz stlaz force-pushed the status_updates branch 3 times, most recently from db68724 to 03fd577 Compare February 20, 2019 14:41
@stlaz
Copy link
Contributor Author

stlaz commented Feb 20, 2019

/retest

1 similar comment
@ericavonb
Copy link
Contributor

/retest

@ericavonb
Copy link
Contributor

/test e2e-aws-operator

}

// TODO update states and handle ClusterOperator spec/status
if statusErr := c.setAvailableStatus(operatorConfig); statusErr != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wholly insufficient - ex: if the deployment fails to rollout we would be stating everything is fine.

@enj
Copy link
Contributor

enj commented Feb 21, 2019

I just did an initial pass review. This is not ready.

This adds a library-go controller for status changes and also
adds status failure reporting in auth operator Sync() if an error
occured in the lower level machinery.

TODO: add a clusteroperator manifest to be deployed by CVO once
we are sure we are not breaking anything
@stlaz
Copy link
Contributor Author

stlaz commented Feb 21, 2019

Sorry about the sorry state of the PR, I think I originally set it up so that we can build up on it, but even back then it was not very complete.

Most comments are fixed, yet the most important still stands and that's the one about when we should report as available. I'll need to see about that.

clusterOperatorStatus := status.NewClusterOperatorStatusController(
targetName,
[]configv1.ObjectReference{
{Group: operatorv1.GroupName, Resource: "authentications", Name: globalConfigName},
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add the infrastructure top level config per #79

@ericavonb
Copy link
Contributor

/retest

@ericavonb
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericavonb, stlaz

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:

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

@ericavonb
Copy link
Contributor

/retest e2e-aws-operator

@ericavonb
Copy link
Contributor

/retest

@openshift-merge-robot openshift-merge-robot merged commit 78dd53b into openshift:master Feb 23, 2019
@enj enj mentioned this pull request Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants