OCPBUGS-24588: use new workload controller with preconditions #247

RomanBednar · 2024-07-24T14:11:19Z

ClusterCSIDriver condition when precondition fails:

  - lastTransitionTime: "2024-07-24T11:13:45Z"
    message: |
      dummy precondition 3 failed
    reason: PreconditionNotFulfilled
    status: "True"
    type: DeploymentDegraded

Controllers can be configured with csi-operator to have multiple preconditions, errors are then aggregated in ClusterCSIDriver:

- lastTransitionTime: "2024-07-24T11:13:45Z"
    message: |
      dummy error 2
      dummy error 3
    reason: PreconditionNotFulfilled
    status: "True"
    type: DeploymentDegraded

openshift-ci · 2024-07-24T14:11:23Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-07-24T14:11:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RomanBednar

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~~ [RomanBednar]

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

jsafrane · 2024-08-09T11:05:20Z

pkg/operator/config/config.go

+type Precondition func(ctx context.Context, c *clients.Clients) (bool, error)
+type PreconditionFunc func(context.Context) (bool, error)


Why there are two functions that do the same? IMO only PreconditionFunc should exist and perhaps be a type defined somewhere in the workload controller.

I don't remember, probably wanted to keep it separate but there can be just one - changed.

jsafrane · 2024-08-09T11:07:07Z

pkg/operator/config/config.go

+func (o *OperatorControllerConfig) AddPreconditions(c *clients.Clients, builders ...Precondition) {
+	for _, builder := range builders {
+		f := func(ctx context.Context) (bool, error) {
+			return builder(ctx, c)
+		}
+		o.Preconditions = append(o.Preconditions, f)
+	}


just add preconditions (better PreconditionFuncs) to o.Preconditions without any closure in between.

Ok, changed.

jsafrane · 2024-08-09T11:20:36Z

...thub.com/openshift/library-go/pkg/operator/deploymentcontroller/new_deployment_controller.go

+	required, err := c.getDeployment(opSpec)
+	if err != nil {
+		return nil, err
+	}


syncDeleting already has required in its parameters

Overlooked, removing.

jsafrane · 2024-08-09T11:25:33Z

...thub.com/openshift/library-go/pkg/operator/deploymentcontroller/new_deployment_controller.go

+		return nil, err
+	}
+
+	return required, nil


The controller conditions will be calculated from required, which is a Deployment loaded from yaml manifests, which is odd.

Currently, I am not sure what would be the right condition. Please add a TODO comment, we can sort it out later.

Personally, I think no conditions at all would be the best. I.e. the controller should remove Available / Progressing, an operator has to add their own condition in this case.

Indeed, dealing with conditions in operator makes sense. Adding a TODO comment for now.

jsafrane · 2024-08-09T11:33:55Z

...thub.com/openshift/library-go/pkg/operator/deploymentcontroller/new_deployment_controller.go

+	deployment, err = c.syncManaged(ctx, required, opStatus, syncContext)
+	if err != nil {
+		errors = append(errors, err)
+	}


This should be in else branch of the previous if. Otherwise the operator deletes the Deployment and then just re-creates it here.

Thanks for the catch, fixed.

openshift-ci-robot · 2024-10-01T15:12:50Z

@RomanBednar: This pull request references Jira Issue OCPBUGS-24588, which is invalid:

expected the bug to target the "4.18.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

ClusterCSIDriver condition when precondition fails:
 - lastTransitionTime: "2024-07-24T11:13:45Z"
   message: |
     dummy precondition 3 failed
   reason: PreconditionNotFulfilled
   status: "True"
   type: DeploymentDegraded
Controllers can be configured with csi-operator to have multiple preconditions, errors are then aggregated in ClusterCSIDriver:
- lastTransitionTime: "2024-07-24T11:13:45Z"
   message: |
     dummy error 2
     dummy error 3
   reason: PreconditionNotFulfilled
   status: "True"
   type: DeploymentDegraded

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 openshift-eng/jira-lifecycle-plugin repository.

RomanBednar · 2024-10-01T15:13:21Z

/hold

openshift/library-go#1815 has to be merged first followed by library-go bump here

RomanBednar · 2024-10-01T15:16:43Z

@jsafrane Moved library-go parts to separate PR and addressed all comments. If we can finish this part I think up next is to attempt workloads controller refactor so it can use daemon set as well and after testing this for a while migrate all operators to the new controller.

RomanBednar · 2024-10-02T08:13:37Z

/jira refresh

openshift-ci-robot · 2024-10-02T08:13:44Z

