bug 1804717: Switch to managing openshift-apiserver pods with a deployment #313

marun · 2020-02-20T06:42:36Z

The switch from using a daemonset to using a deployment is intended to improve reliability during upgrade.

bz: https://bugzilla.redhat.com/show_bug.cgi?id=1804717

/cc @sttts @deads2k

marun · 2020-02-20T06:47:06Z

@deads2k Some questions:

should removal of the daemonset block creation of the deployment (and vise-versa for cleanup in previous releases on downgrade)?
the bz mentioned wiring HPA to enable scaling - should that be tackled in this PR or separately?

pkg/operator/starter.go

pkg/operator/workloadcontroller/workload_controller_openshiftapiserver_v311_00.go

deads2k · 2020-02-20T15:59:40Z

should removal of the daemonset block creation of the deployment (and vise-versa for cleanup in previous releases on downgrade)?

no, just rollout. We can tolerate having some extras for a bit.

marun · 2020-02-20T16:31:12Z

@sttts Updated, PTAL

bindata/v3.11.0/openshift-apiserver/deploy.yaml

deads2k · 2020-02-20T17:34:39Z

pkg/operator/starter.go

@@ -246,6 +248,10 @@ func RunOperator(ctx context.Context, controllerConfig *controllercmd.Controller
 			"Progressing",
 			// in 4.1.0-4.3.0 this was used for indicating the apiserver daemonset was available
 			"Available",
+			// in 4.4.0 this was used for indicating the conditions of the apiserver daemonset.


4.1.0-4.3.z

deads2k · 2020-02-20T17:39:15Z

pkg/operator/starter.go

+	// It's safe not to check the error returned by the poll function so long
+	// as conditionFunc never returns an error. The poll function will only
+	// exit on success or if the stop channel halts execution.
+	go wait.PollImmediateUntil(time.Minute, func() (bool, error) {


UntilWithContext please. This should run forever. I'm ok doing it on two cadences if you prefer (fast until we delete, slow after that), but this will run every time the pod restarts, which is just a controller on an unpredictable resync cadence.

Done. I didn't bother with 2 cadences, can address that if you feel strongly.

marun · 2020-02-20T18:51:37Z

@deads2k Updated, PTAL

deads2k · 2020-02-20T19:01:33Z

/lgtm

this could be improved by emitting some events, but we need this badly enough not to quibble.

marun · 2020-02-20T19:05:06Z

@deads2k What events would you like to see? Successful deletion of daemonset? Deployment doesn't have replicas yet?

marun · 2020-02-24T17:44:54Z

/retest

marun · 2020-02-25T03:58:13Z

/retest

The switch from using a daemonset to using a deployment is intended to improve reliability during upgrade.

marun · 2020-02-25T17:13:13Z

Updated to use explicit tolerations which should result in e2e passing, PTAL.

marun · 2020-02-25T19:55:16Z

/retest

marun · 2020-02-25T23:26:37Z

/test all

marun · 2020-02-26T01:24:57Z

/retest

marun · 2020-02-26T04:56:51Z

/test all

sttts · 2020-02-26T08:40:15Z

bindata/v3.11.0/openshift-apiserver/deploy.yaml

 metadata:
  namespace: openshift-apiserver
  name: apiserver
  labels:
    app: openshift-apiserver
    apiserver: "true"
 spec:
-  updateStrategy:
-    type: RollingUpdate
+  replicas: 3


what does this mean for single-node clusters? Will we be always progressing?

shouldn't that be observed from the number of masters in the cluster? if someone scales them to 5?

3 instances in a 5 master node cluster at least sounds sane and doesn't break.

what does this mean for single-node clusters? Will we be always progressing?

Good catch. The logic should be determining health by checking whether the replica count is equal or greater to the number of masters to maintain the invariant previously guaranteed by the daemonset.

3 instances in a 5 master node cluster at least sounds sane and doesn't break.

Is that preferable to adjusting the replica count according to the number of masters that are present?

single node clusters are not a supported platform. Refinements could be made, but I don't see them as blocking us stablizing upgrades for supported platforms.

sttts · 2020-02-26T08:41:31Z

pkg/operator/workloadcontroller/workload_controller_openshiftapiserver_v311_00.go

@@ -363,6 +369,7 @@ func manageOpenShiftAPIServerDaemonSet_v311_00_to_latest(

 	required.Labels["revision"] = strconv.Itoa(int(operatorConfig.Status.LatestAvailableRevision))
 	required.Spec.Template.Labels["revision"] = strconv.Itoa(int(operatorConfig.Status.LatestAvailableRevision))
+	required.Spec.Template.Labels["previousGeneration"] = strconv.Itoa(int(operatorConfig.Status.LatestAvailableRevision))


Removed. I have no recollection of where that came from.

sttts · 2020-02-26T08:50:10Z

pkg/operator/workloadcontroller/workload_controller_openshiftapiserver_v311_00.go

+		desiredReplicas = *(actualDeployment.Spec.Replicas)
+	}
+
+	deploymentHasAllPodsAvailable := actualDeployment.Status.AvailableReplicas == desiredReplicas


do we enforce that two replicasets for the deployment don't overlap and increase this number temporarily although we are still progressing?

No, we do not enforce no overlap. As per your previous comment, health should be determined by comparing replica count to master count.

tnozicka · 2020-02-26T10:37:14Z

The switch from using a daemonset to using a deployment is intended to improve reliability during upgrade.

it would be nice if the description said why

marun · 2020-02-26T15:14:01Z

The switch from using a daemonset to using a deployment is intended to improve reliability during upgrade.

it would be nice if the description said why

I'll leave it to @deads2k to explain. I have no idea why and the bz is short on details.

deads2k · 2020-02-27T00:54:33Z

The switch from using a daemonset to using a deployment is intended to improve reliability during upgrade.

it would be nice if the description said why

There is an issue draining nodes. Daemonsets are not evicted early. When the termination finally happens, there is an issue where graceful deletion doesn't take place. In addition, it is opening us up to SDN restart issues which result in API outages.

deads2k · 2020-02-27T00:56:17Z

/lgtm

openshift-ci-robot · 2020-02-27T00:56:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, marun

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

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

deads2k · 2020-02-27T00:56:49Z

/cherrypick release-4.4

openshift-cherrypick-robot · 2020-02-27T00:56:50Z

@deads2k: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.4

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/test-infra repository.

openshift-ci-robot · 2020-02-27T00:59:45Z

@marun: All pull requests linked via external trackers have merged. Bugzilla bug 1804717 has been moved to the MODIFIED state.

In response to this:

bug 1804717: Switch to managing openshift-apiserver pods with a deployment

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/test-infra repository.

openshift-cherrypick-robot · 2020-02-27T00:59:59Z

@deads2k: #313 failed to apply on top of branch "release-4.4":

Applying: Update bindata
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/operator/v311_00_assets/bindata.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/operator/v311_00_assets/bindata.go
CONFLICT (content): Merge conflict in pkg/operator/v311_00_assets/bindata.go
Patch failed at 0002 Update bindata

In response to this:

/cherrypick release-4.4

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/test-infra repository.

marun · 2020-02-27T07:13:12Z

#324 addresses outstanding comments on this PR.

If the cherry-pick bot can't apply cleanly, does that imply that a manual PR is required?

openshift-ci-robot requested review from deads2k and sttts February 20, 2020 06:42

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 20, 2020

sttts reviewed Feb 20, 2020

View reviewed changes

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

sttts reviewed Feb 20, 2020

View reviewed changes

pkg/operator/workloadcontroller/workload_controller_openshiftapiserver_v311_00.go Show resolved Hide resolved

marun force-pushed the ds-to-deploy branch from 2506a8a to afbfda5 Compare February 20, 2020 15:58

marun force-pushed the ds-to-deploy branch from afbfda5 to 8f5fefa Compare February 20, 2020 16:30

marun force-pushed the ds-to-deploy branch 3 times, most recently from 17e9b5a to 41b66fa Compare February 20, 2020 16:43

marun changed the title ~~WIP Bug 1804717 Switch to managing openshift-apiserver pods with a deployment~~ Bug 1804717 Switch to managing openshift-apiserver pods with a deployment Feb 20, 2020

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 Feb 20, 2020

marun force-pushed the ds-to-deploy branch 2 times, most recently from ce422f9 to e67a91f Compare February 20, 2020 16:57

deads2k reviewed Feb 20, 2020

View reviewed changes

bindata/v3.11.0/openshift-apiserver/deploy.yaml Outdated Show resolved Hide resolved

deads2k reviewed Feb 20, 2020

View reviewed changes

marun force-pushed the ds-to-deploy branch 2 times, most recently from 53d1f90 to d1c4d90 Compare February 20, 2020 18:51

marun force-pushed the ds-to-deploy branch from d1c4d90 to cdf96d9 Compare February 20, 2020 18:58

openshift-ci-robot assigned deads2k Feb 20, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 20, 2020

marun added 2 commits February 25, 2020 09:09

Switch to managing openshift-apiserver pods with a deployment

cba63e5

The switch from using a daemonset to using a deployment is intended to improve reliability during upgrade.

Update bindata

f8ca95f

marun force-pushed the ds-to-deploy branch from 99f9ae5 to f8ca95f Compare February 25, 2020 17:11

sttts reviewed Feb 26, 2020

View reviewed changes

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2020

openshift-merge-robot merged commit 9eda24a into openshift:master Feb 27, 2020

marun deleted the ds-to-deploy branch February 27, 2020 06:58

marun mentioned this pull request Feb 27, 2020

bug 1804717: Cleanup switch to managing openshift-apiserver pods with a deployment #324

Merged

marun mentioned this pull request Feb 27, 2020

Bug 1805872: 4.4 cherry-pick of switch from daemonset to deployment #325

Merged

atiratree mentioned this pull request Sep 21, 2022

manage openshift-controller-manager as a Deployment openshift/cluster-openshift-controller-manager-operator#264

Merged

1 task

bug 1804717: Switch to managing openshift-apiserver pods with a deployment #313

bug 1804717: Switch to managing openshift-apiserver pods with a deployment #313

marun commented Feb 20, 2020

marun commented Feb 20, 2020

deads2k commented Feb 20, 2020

marun commented Feb 20, 2020

deads2k Feb 20, 2020

marun Feb 20, 2020

deads2k Feb 20, 2020

marun Feb 20, 2020

marun commented Feb 20, 2020

deads2k commented Feb 20, 2020

marun commented Feb 20, 2020

marun commented Feb 24, 2020

marun commented Feb 25, 2020

marun commented Feb 25, 2020

marun commented Feb 25, 2020

marun commented Feb 25, 2020

marun commented Feb 26, 2020

marun commented Feb 26, 2020

sttts Feb 26, 2020

tnozicka Feb 26, 2020

sttts Feb 26, 2020

marun Feb 26, 2020

marun Feb 26, 2020

deads2k Feb 27, 2020

sttts Feb 26, 2020

marun Feb 26, 2020

sttts Feb 26, 2020

marun Feb 26, 2020

tnozicka commented Feb 26, 2020

marun commented Feb 26, 2020

deads2k commented Feb 27, 2020

deads2k commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

deads2k commented Feb 27, 2020

openshift-cherrypick-robot commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

openshift-cherrypick-robot commented Feb 27, 2020

marun commented Feb 27, 2020

bug 1804717: Switch to managing openshift-apiserver pods with a deployment #313

bug 1804717: Switch to managing openshift-apiserver pods with a deployment #313

Conversation

marun commented Feb 20, 2020

marun commented Feb 20, 2020

deads2k commented Feb 20, 2020

marun commented Feb 20, 2020

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

marun commented Feb 20, 2020

deads2k commented Feb 20, 2020

marun commented Feb 20, 2020

marun commented Feb 24, 2020

marun commented Feb 25, 2020

marun commented Feb 25, 2020

marun commented Feb 25, 2020

marun commented Feb 25, 2020

marun commented Feb 26, 2020

marun commented Feb 26, 2020

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

tnozicka commented Feb 26, 2020

marun commented Feb 26, 2020

deads2k commented Feb 27, 2020

deads2k commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

deads2k commented Feb 27, 2020

openshift-cherrypick-robot commented Feb 27, 2020

openshift-ci-robot commented Feb 27, 2020

openshift-cherrypick-robot commented Feb 27, 2020

marun commented Feb 27, 2020