@RomanBednar: This pull request references Jira Issue OCPBUGS-24588, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.18.0) matches configured target version for branch (4.18.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (wduan@redhat.com), skipping review request.

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-12-02T21:32:00Z

@RomanBednar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-index-aws-efs-csi-driver-operator-bundle	`04ab0d7`	link	true	`/test ci-index-aws-efs-csi-driver-operator-bundle`
ci/prow/unit	`04ab0d7`	link	true	`/test unit`
ci/prow/smb-operator-e2e-extended	`04ab0d7`	link	false	`/test smb-operator-e2e-extended`
ci/prow/aws-efs-operator-e2e	`04ab0d7`	link	true	`/test aws-efs-operator-e2e`
ci/prow/e2e-azure-ovn-upgrade	`04ab0d7`	link	true	`/test e2e-azure-ovn-upgrade`
ci/prow/e2e-azure	`04ab0d7`	link	true	`/test e2e-azure`
ci/prow/e2e-azure-file-nfs-csi	`04ab0d7`	link	true	`/test e2e-azure-file-nfs-csi`
ci/prow/e2e-aws-ovn-upgrade	`04ab0d7`	link	true	`/test e2e-aws-ovn-upgrade`
ci/prow/smb-operator-e2e	`04ab0d7`	link	false	`/test smb-operator-e2e`
ci/prow/hypershift-aws-e2e-external	`04ab0d7`	link	true	`/test hypershift-aws-e2e-external`
ci/prow/security	`04ab0d7`	link	false	`/test security`
ci/prow/verify	`04ab0d7`	link	true	`/test verify`
ci/prow/images	`04ab0d7`	link	true	`/test images`
ci/prow/e2e-azure-csi-extended	`04ab0d7`	link	false	`/test e2e-azure-csi-extended`
ci/prow/e2e-azure-file-csi	`04ab0d7`	link	true	`/test e2e-azure-file-csi`
ci/prow/aws-efs-operator-e2e-extended	`04ab0d7`	link	false	`/test aws-efs-operator-e2e-extended`
ci/prow/e2e-azurestack-csi	`04ab0d7`	link	false	`/test e2e-azurestack-csi`
ci/prow/ci-index-smb-csi-driver-operator-bundle	`04ab0d7`	link	true	`/test ci-index-smb-csi-driver-operator-bundle`
ci/prow/e2e-azure-csi	`04ab0d7`	link	true	`/test e2e-azure-csi`
ci/prow/e2e-azure-file-csi-extended	`04ab0d7`	link	false	`/test e2e-azure-file-csi-extended`
ci/prow/e2e-aws-csi-extended	`04ab0d7`	link	false	`/test e2e-aws-csi-extended`
ci/prow/e2e-aws-csi	`04ab0d7`	link	true	`/test e2e-aws-csi`
ci/prow/e2e-openstack-cinder-csi	`04ab0d7`	link	true	`/test e2e-openstack-cinder-csi`
ci/prow/e2e-openstack-manila-csi	`04ab0d7`	link	true	`/test e2e-openstack-manila-csi`
ci/prow/hypershift-e2e-openstack-csi-manila	`04ab0d7`	link	true	`/test hypershift-e2e-openstack-csi-manila`
ci/prow/hypershift-e2e-openstack-csi-cinder	`04ab0d7`	link	true	`/test hypershift-e2e-openstack-csi-cinder`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2024

jsafrane reviewed Sep 30, 2024

View reviewed changes

use new workload controller with preconditions

04ab0d7

RomanBednar force-pushed the workload-controller-keep-api branch from 4592d04 to 04ab0d7 Compare October 1, 2024 15:10

RomanBednar changed the title ~~DRAFT: New deployment controller~~ OCPBUGS-24588: use new workload controller with preconditions Oct 1, 2024

RomanBednar mentioned this pull request Oct 1, 2024

OCPBUGS-24588: add new controller based on workload controller openshift/library-go#1815

Open

openshift-ci-robot added jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels Oct 1, 2024

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Oct 1, 2024

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

RomanBednar marked this pull request as ready for review October 2, 2024 08:12

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2024

openshift-ci bot requested review from bertinatto and jsafrane October 2, 2024 08:12

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-24588: use new workload controller with preconditions #247

OCPBUGS-24588: use new workload controller with preconditions #247

RomanBednar commented Jul 24, 2024 •

edited

Loading

openshift-ci bot commented Jul 24, 2024

openshift-ci bot commented Jul 24, 2024

jsafrane Aug 9, 2024

RomanBednar Oct 1, 2024

jsafrane Aug 9, 2024

RomanBednar Oct 1, 2024

jsafrane Aug 9, 2024

RomanBednar Oct 1, 2024

jsafrane Aug 9, 2024

RomanBednar Oct 1, 2024

jsafrane Aug 9, 2024

RomanBednar Oct 1, 2024

openshift-ci-robot commented Oct 1, 2024

RomanBednar commented Oct 1, 2024

RomanBednar commented Oct 1, 2024

RomanBednar commented Oct 2, 2024

openshift-ci-robot commented Oct 2, 2024

openshift-ci bot commented Dec 2, 2024

		type Precondition func(ctx context.Context, c *clients.Clients) (bool, error)
		type PreconditionFunc func(context.Context) (bool, error)

OCPBUGS-24588: use new workload controller with preconditions #247

Are you sure you want to change the base?

OCPBUGS-24588: use new workload controller with preconditions #247

Conversation

RomanBednar commented Jul 24, 2024 • edited Loading

openshift-ci bot commented Jul 24, 2024

openshift-ci bot commented Jul 24, 2024

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

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 1, 2024

RomanBednar commented Oct 1, 2024

RomanBednar commented Oct 1, 2024

RomanBednar commented Oct 2, 2024

openshift-ci-robot commented Oct 2, 2024

openshift-ci bot commented Dec 2, 2024

RomanBednar commented Jul 24, 2024 •

edited

Loading